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

[Kevin Rushforth]

On Thu, 14 Jan 2021 00:31:20 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> The updated API looks fine, with a few wording suggestions. I haven't looked at the updated shaders yet.
>> 
>> I have a few general comments and I left a few inline.
>> 
>> 1. It's too late for JavaFX 16, so you will need to update the `@since` tags to 17 and the fixVersion of both the Enhancement request and the CSR in JBS to openjfx17.
>> 
>> 2. I see you renamed the `setPointLight` method in the Prism pipelines to `setLight` and got rid of the separate `setSpotLight` method. Have you considered whether this will still make sense when adding a DirectionalLight? Maybe leaving the name as `setPointLight` is best?
>> 
>> 3. I get the following error when trying to run any 3D program on Mac, such as the "3D Box" sample in Ensemble8:
>> 
>> Shader compile log: ERROR: 0:314: '==' does not operate on 'float' and 'int'
>> ERROR: 0:314: '==' does not operate on 'float' and 'int'
>> ERROR: 0:315: Expression in 'return' statement must match return type of function (and no available implicit conversion)
>> ERROR: 0:319: '!=' does not operate on 'float' and 'int'
>> ERROR: 0:320: No matching function for call to clamp(float, int, int)
>> ERROR: 0:322: '>=' does not operate on 'float' and 'int'
>> ERROR: 0:326: '!=' does not operate on 'float' and 'int'
>> ERROR: 0:329: No matching function for call to clamp(float, int, int)
>> ERROR: 0:331: '==' does not operate on 'float' and 'int'
>> ERROR: 0:332: Expression in 'return' statement must match return type of function (and no available implicit conversion)
>> ERROR: 0:336: '>=' does not operate on 'float' and 'int'
>> ERROR: 0:342: '!=' does not operate on 'float' and 'int'
>> ERROR: 0:343: No matching function for call to clamp(float, int, int)
>> ERROR: 0:345: '>=' does not operate on 'float' and 'int'
>> 
>> java.lang.RuntimeException: Error creating fragment shader
>>         at javafx.graphics/com.sun.prism.es2.ES2Shader.createFromSource(ES2Shader.java:141)
>>         at javafx.graphics/com.sun.prism.es2.ES2PhongShader.getShader(ES2PhongShader.java:177)
>>         at javafx.graphics/com.sun.prism.es2.ES2Context.getPhongShader(ES2Context.java:142)
>>         at javafx.graphics/com.sun.prism.es2.ES2Context.renderMeshView(ES2Context.java:474)
>>         at javafx.graphics/com.sun.prism.es2.ES2MeshView.render(ES2MeshView.java:123)
>>         at javafx.graphics/com.sun.javafx.sg.prism.NGShape3D.renderMeshView(NGShape3D.java:204)
>>         ...
>>         at javafx.graphics/com.sun.javafx.tk.quantum.QuantumRenderer$PipelineRunnable.run(QuantumRenderer.java:125)
>>         at java.base/java.lang.Thread.run(Thread.java:832)
>
>> I see you renamed the setPointLight method in the Prism pipelines to setLight and got rid of the separate setSpotLight method. Have you considered whether this will still make sense when adding a DirectionalLight? Maybe leaving the name as setPointLight is best?
> 
> Directional light will be simulated as a point light at an infinite distance, making the illumination appear like it's coming from one direction, so I made this renaming deliberately.

A few comments:

I still get a runtime shader compilation error on Mac (see above)

> I forgot to mention that I edited the topmost comment to include another API discussion point about using the rotation of the node as the direction of the light. We might also need to be careful about a (0, 0, 0) direction since it's not normalizable.

Unless the transform itself is non-invertible, I don't think this can happen. What I would expect is that if we go this route -- which seems like a fine API choice to me -- the direction vector would be `(0, 0, 1)` in the local coordinate system of the light, which would then be suitably rotated based on the transform of the light (and any transforms above it).

> Directional light will be simulated as a point light at an infinite distance, making the illumination appear like it's coming from one direction, so I made this renaming deliberately.

Hmm. I wonder if the ideal choice? One of the reasons for wanting a DirectionalLight, as distinct from a very far away PointLight with no attenuation, is performance. On the other hand, since it will complicates the design of the shaders to have a special case for directional lights, it might not be worth implementing. I don't think it matters all that much, so if you want to rename it to setLight that seems OK. It's an implementation detail and could be revisited in the future.

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

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