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

New bulk node light update #4967

Closed
wants to merge 6 commits into from

Conversation

Projects
None yet
10 participants
@juhdanad
Copy link
Contributor

juhdanad commented Dec 28, 2016

Based on #4682, please do not merge this before!
Edit: #4682 is merged.

This is an early version for testing, the code style is not final yet.
Lua changes are not documented yet.
Edit: now the PR is done.

The new lighting algorithm should fix #3421 (mostly, sometimes a well-lit area is overridden, probably by the mapgen). With this v7 mapgen should be playable with the moretrees mod.

This should also fix #4853 and fix #4852.

Changes:

  • If the block above the changed area is not loaded, it gets loaded to avoid lighting bugs.
  • Light is updated as soon as write_to_map is called on a voxel manipulator
  • Therefore update_map does nothing
  • Schematics, trees and voxel manipulators use the new lighting algorithm

@juhdanad juhdanad force-pushed the juhdanad:bulklight branch from 454e3e0 to f418281 Dec 28, 2016

@juhdanad juhdanad force-pushed the juhdanad:bulklight branch from f418281 to 985199b Dec 29, 2016

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Dec 29, 2016

This work is very much appreciated, these lighting bugs are one of the biggest problems in Minetest.

@juhdanad juhdanad force-pushed the juhdanad:bulklight branch from 985199b to c8d79cf Dec 29, 2016

@juhdanad

This comment has been minimized.

Copy link
Contributor Author

juhdanad commented Dec 29, 2016

Done! The WIP label can be removed.

@paramat paramat removed the WIP label Dec 29, 2016

@HybridDog

This comment has been minimized.

Copy link
Contributor

HybridDog commented Dec 29, 2016

If many mods add map generation, all of them use the vmanip object and call calc_lighting,
then each time calc_lighting is executed the previous calculation is removed (it was in vain) and light is calculated again, am l wrong?
So instead calc_light could only set a flag indicating that the mapchunk needs light calculation after executing the mods' on_generateds.
Or does this PR remove the calc_light function from vmanip?

I assume this PR breaks my nuke mod, there when firing the rocket launcher, the explosion (nodes removal using vmanip) happens immediately, then, when the projectile reaches the target, sound is played and update_map is executed. So from the players' views the explosion happens later. But for any reason executing update_map later using minetest.after occasionally causes memory access error…

@juhdanad

This comment has been minimized.

Copy link
Contributor Author

juhdanad commented Dec 29, 2016

@HybridDog yes, it turned out there are now problems with map generating (mods that use singlenode mapgen with nolight flag)
I'll try to fix that.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Dec 29, 2016

If many mods add map generation, all of them use the vmanip object and call calc_lighting,
then each time calc_lighting is executed the previous calculation is removed (it was in vain) and light is calculated again, am l wrong?

In this case it would be the mods responsibility to delay the light calculation until after the last 'on generated' operation. Each mod can have a bool to disable lighting calculations.

@juhdanad juhdanad force-pushed the juhdanad:bulklight branch from c8d79cf to 84a0c3e Dec 29, 2016

@juhdanad

This comment has been minimized.

Copy link
Contributor Author

juhdanad commented Dec 29, 2016

Now I pushed a patch.
The behavior of lighting only changes for not mapgen voxel manipulators.
I have tested with the following mods:

  • moretrees
  • nuke
  • stability
  • pathv7
  • tnt

I hope this is now free of bugs.

@juhdanad

This comment has been minimized.

Copy link
Contributor Author

juhdanad commented Dec 29, 2016

I have also made some speed tests:
Average light calculation time when moretrees places a tree: 125 ms->26 ms
TNT exploding in sunlight (old): 12 ms
TNT exploding in shadow (old): 2,4 ms
TNT exploding (new): 2,1 ms

@Fixer-007

This comment has been minimized.

Copy link
Contributor

Fixer-007 commented Dec 29, 2016

This is very important PR for me, will try it out in some time. This patch also includes ghost light fixes?

@juhdanad

This comment has been minimized.

Copy link
Contributor Author

juhdanad commented Dec 29, 2016

@Fixer-007 the first commit in itself is #4682 and the second commit is the new stuff. If #4682 gets merged, the first commit will disappear from there.

@Fixer-007

This comment has been minimized.

Copy link
Contributor

Fixer-007 commented Dec 30, 2016

Checked for now:

  • fixes ghost light after lava casts
  • fixes incorrect light after tnt in water explosion, lighting dynamics underwater looks good to me
  • fixes shadows induced by tnt blown over to far horizon
  • fixes that notorious L system trees shadows, moretrees are working nice!
  • fixes ghost light after sofar's lightning mod strike, big nukes seem ok too

Please review 👍 👍 👍

@Fixer-007

This comment has been minimized.

Copy link
Contributor

Fixer-007 commented Jan 4, 2017

Needs rebase.

@sfan5 sfan5 added the Rebase needed label Jan 4, 2017

@juhdanad juhdanad force-pushed the juhdanad:bulklight branch from 84a0c3e to 1f15fb6 Jan 11, 2017

