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

Mapgen: Run light spread algorithm iteratively #8832

Closed
wants to merge 1 commit into from

Conversation

@sfan5
Copy link
Member

commented Aug 20, 2019

no more segfaults and in the practically impossible event that all your memory runs out you get an std::bad_alloc instead

@paramat does the order in which light spreading runs matter? (it doesn't appear to from my testing and from how I understood the algorithm)
Additional lightSpread() "calls" will go to the end of the queue, which means older "calls" run before.

To do

This PR is Ready for Review.

How to test

Use lavaland subgame as described in issue 8763

@sfan5 sfan5 force-pushed the sfan5:lightspread branch from 90f56b8 to 553d455 Aug 21, 2019
@DS-Minetest

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Wouldn't it be better to fill a queue in Mapgen::spreadLight (not Mapgen::lightSpread) with the high light values and then empty this one queue? This way, things like lava lakes would be calculated much faster.

@HybridDog

This comment has been minimized.

@sfan5

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2019

If someone wants to implement a better algorithm they can open another PR, it's out of scope here.

src/mapgen/mapgen.cpp Outdated Show resolved Hide resolved
@sfan5 sfan5 force-pushed the sfan5:lightspread branch from 553d455 to 4def1f8 Aug 21, 2019
src/mapgen/mapgen.cpp Outdated Show resolved Hide resolved
src/mapgen/mapgen.cpp Show resolved Hide resolved
src/mapgen/mapgen.cpp Outdated Show resolved Hide resolved
This approach doesn't consume valuable stack space and won't
lead to hard-to-identify segfaults.
@sfan5 sfan5 force-pushed the sfan5:lightspread branch from 4def1f8 to f31320b Aug 21, 2019
@paramat

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

I wonder if #4399 and #4885 are related?

does the order in which light spreading runs matter?

In this context, i doubt it, but mostly i don't know.

Because light spread is so intensive, please can this be carefully performance-tested?

@sfan5

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2019

Because light spread is so intensive, please can this be carefully performance-tested?

The amount of operations performed is identical, the only performance difference can come from the "book-keeping" around it.
This only applied to the upper bound, a queue approach likely even reduces the amount of operations (due to early exit in the algorithm).

@paramat

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

Ok, however it still seems a good idea to quickly check performance as this is so critical, but i won't insist.

@sfan5

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

Closed in favor of the other PR.

@sfan5 sfan5 closed this Aug 23, 2019
@sfan5 sfan5 deleted the sfan5:lightspread branch Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.