Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v27]
On Sat, 19 Jun 2021 13:46:54 GMT, Nir Lisker 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? >> >> - [x] 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 and comparing this branch with `master` using a 1000 >> division sphere, 200 meshes, and 5000 meshes. >> Using an AMD RX 470 4GB GPU. >> >> In this branch, there is a possible CPU optimization for checking the light >> type and using precalculated values (in `D3DMeshView.cc` for d3d and >> `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing >> the spotlight factor contributions (`computeSpotlightFactor`, >> `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out >> different branching and shortcuts. >> >> ### Results >> The CPU "optimizations" made no difference, which is understandable >> considering it will not be the bottleneck. We can remove these if we want to >> simplify, though maybe if we allow a large number of lights it could make a >> difference (I doubt it). I don't have a strong preference either way. >> >> The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu). >> >> **Win 10** >> Compared with the `master` branch, this patch shows 5-10 fps drop in the >> mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several >> occasions and got different results in terms of absolute numbers, but the >> relative performance difference remained more or less the same. Out of the 3 >> `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no >> "optimizations", gives slightly better performance. >> >> **Ubuntu 18** >> The mesh 200 test always gave 60 fps because it is locked to this fps, so we >> can't measure the real GPU performance change. >> The mesh 5000 test shows 2-6 fps drop from master, with >> `computeSpotlightFactor` > `computeSpotlightFactor2` > >> `computeSpotlightFactor3` at in terms of performance (~2 fps difference >> each). >> >> **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. >> `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave >> the best performances. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Updated float constant registers I don't have any more comments or concerns. If you choose to reorder the variables or make other simple changes, I'll re-approve. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/334
Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v27]
On Tue, 22 Jun 2021 10:28:16 GMT, Ambarish Rapte wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Updated float constant registers > > modules/javafx.graphics/src/main/resources/com/sun/prism/es2/glsl/main.vert > line 50: > >> 48: >> 49: varying vec4 lightTangentSpacePositions[3]; >> 50: varying vec4 lightTangentSpaceDirections[3]; > > `lightTangentSpacePositions` is pre existing variable. I think the name > `lightTangentSpacePositions` is misguiding here. We use this variable to hold > a point to light vector in tangent space. I think the name should have been > something like, `tangentSpacePointToLightVec` or > `pointToLightVecInTangentSpace`. Again this is not mandatory for this PR, But > it did confuse me a bit while reading. > > The other variable name `lightTangentSpaceDirections` sounds correct, as it > holds the lights direction in tangent space, but if we change the existing > variable then similarly this variable name should be also be changed. Since this is preexisting, I probably wouldn't change it as part of this PR, but instead it could be done as part of the future work to support more than 3 lights. - PR: https://git.openjdk.java.net/jfx/pull/334
Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v27]
On Tue, 22 Jun 2021 11:25:42 GMT, Ambarish Rapte wrote: > Both Point and Spot light are not correctly applied to Mesh when number of > faces is increased to more than 60. > Following are steps to reproduce with Point light with existing source > without this PR. > As this is an existing issue we can address it in a follow on. @arapte can you file a JBS bug for this? - PR: https://git.openjdk.java.net/jfx/pull/334
Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v27]
On Sat, 19 Jun 2021 13:46:54 GMT, Nir Lisker 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? >> >> - [x] 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 and comparing this branch with `master` using a 1000 >> division sphere, 200 meshes, and 5000 meshes. >> Using an AMD RX 470 4GB GPU. >> >> In this branch, there is a possible CPU optimization for checking the light >> type and using precalculated values (in `D3DMeshView.cc` for d3d and >> `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing >> the spotlight factor contributions (`computeSpotlightFactor`, >> `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out >> different branching and shortcuts. >> >> ### Results >> The CPU "optimizations" made no difference, which is understandable >> considering it will not be the bottleneck. We can remove these if we want to >> simplify, though maybe if we allow a large number of lights it could make a >> difference (I doubt it). I don't have a strong preference either way. >> >> The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu). >> >> **Win 10** >> Compared with the `master` branch, this patch shows 5-10 fps drop in the >> mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several >> occasions and got different results in terms of absolute numbers, but the >> relative performance difference remained more or less the same. Out of the 3 >> `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no >> "optimizations", gives slightly better performance. >> >> **Ubuntu 18** >> The mesh 200 test always gave 60 fps because it is locked to this fps, so we >> can't measure the real GPU performance change. >> The mesh 5000 test shows 2-6 fps drop from master, with >> `computeSpotlightFactor` > `computeSpotlightFactor2` > >> `computeSpotlightFactor3` at in terms of performance (~2 fps difference >> each). >> >> **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. >> `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave >> the best performances. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Updated float constant registers Provided two suggestions. You can address if you think they are worth to address now. Over all the PR looks good to me, I observed an existing issue while testing the attenuation.AttenLightingSample. Both Point and Spot light are not correctly applied to Mesh when number of faces is increased to more than 60. Following are steps to reproduce with Point light with existing source without this PR. As this is an existing issue we can address it in a follow on. 1. Apply this patch current repo, without this PR --- a/tests/performance/3DLighting/attenuation/LightingSample.java +++ b/tests/performance/3DLighting/attenuation/LightingSampl
Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v27]
> 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? > > - [x] 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 and comparing this branch with `master` using a 1000 > division sphere, 200 meshes, and 5000 meshes. > Using an AMD RX 470 4GB GPU. > > In this branch, there is a possible CPU optimization for checking the light > type and using precalculated values (in `D3DMeshView.cc` for d3d and > `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing > the spotlight factor contributions (`computeSpotlightFactor`, > `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out different > branching and shortcuts. > > ### Results > The CPU "optimizations" made no difference, which is understandable > considering it will not be the bottleneck. We can remove these if we want to > simplify, though maybe if we allow a large number of lights it could make a > difference (I doubt it). I don't have a strong preference either way. > > The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu). > > **Win 10** > Compared with the `master` branch, this patch shows 5-10 fps drop in the mesh > 200 test and ~5 in the mesh 5000 test. I repeated the tests on several > occasions and got different results in terms of absolute numbers, but the > relative performance difference remained more or less the same. Out of the 3 > `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no > "optimizations", gives slightly better performance. > > **Ubuntu 18** > The mesh 200 test always gave 60 fps because it is locked to this fps, so we > can't measure the real GPU performance change. > The mesh 5000 test shows 2-6 fps drop from master, with > `computeSpotlightFactor` > `computeSpotlightFactor2` > > `computeSpotlightFactor3` at in terms of performance (~2 fps difference each). > > **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. > `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave > the best performances. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Updated float constant registers - Changes: - all: https://git.openjdk.java.net/jfx/pull/334/files - new: https://git.openjdk.java.net/jfx/pull/334/files/5c775341..0b5d73cd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=334&range=26 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=334&range=25-26 Stats: 51 lines in 4 files changed: 23 ins; 11 del; 17 mod Patch: https://git.openjdk.java.net/jfx/pull/334.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/334/head:pull/334 PR: https://git.openjdk.java.net/jfx/pull/334
