[Rev 03] RFR: 8217472: Add attenuation for PointLight

[Kevin Rushforth]

On Fri, 10 Jan 2020 00:34:18 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>>> This will need a fair bit of testing to ensure that there are no regressions either in functionality or (especially) performance, in addition to tests for the new functionality.
>> 
>> Which tests for the new functionality should I write? Up to the shader itself it's mostly just passing on variables, and the API is standard `DoubleProperty`s. I can test the dirty bits / redraw logic.
>> 
>>> On the performance aspect, the inner loop of the lighting calculation now has an additional if test for the max range and additional arithmetic calculations for the attenuation. What we will need is a test program that we can run on Mac and Windows to measure the performance of rendering in a fill-rate-limited case. Ideally, we would not pay much of a performance hit in the default case where `ca == 1, la == 0, qa == 0`, but we first need to be able to measure the drop in performance before we can say whether it is acceptable.
>> 
>> Can you point me to a similar performance test? I didn't see a way to easily measure FPS.
>> I don't know how to avoid the `if` test for the `maxRange`, I'm open to suggestions. The only thing I can think of is if `maxRange` is infinite (the default), we will swap the shader for one that doesn't make that check. However, swapping shaders also costs performance.
> 
> We have a few performance tests in apps/performance, but I don't know how up to date they are. They might give you a starting point on how to measure FPS, but really the harder part is going to be coming up with a workload -- a scene with a number of Shape3Ds with large triangles (so that they are fill-rate limited) and 1-3 lights, etc -- that you can use to measure rendering performance without measuring overhead. Typically you want a scene that is rendering continuously in the < 30 fps range, and more like 10 fps to minimize the overhead even more.
> 
> Before we figure out whether we need to double the number of shaders (which was one of the ideas that I had as well), we need to know how much of a problem it is. Is it < 2% performance drop on typical cases on a variety of machines or it is a 25% drop (or more likely somewhere in between). If the perf drop is negligible, then it isn't worth doubling the shaders. If it is significant, then we probably need to.
> 
> If we do need to double the shaders, I wouldn't do it based on the maxRange being infinite, rather I would do it based on whether attenuation is being used. That way the existing shaders would be unchanged, while the new shader would deal with both attenuation and the maxRange test.
> 
> Hopefully, there won't be enough of a perf hit to require doubling the shaders, but we need to be able to measure it.
> 
> For functional testing, in addition to the simple API tests, we want to make sure that the basic rendering is working with and without attenuation. Some sort of visual test where you verify that attenuation is / isn't happening as well as testing the cutoff. I wouldn't get too fancy with these...just a sanity test.

Looks like the `jcheck` bot removed the `rfr` label because the CSR isn't complete. An incomplete CSR should be treated the same way as an insufficient number of reviewers. I filed [SKARA-262](https://bugs.openjdk.java.net/browse/SKARA-262) to track this.

-------------

PR: https://git.openjdk.java.net/jfx/pull/43