-
-
Notifications
You must be signed in to change notification settings - Fork 162
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
Add clone() to Vector classes and add new Mutable Vector Classes #1137
base: 2.0
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would be making immutable vector types mutable, which is a major change in behavior and will have unintended side effects
Please either return a new vector instead of modifying the existing one - or create a new mutable vector type
Understandably, I didn't want to have to do this but I get it. I'll make |
@booky10 I've made appropriate changes I think you would find acceptable. I've added a mutable variant and an interface they both share. Is this design acceptable to you? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This design looks good 👍
You could maybe move now duplicate logic to the interface (e.g. read/write/equals/etc.)
api/src/main/java/com/github/retrooper/packetevents/util/Vector3f.java
Outdated
Show resolved
Hide resolved
api/src/main/java/com/github/retrooper/packetevents/util/Vector3i.java
Outdated
Show resolved
Hide resolved
return z; | ||
} | ||
|
||
public boolean equals(VectorMutable3i other) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldnt the parameter be the interface? (Same for every other method in this class)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JVM can't devirtualize and we have to use getter methods if we use the interface. I could have separate equals methods for Vector3d
and VectorMutable3d
in both Vector3d
and VectorMutable3d
I suppose...
The original idea was to have a fast equals path when you know you're working with a Vector3d and another Vector3d or a VectorMutable3d and VectorMutable3d. When you're working purely with the interface though you'll call the slower equals(Object)
api/src/main/java/com/github/retrooper/packetevents/util/VectorMutable3i.java
Outdated
Show resolved
Hide resolved
api/src/main/java/com/github/retrooper/packetevents/util/VectorMutable3f.java
Show resolved
Hide resolved
api/src/main/java/com/github/retrooper/packetevents/util/VectorInterface3d.java
Outdated
Show resolved
Hide resolved
api/src/main/java/com/github/retrooper/packetevents/util/VectorInterface3f.java
Show resolved
Hide resolved
api/src/main/java/com/github/retrooper/packetevents/util/VectorInterface3i.java
Show resolved
Hide resolved
Ok I pushed some new changes to VectorInterface3d but not the others just as a demo. I don't actually really like this design because there are few times if any where you want to directly work with and between mutable and immutable vectors, they should stay largely separate and I hate having to eat the virtualization cost of the abstraction. I would appreciate it if you went through VectorInterface3d and told me what methods you would be okay with stripping, because I personally want to keep it very minimal. |
public VectorMutable3d multiply(double x, double y, double z) { | ||
this.x -= x; | ||
this.y -= y; | ||
this.z -= z; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.x *= x;
this.y *= y;
this.z *= z;
Allow Vector3d, f, and i classes to be cloned and add setX, setY, and setZ methods to match Bukkit Vector abilities.
This is intended as part of the work in porting Grim to being multi-platform, which involves removing all usage of Bukkit Vectors, nevertheless we need to replace their functionality and would like to remain compatible with upstream PE.
I can understand if mutability in Vector3d, f, and i are things that you don't want changed, and if that's the case I'll instead work with you in finding a solution that allows us to not break from using PE vectors, such as adding new Vector3dI, Vector3fI, etc.. classes as immutable variants and having Vector3d and Vector3dI share the same interface which is used internally in PE code that takes the vectors as arguments instead of
Vector3d
.This would allow developers to choose immutability if they so wish and use them with the PE methods that take Vector objects as arguments (there aren't that many so it'll be an easy fix).