RFR: 8234920: Add SpotLight to the selection of 3D light types [v9]

[Nir Lisker]

On Sun, 21 Mar 2021 17:48:10 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> Finally getting back to this.
>> 
>> There is an outstanding API question regarding the direction vector: Should the transforms of the SpotLight node to scene coordinates affect the direction vector (e.g., such that a rotation would rotate the direction the spotlight is pointing)? If so, do we even need the direction vector or could we define the direction as `(0,0,1)` in the local coordinates of the SpotLight, and tell applications to apply the appropriate rotation to define the direction.
>> 
>> I think for consistency, the answer is "Yes" to the first question (that's how the position works and it would be odd for the transform to affect the position and not the direction). I'm less certain about the answer to the second question. Without utility functions like `lookAt` to help compute the appropriate transform, it seems important to be able to specify the direction as an explicit parameter. And even if we had utility functions, an app might want the control (although you might argue it would be unneeded). My instinct is to keep it as defined. If we go with this, the only change that is needed is a note that the transforms applied to the SpotLight node affect it's direction as well as its position.
>> 
>> Implementation:
>> 
>> Can you add a system test similar to what you did for attenuation to verify the basic functionality?
>> 
>> I tested this a bit on Windows using D3D and it behaves as I would expect, although I didn't do exhaustive testing.
>> 
>> On Mac I no longer get a GLSL shader error at runtime, but spotlights aren't working correctly, either. The falloff happens too fast (at 1 you can't see the light at all). For the inner/outer angles, something looks backwards. See the attached image
>> <img width="1212" alt="SpotLight-macOS" src="https://user-images.githubusercontent.com/34689748/111841271-f293d000-88ba-11eb-915c-38e2c69f5e70.png">
>> 
>> I left a few inline comments, although I haven't reviewed the shader files in detail yet.
>
>> There is an outstanding API question regarding the direction vector: Should the transforms of the SpotLight node to scene coordinates affect the direction vector (e.g., such that a rotation would rotate the direction the spotlight is pointing)? If so, do we even need the direction vector or could we define the direction as `(0,0,1)` in the local coordinates of the SpotLight, and tell applications to apply the appropriate rotation to define the direction.
>> 
>> I think for consistency, the answer is "Yes" to the first question (that's how the position works and it would be odd for the transform to affect the position and not the direction). I'm less certain about the answer to the second question. Without utility functions like `lookAt` to help compute the appropriate transform, it seems important to be able to specify the direction as an explicit parameter. And even if we had utility functions, an app might want the control (although you might argue it would be unneeded). My instinct is to keep it as defined. If we go with this, the only change that is needed is a note that the transforms applied to the SpotLight node affect it's direction as well as its position.
> 
> I would like to allow a developer to achieve a functionality like is shown in https://www.youtube.com/watch?v=CFgwZX5dkcM at 9:50. The rotations are intuitive there. If we allow both rotation transforms and a direction, wouldn't that cause the direction to be unintuitive? Or do you mean that the direction is always the look-at regardless of rotation transforms and if it's `null` then the rotations take over?
> 
>> On Mac I no longer get a GLSL shader error at runtime, but spotlights aren't working correctly, either.
> 
> I don't see this on Linux and I don't have a Mac. Can you try on Linux and see what you get? If Linux works, I'm afraid I would not be able to debug the Mac.

I modified the existing attenuation test to use a `SpotLight`, which is more general, and separated the tests for the various contributions of the light. I renamed the class, so the previous version is removed, but its tests are the same in the new class.

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

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