Simplify elevation retrieval in each element processor #423
Conversation
|
Before After |
|
So the y argument is relative against ground level now? Interesting solution, not sure if I like it, but let's see if it's faster |
|
I think this might cause problems where you want to place a block on absolute height, not relative to the ground level, especially ground filling and bedrock layer |
My main intention with this PR is to simplify the element processors. Would be even better if it turns out to make a significant difference in speed.
Good point, since absolute block setting is used rarely - does a seperate function for that sound okay for you? I'll commit a draft of that real quick |
Yeah, looks good to me |
There was a problem hiding this comment.
Pull Request Overview
This PR centralizes elevation retrieval logic in world_editor.rs by introducing a ground field, a set_ground method, and a get_absolute_y helper to compute an absolute Y coordinate based on a relative offset. Key changes include:
- Adding ground management and elevation calculation functions in world_editor.rs.
- Removing individual ground parameter usage in element processors and replacing direct ground level calls with fixed relative Y offsets.
- Updating calls in data_processing.rs to set the ground reference and adapting function signatures across multiple element processing files.
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/world_editor.rs | Introduces ground field, set_ground(), and get_absolute_y() for elevation handling. |
| src/ground.rs | Adds Clone derive to Ground for use in set_ground. |
| Various src/element_processing/* | Removes ground parameters and replaces dynamic ground-level retrieval with fixed relative Y offsets. |
| src/data_processing.rs | Updates function calls to remove explicit ground arguments and sets the ground reference. |
Comments suppressed due to low confidence (1)
src/data_processing.rs:29
- [nitpick] Ensure that cloning the Ground instance during set_ground does not introduce performance issues; if Ground becomes a large structure, consider passing a reference or using a more efficient strategy.
editor.set_ground(&ground);
|
I'll merge yours first and then follow up with this PR! :) |
|
P.S. Don't forget that there are buildings with min_level tag, they don't need a basement, it's already implemented, just don't break it :) |
|
Thanks for the comments and then ping @Oleg4260! Sometimes I don't have the overview about all updates in here since my email inbox gets spammed with tons of mails per day hahha!
I noticed a bug with that tag as well as with building bridges since the merge of this elevation retrieval refactoring, I will take care of that!
I actually liked this approach a lot. I want to get some more impressions of it, also in comparison to the stone foundation approach. Maybe we can do a vote on which one people like more with some example images in the next few days! |
I mean, buildings in real life are likely never built like this, they always have a correct geometric shape, not with bumps in floors and roofs, it looks very wrong especially on relatively smooth surfaces with small height difference of 1-2 blocks |
Also as I said in the discussion, I think it's best to make it of the same block as wall block instead of cobblestone |
Instead of retrieving the elevation individually in each element processor, we now do it directly in world_editor.rs. Work in progress, not done yet (especially for buildings.rs and amenities.rs)