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

chore: remove deprecated Chunk#getPosition(Vector3i) #4888

Merged
merged 16 commits into from
Jan 7, 2022

Conversation

pollend
Copy link
Member

@pollend pollend commented Sep 4, 2021

we've had this method depreacted for a while in Chunk, I made sure to have the Vector3i that's allocated in Chunk fixed in place so once allocated we won't be able to modify the position of a chunk. do you think we should be a bit more defensive @keturn :?

@github-actions github-actions bot added the Type: Chore Request for or implementation of maintenance changes label Sep 4, 2021
@keturn
Copy link
Member

keturn commented Sep 5, 2021

Why are so many of the calls to chunk.getPosition() wrapped in new Vector3i?

Maybe that's related to your question about how defensive the code needs to be, but I'm not quite connecting the dots.

Copy link
Member

@skaldarnar skaldarnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add to the description in

/**
* @return Position of the chunk in world, where units of distance from origin are chunks
*/
Vector3ic getPosition();
that the resulting vector can be considered stable/constant for a given chunk (at least we are using it that way).

Also, I'm wondering whether some of the new Vector3i(...) allocations are just because some other API demands to pass in a mutable Vector3i instead of a immutable Vecor3ic.
Where possible, we should fix those APIs to work with the immutable type instead. We can likely do that in smaller batches and thereby make better use of Chunk#getPosition().

@@ -33,7 +32,7 @@ public TestStorageManager(Map<Vector3ic, ChunkStore> chunkStores) {
}

public void add(Chunk chunk) {
chunkStores.put(chunk.getPosition(new Vector3i()), new TestChunkStore(chunk));
chunkStores.put(chunk.getPosition(), new TestChunkStore(chunk));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, we often recommend to create a copy when storing vectors in a map, but I guess for chunk position it is fine to assume the position to be constant and stable for a chunk, not changing ever. 🤔

@jdrueckert
Copy link
Member

@pollend care to answer @keturn's questions above? 🙃

@pollend
Copy link
Member Author

pollend commented Oct 15, 2021

@pollend care to answer @keturn's questions above? upside_down_face

umm, I'm guessing its defensive programming. I'll have a look later tonight.

@pollend
Copy link
Member Author

pollend commented Nov 16, 2021

Why are so many of the calls to chunk.getPosition() wrapped in new Vector3i?

Maybe that's related to your question about how defensive the code needs to be, but I'm not quite connecting the dots.

those shouldn't be wrapped in a new

Copy link
Member

@keturn keturn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does look much better without the extra new Vectors. 👍 Just once left I wasn't sure about.

I noticed a signature change on fireChunkTessellated, but it looks like there's only one call site for that and it did get updated, so that part seems good too.

keturn
keturn previously approved these changes Nov 28, 2021
@keturn
Copy link
Member

keturn commented Nov 28, 2021

Currently being held by CI for some unused code checks. I recommend turning on IntelliJ's “optimize imports on the fly“ setting—or, if you don't like it doing stuff on the fly, at least the “optimize imports“ option in the “before commit“ section.

@jdrueckert jdrueckert merged commit b27c8e2 into develop Jan 7, 2022
@jdrueckert jdrueckert deleted the chore/remove-deprecated-getPositision-Chunk branch January 7, 2022 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Chore Request for or implementation of maintenance changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants