Improved lighting #4655

juhdanad · 2016-10-22T17:26:19Z

This PR rewrites the procedure that is responsible for light
updating when a single node changes.

The PR

provides iterative solutions for unlighting and light spreading
introduces a new priority queue-like container for the iteration
creates per-node MapBlock caching to reduce retrieving MapBlocks from the map
calculates with map block positions and in-block relative node coordinates
skips light updating if it is not necessary since the node's new light will be the same as its old light was

This commit rewrites the procedure that is responsible for light updating. this commit -provides iterative solutions for unlighting and light spreading -introduces a new priority queue-like container for the iteration -creates per-node MapBlock caching to reduce retrieving MapBlocks from the map -calculates with map block positions and in-block relative node coordinates -skips light updating if it is not necessary since the node's new light will be the same as its old light was

juhdanad · 2016-10-22T17:47:32Z

I have made some speed tests, too (in debug build, so Minetest ran slower than its maximal speed).
I tested in singleplayer mode, so the measured times are sums of three durations:
final duration=first client light update duration+server light update duration+second client light update duration (when the server confirms the placement of the block, my version always skips lighting there).
The durations are the average of ~30 measurements.

Single cobblestone placed in sunlight: 0,217 ms -> 0,122 ms
Single cobblestone removed in sunlight: 0,305 ms -> 0,129 ms
Sunlight propagating down in a thick, dark pit (30 node deep): 69,6 ms -> 11,3 ms
Sunlight disappearing from the same pit: 89,5 ms -> 11,3 ms
Placing light source (source=14) in a dark room: 71,9 ms -> 9,4 ms
Removing that light source: 36,2 ms -> 9,4 ms
Placing light source next to same light source (no sunlight): 75,7 ms -> 5,1 ms
Removing that light source: 56 ms -> 9,7 ms
Placing light source (12) next to brighter light source (14): 55 ms -> ~0 ms
Removing the same light source: 53,7 ms -> ~0 ms
Placing light source (14) next to dimmer light source (12): 89,7 ms -> 9 ms
Removing the same light source: 69,4 ms -> 15,8 ms
Placing cobblestone diagonally next to light source (no lights change, so running time should be 0, but implementing this would decrease other running times, I think): 17,7 ms -> 3,7 ms
Removing that cobblestone: 7,8 ms -> ~0 ms

SmallJoker · 2016-10-22T20:12:07Z

Same mesecons delayer circuit in both cases. Works fine on Windows using MSVC.

Fixer-007 · 2016-10-22T20:42:21Z

Tried ingame, nothing suspicious yet.

paramat · 2016-10-22T23:23:56Z

Are bulk node updates affected? Such as lua voxelmanip used after mapgen, placement of schematics, and placement of L-system structures? Currently we have problems with lighting bugs caused by these 3 operations.

Fixer-007 · 2016-10-23T00:07:24Z

@paramat
I've managed to reproduce infamous shadows caused by TNT with this PR too. This PR will probably not affect known shadow bugs... Lighting bugs live on!
https://i.imgur.com/6jT4OMr.jpg

Someone needs to try Vanessas trees (maybe me, but tommorrow).

juhdanad · 2016-10-23T08:38:14Z

@SmallJoker, sorry if I disappointed you, in fact I was also a bit disappointed. When I have been writing this commit, I only measured the speed of light updating with a TimeTaker. I did not take two things into account:

the current light updating procedure is also fast, so making it more efficient does not introduce visible changes.
when a server-side mod alters nodes, the first client light update is not present, so the improvement of the running time is less significant.

Also, since lights do not change in every server step, the improvement is hardly noticeable.

But you can see the performance increase in extreme situations: if you create a big hole in the ground (see the image) with a piston, SEnv::step will be noticeably faster (the server does not compute meshes, so the light updating is more significant).

Sorry if I have misinformed you.

@paramat, @Fixer-007 my code only alters single node changes. This is why it does not correct the TNT lighting bug. I would like to rewrite all other lighting to this new style, but this will be in another PR.

paramat · 2016-10-23T09:00:51Z

Any work on the lighting update for bulk node changes will be very welcome, our lighting bugs are a big problem, possibly MT's most serious bug.
The lighting bug caused by L-system is detailed here #3421

SmallJoker · 2016-10-23T10:07:42Z

Would it be possible to light the bottom face of a node a bit more? This also happens in older Minetest versions and looks somehow strange IMO.

juhdanad · 2016-10-23T10:11:18Z

@SmallJoker, the lighting of bottom faces is in the mesh generation code, and is not related to light spreading, it is pure graphical. I think they are darker because their normal vector points downwards. This helps to visually separate the directions of the nodes' faces.

This commit modifies the liquid transforming procedure to light and unlight nodes instead of whole map blocks.

