Prevent seeing light through walls

[jbytheway]

Summary

SUMMARY: Interface "Prevent seeing light through walls"

Purpose of change

There's a long-standing issue where walls that are lit from one side appear bright from the other side. This is strange and unintuitive.

I have a proof-of-concept solution, and wanted to share it for feedback before spending the time to polish it.

Describe the solution

First some pictures.
Before:

After:

Works with vehicle lights too:

The algorithm is fairly simple. Rather than storing a single illumination value in the light map, when a tile is opaque (like a wall) we store four values: one for each quadrant of 2D space. Each value represents the illumination from that quadrant. When computing apparent visibility, we check which other tiles adjacent to the tile in question we can see, and count only those light values corresponding to the quadrants where we can see adjacent tiles.

This works pretty well in practice, the only small annoyance I've observed is that you get a 'looking around corners' kind of effect in some cases. This can be seen in the second image above: the wall to the left of the northern door appears bright because we can also see the tile to the NE of it, and it is being lit from that direction. But that's never giving away any information the player couldn't have deduced, so I think it's fine, and it doesn't look too weird.

Fixes #13356.

Performance

No formal measurements yet, but it seems fine in "normal" situations.
There are two pieces of code to consider:

• The light casting code. When optimized it should be only one extra branch+array index per tile here, so that ought to be fine. Even in debug build I'd expect negligible overhead.
• Calculating lighting levels. There will be a largish overhead when calculating lighting level for opaque tiles, but the number of opaque tiles it's possible to see at once is much less than the number of non-opaque ones, so I think this should be fine too.

The current memory layout is rather cache-unfriendly. If performance testing dictates, it may be wise to swap the array dimensions around.

Still to do

• Update the tests (including adding tests verifying that walls are lit correctly in this way).
• Update the 3D FOV code.
• Test performance when many light sources exist.

Additional context

I'm aware there has been discussion of a new lighting algorithm (#23996). I think this change is entirely compatible with that new algorithm.

[kevingranade]

This definitely looks like the right way to do it, I've been concerned that it was going to unduly impact performance, but I hadn't thought of only applying the 4 quadrant update to opaque tiles.

I agree this seems to mesh with all the optimizations I have planned for shadowcasting (both the dynamic lighting overhaul and refactoring the shadowcasting algorithm to be breadth-first).

[AMurkin]

The issue is #13356.

[jbytheway]

The tests comparing various algorithms (shadowcasting_runoff, etc.) don't pass on master. Should they? I tried rolling back to when they were first added in 4353ec9 to see if they worked then, but that version requires libtap++, which I can't easily install.

[jbytheway]

OK, I found a version that does pass (f48f788); bisecting.

[jbytheway]

OK, so it broke in e858c00 slightly. It looks like it might have got worse at some point since then, but it's kind of subjective, so I'm not sure. In any case, I assume I should not be worrying about that test. (Should have paid more attention to the comment written above it, I suppose...)

[BevapDin]

inline is redundant here. Functions defined inside the class definition are automatically inline.

[jbytheway]

I thought that was only true of member functions, but it seems you're right; it applies to friend definitions also. Thanks.

[kevingranade]

Sorry about the confusion, most of those tests are execution aids and migration validation rather then ongoing regression tests.

[jbytheway]

There's another oddity I'd like to understand. If we look at the following chunk of code from the visibility algorithm (this is from master, not my branch):

Cataclysm-DDA/src/map.cpp

Lines 5458 to 5471 in f99a963

	// Then we just search for the light level in descending order.
	if( apparent_light > LIGHT_SOURCE_BRIGHT || map_cache.sm[p.x][p.y] > 0.0 ) {
	return LL_BRIGHT;
	}
	if( apparent_light > LIGHT_AMBIENT_LIT ) {
	return LL_LIT;
	}
	if( apparent_light > cache.vision_threshold ) {
	return LL_LOW;
	} else {
	return LL_BLANK;
	}
	// Is this ever supposed to happen?
	return LL_DARK;

