Skip to content
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

Open
wants to merge 7 commits into
base: 2.0
Choose a base branch
from

Conversation

Axionize
Copy link
Contributor

@Axionize Axionize commented Feb 12, 2025

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).

Copy link
Collaborator

@booky10 booky10 left a 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

@Axionize
Copy link
Contributor Author

Understandably, I didn't want to have to do this but I get it.

I'll make Vector3XMutable classes for each type

@Axionize
Copy link
Contributor Author

Axionize commented Feb 12, 2025

@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?

@Axionize Axionize changed the title Add clone() and setX, Y, and Z methods to Vector classes Add clone() to Vector classes and add new Mutable Vector Classes Feb 12, 2025
Copy link
Collaborator

@booky10 booky10 left a 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.)

return z;
}

public boolean equals(VectorMutable3i other) {
Copy link
Collaborator

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)

Copy link
Contributor Author

@Axionize Axionize Feb 13, 2025

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)

@Axionize
Copy link
Contributor Author

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.

Comment on lines +186 to +189
public VectorMutable3d multiply(double x, double y, double z) {
this.x -= x;
this.y -= y;
this.z -= z;
Copy link

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;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants