On Sun, 21 Mar 2021 17:48:10 GMT, Nir Lisker <nlis...@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

Reply via email to