-
Notifications
You must be signed in to change notification settings - Fork 45
Refactor: Rename Step to Vector2 and include vector operations #437
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Refactor: Rename Step to Vector2 and include vector operations #437
Conversation
Also extract Step from WalkingQueue and move to io.luna.math
Also extract Step from WalkingQueue and move to io.luna.math
Not to be overly negative here, and I havnt done any benchmarks on this, but it looks like a regression. Direction between? It might be that ive not had coffee yet, but even bounds checks you would want to include the plane its on for collision for area of effect checks. In rs most distance calculations (including attack ranges use https://en.wikipedia.org/wiki/Chebyshev_distance) and so it is much simpler to just include the calculation in the Tile/Location or whatever the class in Luna is to avoid creating placeholder objects. There are a few pieces of content that use a different formula. So with this in mind, what content would a "vector2" be used for outside of movement in the context of rs? Maybe Lare can chime in, but i cant think of any times ive thought to myself, "I need a vector2 class" If you are wanting to pull the step class out an IntPair or similar class is useful for some content and I prefer using it to kotlins built in pair sometimes to avoid any issues with allocations/boxing. However a generic intpair class you do lose information on what the x/y is. |
Includes additional documentation for the class and associated test cases.
You are coming off a bit unfriendly but I'll overlook it and address some of your concerns :)
Direction is an enum that stores a vector. If we can calculate the vector directly, then we don't need a bunch of if statements to select a vector. Maybe I missed something. What are "the rest of the checks" exactly? After reviewing my code, I think I overengineered the normalization function. Thanks for pointing out signum 👍🏽
If we include the plane (z-axis), we now have a Vector3 (x, y, and z). In Luna a Vector3 is called Position. Why would we need the z-axis for a bounding box check anyways? It's a box.
Chesbyshev distance is already implemented. If you notice in the function header, the return type is an int. That int will eventually have to be applied to a vector (2 or 3) anyways. Now that we have a vector class, we can write those operations in-line.
It depends what you consider simpler. Yes, in the short term we could cram a bunch of related functions into a file. But in a different system we might want to reuse code from that class without all of the additional context. Wouldn't it be nice to have a simple class that has all of the basic functions without the bloat? This is why I chose to rename Step to Vector2. Step has implied context around movement whereas Vector2 is generic.
I think you are referring to using the 'new' operator on vector operations. You are right. I should be reusing the same object. Good catch👍🏽
Vector2 is a pretty primitive class. It exists is nearly every game engine, and for good reason. There's an infinite amount of uses for vectors. Projectile direction and speed, rotations, knockbacks, explosions, pathfinding, wandering radius, following, vision cones, event radius. That's just on the backend. It's also used heavily for rendering, animations, and even UI. There's more but you get the idea.
:) |
…ing the vector object.
I don't think anyone has malicious intent here — it's all for the love of the project. I can see the thought that went into this PR, but I'm struggling to see how this would be used in practice. What does this do that the |
I believe the default should lean toward using Vector2 when a feature only depends on (x, y) coordinates. It’s more efficient in terms of memory, and more importantly, it communicates intent clearly — highlighting that height isn’t part of the equation. For cases that truly require (x, y, z), we already have Position. These aren't competing classes but complementary tools — each designed for different spatial needs. Choosing the one that fits the job best makes the system cleaner and easier to maintain. In fact, we already have existing features that could benefit from this distinction and would be simplified by using Vector2. A few examples: Area: No Z-axis usage across the interface or implementations. We could also standardize by replacing java.awt.Point with Vector2. Chunk: There’s redundancy we could clean up by expressing this in 2D terms. PathfindingAlgorithm: All current implementations are effectively 2D, making this a strong candidate for conversion. This isn’t about pushing a major structural change, but rather embracing small, intentional choices that make the codebase clearer, more consistent, and better aligned with how the systems actually behave. |
Just as a note, chunk shouldnt be in 2D terms. Should be in 3D imho. This helps them feel more realistic and trigger region changes when there is a heightlevel change to ensure things like ground items, objects, update properly. Doing this helps because you can hook into it for the chunk packets/zone prots. These need to know the original height data for easy rendering in the client. Regarding memory efficiency. You mention complexity etc and memory, but if every npc/player/object etc has a specific class that stores their position, it makes sense for any checks for contains and other methods to use that class rather than introducing something else that requires converting an object to a Vector2 in order to do some operations. Not only is this wasteful object conversion, but I personally think its clearer to have the relevant methods in 1 place and have a nice autocomplete. However, this thread isnt without merit. Regarding highlighting Pathfinding Algorithm, that is in the same vein was this step class earlier in walking. But ultimately it would jsut be a simple x/y class and not need multiple operations/additional methods. Regarding the comments on java.awt.Point, thats valid and Im not fond of incorporating awt classes etc in server sided applications. Depending what you call you can trip static blocks in the awt code and trigger the jvm to load a bunch of useless crap from a server perspective. So with that in line, I personally think itd make the most sense to rename Step->Point rather than Vector2D (not using vector name because it would be lacking vector operations) and then just having point have x/y and then a constructor/equals method. |
You would need the if statements to convert that Vector2D as you would get here to the direction object. Part of player updating is to write the direction information to the client. This is used by walking and then also by facing changes |
Valid point. This can be lifted to a separate issue for further investigation if you'd like.
I haven't spent much time figuring out how the map works to determine if this is a valid strategy. This could be another issue you can raise as a possible improvement.
I agree. Npcs, players, and objects live in 3D space. I'm not sure what this has to do with Vector2.
These vector operations can, and should imo, be applied to Position as well. We can discuss its merit in a separate issue.
I don't think this is a valid reason to exclude methods from a class. These methods can and will be used elsewhere. |
This change includes moving the class to a more logical package and adding some basic test coverage.
I've also deprecated Direction.between() in favor of Vector2.directionTowards().
All of the vector operations return a new instance of Vector2 to enable chaining of operations.
Vector operations can be reused across different systems outside of movement. This will be useful for area-of-effect capabilities, attack ranges, and a lot more.