Minor adaptions for PR 403 #406

louis-e · 2025-04-10T19:41:59Z

No description provided.

Oleg4260 · 2025-04-11T09:30:00Z

Hi Louis, thank you for merging my PR, but looks like we have some disagreement here, and you cancelled a lot of my changes and made some parts worse. Please read my review here, I am asking you to revert some of your changes in this commit

Oleg4260 · 2025-04-11T09:23:13Z

src/data_processing.rs

                if !editor.check_for_block(x, max_y, z, Some(&[STONE]), None) {
                    editor.set_block(groundlayer_block, x, max_y, z, None, None);
                    editor.set_block(DIRT, x, max_y - 1, z, None, None);
-                    editor.set_block(DIRT, x, max_y - 2, z, None, None);


This shouldn't be removed, it looks much better with 2 blocks of dirt under it, like in a normal minecraft world, also this depth might be needed in the future for some features that go in the ground

I removed it for generation speed reasons, but I understand your point. I've noticed some generation performance issues since the last release and wanted to counteract this by removing this layer for now until we managed to improve the performance again. I can add the line again in the meanwhile

Oleg4260 · 2025-04-11T09:26:01Z

src/element_processing/landuse.rs

I'm sorry, but what you did is really weird. You basically cancelled almost everything I did, you made trees randomly spawn again and the ice in the winter mode now looks weird, also this code is much bigger and more complicated. Could you please revert this file back to the original state as it was when I commited it? it looked much better.

I tend to generate rural areas during development and in my personal opinion it looked better with a bit of random tree generation on the fields. I'll look into this again with some areas

Regarding the ice generation in winter; I was thinking it looked a bit odd so I tried to give it some more randomness. I agree, this doesn't look good either - I didn't test this one thoroughly.

I understand, with trees the farmlands look a little more detailed, but they are meant to be smooth, no farmer would be happy to see so many trees taking space in their field and not letting tractors move. Speaking of the winter mode, neither mine symmetric option nor your random look good, but I think mine could be there temporarily, its code is more compact and probably more fast, so I'm asking you again to revert this file to my version if you agree with me

Thanks for the great discussion, I just reverted it to your change!

Oleg4260 · 2025-04-11T09:27:50Z

src/ground.rs

Do you keep this to prevent very steep differences in elevation? I don't like that scale.sqrt() because it distorts the scale even more. Also when there wasn't BASE_HEIGHT_SCALE the hills had their real size

With the addition of elevation we have the problem that some large buildings on bigger elevation differences basically "merge with the ground". I attached an image to demonstrate what I mean.

When testing your PR I've seen this happen on the first generation already, which is why I reverted it. Do you have an idea on how we can counteract this?

Ok, I've noticed that as well, I think this can be fixed by adding generation of the basement floor, so that the first floor is elevated above the ground, after fixing this issue we can increase the elevation scale to 1.0

Many buildings have a basement and their first floor is elevated about 1 meter above the ground, so this would look good

I imagine it to look like this. The first floor of the building has to start on the highest point and have a basement filling the gap under it.

Also please note the issue of OSM data and elevation data being not aligned perfectly, it is noticeable in many generations, see #399

I see, that could be an idea. It'd be good to have a POC of this so we can take a look at it visually. If I have the time I can try to do that and ping you with the results.
Also of course the current elevation 'stretching' doesn't get rid of this issue either, but I observed it way less. That's why I'd keep it as it is for now - but we should definitely reconsider it before creating the next release!

Before and after

This really looks good. But I wonder if the left door is meant to go into the basement? This building might not be meant to be located on a side of a hill, but on the top, since the issue of OSM data and elevation data is still present so it also should be fixed, as I said before, it's in #399 and also here. As I noticed, on different hemispheres of the earth the terrain is shifted into a different direction, do you have any idea how to align terrain with osm objects properly?

Thanks for your feedback, do you think the foundation should be a kind of stone block for all buildings or should it be the wall block of the building? For the OSM and elevation data shift - that's going to be a tricky task to fix haha! Not sure yet what causes this, I wanted to look into this with a bit of more time. Also thanks for the hint with the hemispheres, I wasn't aware of that yet.

I think it should be the same as the wall block for now

louis-e

Thanks a lot for your feedback @Oleg4260, let me hear your thoughts on my comments and then we can proceed with the next steps!

louis-e · 2025-04-13T19:15:37Z

src/data_processing.rs

                if !editor.check_for_block(x, max_y, z, Some(&[STONE]), None) {
                    editor.set_block(groundlayer_block, x, max_y, z, None, None);
                    editor.set_block(DIRT, x, max_y - 1, z, None, None);
-                    editor.set_block(DIRT, x, max_y - 2, z, None, None);


I removed it for generation speed reasons, but I understand your point. I've noticed some generation performance issues since the last release and wanted to counteract this by removing this layer for now until we managed to improve the performance again. I can add the line again in the meanwhile

louis-e · 2025-04-13T19:24:43Z

src/ground.rs

With the addition of elevation we have the problem that some large buildings on bigger elevation differences basically "merge with the ground". I attached an image to demonstrate what I mean.

When testing your PR I've seen this happen on the first generation already, which is why I reverted it. Do you have an idea on how we can counteract this?

louis-e · 2025-04-13T19:36:59Z

src/element_processing/landuse.rs

I tend to generate rural areas during development and in my personal opinion it looked better with a bit of random tree generation on the fields. I'll look into this again with some areas

Regarding the ice generation in winter; I was thinking it looked a bit odd so I tried to give it some more randomness. I agree, this doesn't look good either - I didn't test this one thoroughly.

Oleg4260 · 2025-04-13T22:39:27Z

src/element_processing/leisure.rs

+                            editor.set_block(OAK_LOG, x, ground_level + 1, z, None, None);
+                            editor.set_block(OAK_LOG, x + 1, ground_level + 1, z, None, None);
+                            editor.set_block(OAK_LOG, x - 1, ground_level + 1, z, None, None);
+                        }


I have another question about these benches. There are too many of them, they are scattered randomly and they don't look good. Also the program already has the code to generate benches that are mapped in the OSM, so why would you not want to remove this part of code?

Pretty much the same thing here as with the trees on farmland - I originally added the generation on leisure because it added a bit more variety. I'll revert the lines as well, maybe we can come up with a good looking bench structure soon hah!

Oleg4260 · 2025-04-16T15:29:49Z

Thanks for your reply and all the explanations, @louis-e, just tagging you in case you didn't get notified about my answers

louis-e · 2025-04-17T17:54:00Z

Hey man, thanks for the ping - I didn't notice your updates. Will look into it on the weekend, thanks for all your input! :)

Oleg4260 · 2025-04-22T12:48:35Z

@louis-e Thanks for reverting most of your changes and keeping my work, just don't forget to put back the editor.set_block(DIRT, x, max_y - 2, z, None, None); line in data_processing.rs, it used to be there before my changes and we agreed that there has to be 2 blocks layer of dirt, also take a look at #414, just did some small changes to farmland code that I forgot to do before

louis-e added 2 commits April 10, 2025 21:41

Minor adaptions for PR 403

6e11374

fmt: Fix formatting

4c16432

louis-e merged commit fa6a023 into main Apr 10, 2025
3 checks passed

louis-e deleted the pull403-adaptions branch April 10, 2025 19:50

Oleg4260 reviewed Apr 11, 2025

View reviewed changes

louis-e commented Apr 13, 2025

View reviewed changes

Oleg4260 reviewed Apr 13, 2025

View reviewed changes

louis-e mentioned this pull request Apr 19, 2025

Revert improvement #412

Merged

Oleg4260 mentioned this pull request Apr 30, 2025

Simplify elevation retrieval in each element processor #423

Merged

Uh oh!

Minor adaptions for PR 403 #406

Minor adaptions for PR 403 #406

Uh oh!

Conversation

louis-e commented Apr 10, 2025

Uh oh!

Uh oh!

Oleg4260 commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Oleg4260 Apr 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Oleg4260 Apr 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Oleg4260 Apr 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Oleg4260 Apr 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Oleg4260 Apr 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Oleg4260 Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louis-e left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Oleg4260 Apr 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Oleg4260 commented Apr 16, 2025

Uh oh!

louis-e commented Apr 17, 2025

Uh oh!

Oleg4260 commented Apr 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Oleg4260 commented Apr 11, 2025 •

edited

Loading

Oleg4260 Apr 13, 2025 •

edited

Loading

Oleg4260 Apr 13, 2025 •

edited

Loading

Oleg4260 Apr 13, 2025 •

edited

Loading

Oleg4260 Apr 13, 2025 •

edited

Loading

Oleg4260 Apr 20, 2025 •

edited

Loading

Oleg4260 Apr 25, 2025 •

edited

Loading

Oleg4260 Apr 13, 2025 •

edited

Loading