we see that it's choosing from a sequence of decreasing light levels.

There's one strangeness which is already called out in a comment in this code. It can never return LL_DARK.

However, there is more. Look at the definitions of the constants:

Cataclysm-DDA/src/lightmap.h

Line 6 in f99a963

#define LIGHT_SOURCE_BRIGHT 10

Cataclysm-DDA/src/lightmap.h

Line 15 in f99a963

#define LIGHT_AMBIENT_LIT 10.0

LIGHT_SOURCE_BRIGHT and LIGHT_AMBIENT_LIT are the same, so this function can also never return LL_LIT. Is that intentional?

[jbytheway]

Tests written. Some bugs found and fixed. I believe it's functionally complete now. Still want to do some performance testing.

[jbytheway]

(Rebased to avoid build error about mismatched Makefile)

I think this is now ready. Of course, testing by others would be welcome.

Performance testing shows very little difference between the light casting algorithms' performance (up to ~1%):

castLight on four_quadrants (denominator 10) executed 1000000 times in 655537 microseconds.
castLight on floats (denominator 10) executed 1000000 times in 652451 microseconds.

castLight on four_quadrants (denominator 100) executed 1000000 times in 652528 microseconds.
castLight on floats (denominator 100) executed 1000000 times in 651208 microseconds.

castLight on four_quadrants (denominator 10) executed 1000000 times in 857010 microseconds.
castLight on floats (denominator 10) executed 1000000 times in 848121 microseconds.

castLight on four_quadrants (denominator 100) executed 1000000 times in 651661 microseconds.
castLight on floats (denominator 100) executed 1000000 times in 655883 microseconds.

castLight on four_quadrants (denominator 10) executed 1000000 times in 651561 microseconds.
castLight on floats (denominator 10) executed 1000000 times in 649798 microseconds.

castLight on four_quadrants (denominator 100) executed 1000000 times in 656125 microseconds.
castLight on floats (denominator 100) executed 1000000 times in 651559 microseconds.

castLight on four_quadrants (denominator 10) executed 1000000 times in 855901 microseconds.
castLight on floats (denominator 10) executed 1000000 times in 846490 microseconds.

castLight on four_quadrants (denominator 100) executed 1000000 times in 651638 microseconds.
castLight on floats (denominator 100) executed 1000000 times in 652841 microseconds.

castLight on four_quadrants (denominator 10) executed 1000000 times in 820022 microseconds.
castLight on floats (denominator 10) executed 1000000 times in 816416 microseconds.

castLight on four_quadrants (denominator 100) executed 1000000 times in 651293 microseconds.
castLight on floats (denominator 100) executed 1000000 times in 652072 microseconds.

As anecdotal evidence, I also tried burning down some buildings and the performance seems fine.

I used perf while doing so; it shows that generate_lightmap is a large part of the cost in such a situation. ~35% of samples are in map::build_map_cache and 30% in map::generate_lightmap. I think that's pretty much the same as on master (it's hard to construct a repeatable experiment for comparison).

My other reason for running perf was to verify that map::update_visibility_cache was not suddenly very expensive. Indeed it is not, clocking in at ~2% of samples.

[jbytheway]

Got a slightly concerning crash on one of the mingw builds on Travis (looks like a null pointer dereference). Not sure how to debug that. Is it possible to get the actual binary that was built there so I can interpret the backtrace?

[kevingranade]

The container is destroyed, you could possibly stick a command in the travis.yml file to push it somewhere, other than that youd have to try and recreate the docker image travis uses.

[jbytheway]

OK, after some investigation it seems my test refactoring had exposed some flakiness in other tests. All the failures I've observed should be fixed now. The current test failures are the same vehicle power ones that are failing everywhere.

[jbytheway]

I've rebased and tidied up these changes after pulling out #26701 and #26728. I think it's as clean as it's going to get and good for review.