juhdanad · 2016-10-23T15:55:59Z

Now liquid transform uses bulk per-node updating instead of map block updating.
Liquids flowing over glass cast shadows.
I have not made any speed tests yet, but I think flowing lava will be faster now.
Fixes #4658

nerzhul · 2016-10-24T18:59:07Z

nice code quality (haven't review the feature itself, just the code quality), thanks

est31 · 2016-10-24T21:55:25Z

src/voxelalgorithms.cpp

+	 * The parameters are the same as in ChangingLight's constructor.
+	 * \param light light level of the ChangingLight
+	 */
+	inline void push(u8 light, relative_v3 &rel_pos, mapblock_v3 &block_pos,


Please make these references const

est31 · 2016-10-24T21:57:19Z

src/voxelalgorithms.cpp

+		for (u8 i = 0; i <= LIGHT_SUN; i++) {
+			const std::vector<ChangingLight> &lights = light_sources.lights[i];
+			for (std::vector<ChangingLight>::const_iterator it = lights.begin();
+					it < lights.end(); it++) {


Always use ++it for iterators because it++ creates a wasted copy.

I doubt compilers are actually that stupid, do you have a source?

I've asked that in C++ once, and they only said "maybe its optimized, maybe not".

Either way, we have commit 34b7a14 and I'd prefer if it++ wouldn't start entering the codebase again.

I agree that it makes sense to follow the general code style, the optimization explanation just seemed weird to me.

est31 · 2016-10-24T21:58:02Z

src/voxelalgorithms.cpp

+
+void update_lighting_nodes(Map *map, INodeDefManager *ndef,
+	std::vector<std::pair<v3s16, MapNode> > &oldnodes,
+	std::map<v3s16, MapBlock*> & modified_blocks)


est31 · 2016-10-24T21:58:21Z

src/voxelalgorithms.cpp

+ * \param modified_blocks output, all modified map blocks are added to this
+ */
+void spreadLight(Map *map, INodeDefManager *nodemgr, LightBank bank,
+	LightQueue & light_sources, std::map<v3s16, MapBlock*> & modified_blocks)


est31 · 2016-10-24T21:58:40Z

src/voxelalgorithms.cpp

+ */
+void unspreadLight(Map *map, INodeDefManager *nodemgr, LightBank bank,
+	UnlightQueue &from_nodes, ReLightQueue &light_sources,
+	std::map<v3s16, MapBlock*> & modified_blocks)


est31 · 2016-10-24T22:00:19Z

src/voxelalgorithms.cpp

+ * 4=Y-
+ * 5=X-
+ * 6=no direction
+ * Two directions ate opposite only if their sum is 5.


est31 · 2016-10-24T22:02:26Z

src/voxelalgorithms.cpp

@@ -19,6 +19,9 @@ with this program; if not, write to the Free Software Foundation, Inc.,

 #include "voxelalgorithms.h"
 #include "nodedef.h"
+#include "mapblock.h"
+#include "map.h"
+#include "util/timetaker.h"


Is this still needed?

est31 · 2016-10-24T22:03:47Z

src/map.cpp

-		liquid (=water node was added) to transform queue.
-	*/
+		Add neighboring liquid nodes and this node to transform queue.
+		(it's vital for the node itself to get updated last, if it was removed.)


Can you explain in the comment why it is important for the node to get updated last?

To tell the truth, I don't know. Since it was written in Map::removeNodeAndUpdate, I didn't want to alter it, because I only wanted to modify code related to lighting and not to introduce liquid flow bugs.
I don't know who wrote this comment originally, but I would be very thankful if he/she explained there why is this important.
If you want me, I will try to find it out myself, but I think it is related to liquids, not lights.

Ah I see, it appeared from the diff that you added it but if it was there before its okay.

est31 · 2016-10-25T02:03:48Z

src/voxelalgorithms.cpp

+	relative_v3 rel_position;
+	//! Position of the node's block.
+	mapblock_v3 block_position;
+	//! Reference to the node's block.


Its a pointer not a reference

est31 · 2016-10-25T02:04:21Z

src/voxelalgorithms.cpp

+struct ChangingLight {
+	//! Relative position of the node in its map block.
+	relative_v3 rel_position;
+	//! Position of the node's block.


Add to the comment whether its in node coordinates or in block coordinates

Added to mapblock_v3's description.

Fixer-007 · 2016-10-26T12:46:37Z

I played with it in game and did not find nothing critical yet.

Zeno- · 2016-10-27T04:25:33Z

I've looked over the code and for the most part I think it's great. I don't have any huge issues with it and find it easy to read (meaning it should be easy to maintain I guess).

Spent an hour doing profiling with callgrind with and without this PR and there is certainly a performance difference (improvement with the PR).

valgrind reports no memory related issues related to the PR.

Played a singleplayer game for about 30 minutes. Didn't notice anything unusual with the lighting.

So, in summary I think it's nice work 👍

est31 · 2016-10-27T04:30:44Z

👍

nerzhul · 2016-10-27T05:46:53Z

Okay for me too.

paramat · 2016-10-27T08:30:19Z

@juhdanad does this fix #2823 ?
Here's the water lighting bug i mentioned earlier #3004

Fixer-007 · 2016-10-27T11:22:26Z

#3004 is fixed by this it seems (good job 👍)
On #2823 bug I have question: #2823 (comment)
If someone interested, even with this PR lava casting ghost lighting still appears... https://i.imgur.com/V9DUnuf.png

juhdanad · 2016-11-26T21:43:57Z

I have noticed that this PR causes a compatibility break - before this PR, shadow next to sunlight had light level 13, after this PR it is changed to 14. This causes that players playing with new clients on old servers might see nodes one light level brighter than they are.

HybridDog · 2019-03-07T17:46:09Z

@juhdanad, I've got a question about the order in which the ChangingLights are traversed.
If I understand this correctly, unspread_light and spread_light traverse the positions with Breadth First Search and often almost all neighbours are added to the queue with the same light value. When there are no remaining positions for a specific light value, the positions for the next lower light value are traversed. These positions can be ordered quite randomly, which means slower memory accesses.
The ChangingLights could be sorted by block and relative position (z-y-x order) before traversing them by using a Priority Queue instead of a std::vector or by directly using Quicksort when switching to the next lower maxlight.
Would sorting them make light spreading a bit faster or is the memory access order negligible?

minetest/src/voxelalgorithms.cpp

Lines 82 to 84 in 431d8a9

    
           struct LightQueue { 
        
           	//! For each light level there is a vector. 
        
           	std::vector<ChangingLight> lights[LIGHT_SUN + 1];

est31 added the Performance label Oct 22, 2016

est31 added this to the 0.4.15 milestone Oct 22, 2016

Use node lighting for liquid spreading

569bc48

This commit modifies the liquid transforming procedure to light and unlight nodes instead of whole map blocks.

paramat added the Bugfix 🐛 PRs that fix a bug label Oct 23, 2016

est31 suggested changes Oct 25, 2016

View reviewed changes

Cleanup

941988e

est31 added the One approval ✅ ◻️ label Oct 27, 2016

Zeno- added >= Two approvals ✅ ✅ and removed One approval ✅ ◻️ labels Oct 27, 2016

nerzhul merged commit bcb06ae into minetest:master Oct 27, 2016

This was referenced Oct 27, 2016

Lighting bug with light-emitting liquids #2823

Closed

Lighting bug with water #3004

Closed

juhdanad mentioned this pull request Oct 27, 2016

Fix water flooding onto lava #4682

Closed

paramat mentioned this pull request Oct 30, 2016

Water lighting bug #4658

Closed

juhdanad mentioned this pull request Nov 26, 2016

Client crash (lighting related) #4807

Closed

juhdanad mentioned this pull request Dec 6, 2016

Broken lighting of water after TNT explosion (square-like) #4852

Closed

Fixer-007 mentioned this pull request Feb 23, 2017

Lighting glitches on borders (temporarely) #5295

Closed

juhdanad mentioned this pull request Feb 3, 2018

Remove unused light updating code #6978

Merged

Improved lighting #4655

Improved lighting #4655

Conversation

juhdanad commented Oct 22, 2016

juhdanad commented Oct 22, 2016

SmallJoker commented Oct 22, 2016 • edited

Fixer-007 commented Oct 22, 2016

paramat commented Oct 22, 2016

Fixer-007 commented Oct 23, 2016 • edited

juhdanad commented Oct 23, 2016

paramat commented Oct 23, 2016

SmallJoker commented Oct 23, 2016

juhdanad commented Oct 23, 2016

juhdanad commented Oct 23, 2016 • edited

nerzhul commented Oct 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfan5 Oct 25, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fixer-007 commented Oct 26, 2016

Zeno- commented Oct 27, 2016 • edited

est31 commented Oct 27, 2016

nerzhul commented Oct 27, 2016

paramat commented Oct 27, 2016 • edited

Fixer-007 commented Oct 27, 2016 • edited

juhdanad commented Nov 26, 2016

HybridDog commented Mar 7, 2019

SmallJoker commented Oct 22, 2016 •

edited

Fixer-007 commented Oct 23, 2016 •

edited

juhdanad commented Oct 23, 2016 •

edited

sfan5 Oct 25, 2016 •

edited

Zeno- commented Oct 27, 2016 •

edited

paramat commented Oct 27, 2016 •

edited

Fixer-007 commented Oct 27, 2016 •

edited