@juhdanad

This comment has been minimized.

Copy link
Contributor Author

juhdanad commented Jan 11, 2017

Rebased! Please review #4682 first, as merging it will remove two commits from there.

@sfan5 sfan5 removed the Rebase needed label Jan 11, 2017

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Jan 11, 2017

Unfortunately hmmmm isn't around recently, because it affects voxel manipulators and 'upate map' i feel hmmmm should look at this.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Jan 24, 2017

Bump! Unexciting but this is top priority for review as it fixes MTs most serious current bug.
It looks unlikely hmmmm will want to look at this.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Mar 2, 2017

Ok so i assume the code in map.cpp is something run when core mapgen generates.
It's worth timing this in a release build, 0.12s added to core mapgen mapchunk generation time is totally unacceptable for a rarely-triggered bug.
Usually usages of a non-mapgen-object-vm are not near generating mapchunks, i think my catacomb mod has an unreasonably fast generation sometimes, something to do with how the ABMs behave.

So essentially my opinion is let's not bother with the code in map.cpp in commit 5, please could you remove it? We could consider it in another PR later, it will only delay this PR.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Mar 3, 2017

In case you want to test i have updated my 'catacomb' mod to place full nodes under the stairs to block the light. I will retest your PR soon, with 'projects' mod too.

@HybridDog

This comment has been minimized.

Copy link
Contributor

HybridDog commented Mar 3, 2017

[Why] would your mod become slow?

Worldedit allows setting lots of nodes at once. If someone sets a 100x100x100 cube made of mese it takes ~0.05 s with my Worldedit and ~5.78 s with the original one, the current update_map takes lots of time. My version uses my function_delayer mod to distribute necessary map updates over time without causing noticeable server lag. By updating each chunk separately, l can omit unloaded chunks, which may be the majority, by using minetest.get_node_or_nil.

Are your voxel manipulators huge?

If players set lots of nodes, they're huge. If they set a few nodes, the overhead is negligible.

[Do] you update the same area often?

Yes, often players use the command frequently to change the nodes e.g. just because they noticed that cobble, then stone brick looked more fitting after all. If the map updates are delayed, no redundant ones are executed.

Is updating the map after your change still about 100 times faster than writing to map?

@juhdanad

This comment has been minimized.

Copy link
Contributor Author

juhdanad commented Mar 3, 2017

@HybridDog the current state is:

  • write_to_map automatically corrects light (but light correction is faster than before)
  • update_map does nothing

I will see how can I solve this problem, as simply disabling light updates is not enough: all of my lighting methods assume that the lighting on the map is correct (this gives some extra speed) and by placing incorrect light on the map it is not guaranteed that bugs will disappear when you update the light again.

@HybridDog

This comment has been minimized.

Copy link
Contributor

HybridDog commented Mar 3, 2017

thanks

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Mar 3, 2017

I'm doubt it's worth altering your PR for this single very specialist usage, especially if it makes your PR less effective and slower for all other uses.

@juhdanad

This comment has been minimized.

Copy link
Contributor Author

juhdanad commented Mar 5, 2017

@paramat I have tested the post-mapgen light update on a release build, it takes 0,024 seconds.
However, I removed that as you asked. I hope that this pull is finished now.

@HybridDog I will open a new PR for you, I hope that you understand that this PR is complex enough already.

@Fixer-007 Fixer-007 referenced this pull request Mar 5, 2017

Closed

Scheduling 0.4.16 #5145

@juhdanad

This comment has been minimized.

Copy link
Contributor Author

juhdanad commented Mar 5, 2017

@HybridDog there are the corrections for you, I hope they will be sufficient: juhdanad@23fdd16
With this, you can disable light updating during write_to_map() and call minetest.fix_light() later (new function, repairs the light in one map block)
I will open a PR when this is mered.

@HybridDog

This comment has been minimized.

Copy link
Contributor

HybridDog commented Mar 5, 2017

thanks, does write_to_map still work about 100 time faster without light fixing?

@juhdanad

This comment has been minimized.

Copy link
Contributor Author

juhdanad commented Mar 5, 2017

@HybridDog I do not know how fast write_to_map without light update is. But light update is about 3-4 times faster than before.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Mar 6, 2017

Thanks, will test.
24ms is quite significant, i think too much for a rare bug. Anyway that can be considered in a separate PR.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Mar 7, 2017

Retested with 'catacomb' and 'projects' mods, all seems good now 👍

@SmallJoker
Copy link
Member

SmallJoker left a comment

Code looks ok. I really hope this is tested entirely.

@sofar

This comment has been minimized.

Copy link
Member

sofar commented Mar 10, 2017

👍 tested with large voxelmanips.

Any remaining issues can be ironed out later - I think this shouldn't wait much longer.

@sofar sofar added >= Two approvals and removed One approval labels Mar 10, 2017

@MarkuBu

This comment has been minimized.

Copy link
Contributor

MarkuBu commented Mar 10, 2017

Great. Than I finally can add l-system trees?

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Mar 11, 2017

ab371cc
Hero.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.