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

Fix and simplify world::for_each_block_optimized #169

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

verglasz
Copy link

First time contributor, so please let me know if I'm doing anything wrong.

I was looking at the source for no particular reason at all and decided to take a stab at this:

// TODO: I have no idea how to deduplicate this in a sane way

finding a bug in the process.

This PR replaces the duplicated implementation of for_each_block_optimized and
for_each_block_mut_optimized in the mchprs_world crate with a simpler implementation
that abstracts the iteration logic.

Also this adds tests for the iteration logic and for the for_each_block_optimized function
(which is how I realised that the current implementation had a bug that could lead
to some non-air block being skipped, see #168 for more details); let me know if
they're too verbose or if there should be other cases covered.

With the new implementation #168 should be fixed, since the new iteration special-cases
the bounding box boundaries and otherwise checks the block in batches corresponding
to chunk sections.

implement iterators over chunk/section indices intersecting a bounding
box delimited by two corners (block positions) and over block positions
belonging to a chunk that are within a bounding box
new tests for the new iterators and the `for_each_block_optimized`
function in the `world` crate.
Note that the function has a bug and some of the tests fail.
use common iterator abstraction to simplify the implementation of
`for_each_block_optimized` and `for_each_block_mut_optimized`,
also solving a bug they had.
@Paul1365972
Copy link
Contributor

Good catch, I initally wrote this code to only work for entire plots, but I seems like I somehow forgot that invariant when making it more generic, thank you very much!
The new code might perform worse as we call block_pos_in_chunk_section_between for each chunk, instead of handling the edge cases separately, but I would guess this is negligible.

@verglasz
Copy link
Author

verglasz commented Jan 3, 2025

Yeah, I think it’d need benchmarking to see what’s the best performant approach for that, couldn’t think of anything particularly clever to do around it but it may be worth seeing if checking whether we’re in one of the chunk sections on the edge or maybe iterating over those separately from the interior do perform better.

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.

2 participants