On Thu, 14 Jan 2021 00:31:20 GMT, Nir Lisker <nlis...@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
