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

Make Vector[23] immutable #4

Open
SOF3 opened this issue Nov 4, 2018 · 3 comments
Open

Make Vector[23] immutable #4

SOF3 opened this issue Nov 4, 2018 · 3 comments
Labels
enhancement New feature or request

Comments

@SOF3
Copy link
Member

SOF3 commented Nov 4, 2018

Vector2 and Vector3 mutability is not self-documented. This is unclear in program design.

For performance reasons, it is possible to want to make a vector mutable.
For API design reasons, it is possible to want to make a vector immutable.

This issue proposes the separation of mutability from the vector classes.

See https://medium.com/@elye.project/read-only-collection-in-kotlin-leads-to-better-coding-40cdfa4c6359 for a similar discussion about kotlin collections.

@SOF3 SOF3 added the enhancement New feature or request label Nov 4, 2018
@dktapps
Copy link
Member

dktapps commented Dec 16, 2018

This can be solved by moving Vector3 setters into a MutableVector3 class and making Vector3's fields protected. However, this has some problems with relation to PM since there's a vast array of things which depend on Vector3 being mutable (blocks, entities). We also have PM Position and Location to consider.

@dktapps
Copy link
Member

dktapps commented May 10, 2021

My preferred direction for this is to just make Vector3 entirely immutable.

In addition, my previous recommendation for MutableVector3 would have been a bad idea, because it would have also made Vector3 implicitly mutable (anything that accepted Vector3 wouldn't be able to rely on the immutability of whatever got passed).

@dktapps dktapps changed the title Separate mutable functionalities of Vector[23] Make Vector[23] immutable Oct 24, 2021
@dktapps
Copy link
Member

dktapps commented Oct 24, 2021

Might be a good idea to wait until after 8.1 minimum is imposed for this. 8.1 has readonly for properties, which would allow us to continue to allow direct read access to the x/y/z fields without imposing the performance penalty that results from hiding them behind getters.

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

No branches or pull requests

2 participants