R_LightPoint Interpolation Code - Broken?

[mh]

This one's just come up with RMQ and is worth sharing.

I suspect that the commonly-used R_LightPoint (strictly speaking, RecursiveLightPoint) "enhanced to interpolate lighting" code is both broken and - potentially - dangerous.

First of all, broken because it can sample from up to 3 points outside of the surface's lightmap data. These would be the 3 points in the lightmap data for the next surface according to the order in which light.exe built the data (in some cases - if the next surface's smax * tmax was lower than the current surface's smax, it may even go beyond that and into the one after). This became obvious when we recently moved to RGB10 lightmaps and LIT files (for 4x the dynamic range of traditional lightmaps) and compiled some test maps - lighting on models as they moved (particularly noticeable with the gun) began to get really really weird, with sudden ultra-bright flashes.

Dangerous because 1 or more of those 3 points may go off the end of the loadmodel->lightdata buffer. With Hunk_Alloc you'll just read into the memory for whatever was allocated next, if you're using a different allocator you may crash.

[andrewj]

I can confirm that the interpolation code does read samples outside of the current lightmap. So that indeed could cause a memory access fault.

However when this happens, the fraction for those samples is 0 and hence they never influence the lighting result. So the logic itself is not broken.

[metlslime]

If this came from fitzquake, i probably wrote this code. There's a lot of code from my first year on that which is pretty newbie stuff now.

There's definitely a bug that sometimes causes models near walls to appear black or very dark; that may be a side effect of the flaw you describe.

[andrewj]

[szo]

What is the proposed solution? (I am not much familiar with those parts of the engine.)

[revelator]

i had a few instances where it behaved weirdly but i allways assumed the error was on my part, i newer suspected it might be broken tbh.

[Baker]

Is this the animated light styles interpolation stuff or something else?

(I'm thinking it couldn't possibly be that, that's like 3 lines of code.)

[qbism]

The R_LightPoint interpolation attempts to reduce jerky lighting changes on models, especially as a model moves across an extreme light/dark boundary. This is especially noticeable on the viewmodel. Is there any non-computationally-expensive way to detect if a sample is in-bounds?

Animated light style interpolation is different, but R_LightPoint could use a similar idea, by blending the previous lightpoint value with the new value over several frames. Cheesy, but it takes the edge off transitions and it won't break.

[metlslime]

blending over time instead of lerping over space sounds good, I like the fact that it means that stationary props get lit the same as in a non-lerping engine (of course enough engines lerp now that there is no way to have consistency across all engines.)

Of course if the blend time you choose is faster than the time it takes to cross a luxel boundary (i.e. slow moving objects), you may see periodic plateaus in the light value, which could look weird.

And for fast moving objects, if the blend time is slow, you'd see lagged lighting -- object will have passed into shadow and retain its bright light for too long.

I wonder if it would be best just to pad all lightmaps with an extra row and column of light data in memory, so that you will never go out of bounds from a lerp.

[leileilol]

I wondered where those memory leaks came from in engoo. Now I know!

I use that LH Lightpoint too

[andrewj]

[LordHavoc]

While I don't know of any bugs in my R_LightPoint interpolation, I do accept the possibility.

By design qbsp ensures that all surfaces have an extra column and row of lightmap data for each surface, this is used for lightmap interpolation in software quake when generating surface cache images from the repeating texture and the lightmap (which is scaled up with interpolation and applied).

This data is conveniently always existing, for any point location within the surface bounds there will be 2x2 pixels to read.

But you are right that the extents checks look bugged, >= extents should probably be used instead of > extents.

The bsp tree imposes many restrictions on what values will occur and how the bug would manifest, so it is hard to easily find a test case to verify a fix, but a fix is warranted.

[r00k]

[LordHavoc]

Seems I fixed this bug in DP in 2004 but did not realize that the bug dated back to the litsupport sample code.

Here's the svn entry:
r4041 | havoc | 2004-03-18 02:49:02 -0800 (Thu, 18 Mar 2004) | 2 lines
fixed 'black models' bugs in RecursiveLightPoint code

The correct change is just to make the extents checks use >= rather than > as comparison operator.