On Tue, 29 Dec 2020 15:03:15 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>> Added a SpotLight only to the D3D pipeline currently. >> >> ### API discussion points >> >> - [X] Added `SpotLight` as a subclass of `LightBase`. However, it could >> also be a subclass of `PointLight` as it's a point light with direction and >> extra factors. I saw that `scenario.effect.light.SpotLight` extends its >> respective `PointLight`, but it's not a perfect analogy. In the end, I think >> it's a questions of whether `PointLight` will be expanded in a way which >> doesn't not suit `SpotLight`, and I tend to think that the answer is no. >> >> - [X] The inner and outer angles are the "diameter angles" as shown >> [here](https://docs.microsoft.com/en-us/windows/win32/direct3d9/light-typeshttps://docs.microsoft.com/en-us/windows/win32/direct3d9/light-types). >> I, personally, find it more intuitive that these are the "radius angles", >> so half these angles, as used in the spotlight factor formula. Do you think >> I can change this or do you prefer the current definition of the angles? >> >> - [ ] The current implementation uses an ad-hoc direction property (using a >> `Point3D`). It crossed my mind that we could use the rotation transforms of >> the node to control the direction instead, just like we use the >> translation/layout of the node to get the position (there is an internal >> Affine3D transform for lights, not sure why `AmbientLight` needs it). >> Wouldn't that make more sense? When I rotate the light I would expect to see >> a change in direction. >> >> ### Implementation discussion points >> >> - [ ] I've gotten advice from a graphics engineer to treat point lights as >> spot lights with a 360 degrees coverage, which simplifies a few places. We >> can still try to optimize for a point light by looking at the light >> parameters: `falloff = 0` and `outerAngle = 180`. These possible >> optimization exist in `ES2PhongShader.java` and `D3DMeshView.cc`, and in the >> pixel/fragment shaders in the form of 3 different ways to compute the >> spotlight factor (the `computeLightN` methods). We need to check which of >> these give the best results. >> >> ### Performance >> >> Testing 3 point lights in order to compare with the [pre-patch >> performance](https://github.com/openjdk/jfx/pull/43#issuecomment-600632114): >> >> Without the `falloff != 0` branching: >> sphere 1000 subdivisions: 120 >> mesh 5000: 9.5 >> mesh 200: 111.5 >> >> With the `falloff != 0` branching: >> sphere 1000 subdivisions: 120 >> mesh 5000: 9.3 >> mesh 200: 112.5 >> >> Ubuntu 20: >> With the patch: >> Mesh 200: 60 fps >> Mesh 5000: 15 fps >> Sphere 1000: 60 fps >> >> Without the patch (master branch) >> Mesh 200: 60 fps >> Mesh 5000: 16.3 fps >> Sphere 1000: 60 fps >> >> So no major changes. I will repeat these tests to make sure there was no >> mistake. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Update to the gl pipeline 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) modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGShape3D.java line 125: > 123: NGPointLight.getDefaultQa(), > 124: NGPointLight.getDefaultMaxRange(), > 125: 0, 0, 1, 0, 180, 0); // simulating point light); Do you think it is worth using defined constants for these? modules/javafx.graphics/src/main/java/com/sun/prism/es2/ES2PhongShader.java line 229: > 227: float dirZ = light.dirZ; > 228: float length = (float) Math.sqrt(dirX * dirX + dirY * dirY + > dirZ * dirZ); > 229: shader.setConstant("lights[" + i + "].dir", dirX / length, > dirY / length, dirZ / length); In the `if` block the parameter is named `lightDir` and here it is named `dir`. I presume one of them is wrong. modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 52: > 50: * <pre>I = pow((cos(a) - cos(outer)) / (cos(inner) - cos(outer)), > falloff)</pre> > 51: * which represents a drop in illumination from the inner angle to the > outer angle. {@code falloff} determines the > 52: * behavior of the drop. The expect values are {@code 0 <= innerAngle <= > outerAngle <= 180} and {@code falloff >= 0}; That should be "expected" values, although since this is a specification, I would prefer to use "valid range". Perhaps something like: "The valid ranges for these properties are `{@code 0 <= innerAngle <= outerAngle <= 180}` and `{@code falloff >= 0}`; values outside either of these ranges can produce unexpected results". modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 129: > 127: /** > 128: * The angle of the spotlight's inner cone. Surfaces whose angle to > the light's origin is less than this angle > 129: * receive the full light's intensity. At larger angles, the light > intensity starts to drop. The expected values are Same comment as earlier about using "valid range". modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 156: > 154: /** > 155: * The angle of the spotlight's outer cone. Surfaces whose angle to > the light's origin is greater than this angle > 156: * receive no light. At smaller angles, the light intensity starts > to increase. The expected values are between "valid range" modules/javafx.graphics/src/main/native-prism-es2/GLContext.c line 2273: > 2271: /* > 2272: * Class: com_sun_prism_es2_GLContext > 2273: * Method: nSetPointLight nSetLight ------------- PR: https://git.openjdk.java.net/jfx/pull/334
