On Fri, 8 May 2020 12:36:41 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8218264 > > Nir Lisker has updated the pull request with a new target base due to a merge > or a rebase. The pull request now > contains 11 commits: > - Merge branch 'master' into 8217472_Add_attenuation_for_PointLight > - Attenuation and range changed internally to floats from doubles > - Fixed shader compilation errors for 2 and 3 lights in es2 > - Addressing review comments > - Fixed whitespaces > - Correction for indexes > - Docs and year update > - Merge remote-tracking branch > 'nlisker/8217472_Add_attenuation_for_PointLight' into > 8217472_Add_attenuation_for_PointLight > - GL pipeline > - Separate range from attenuation > - ... and 1 more: > https://git.openjdk.java.net/jfx/compare/4ec163df...2e1223ed Added some minor comments. The changes suggested in the shader cause no difference in FPS, but only reduces number of instructions in compiled fragment shader. I verified it in the compiled D3D shader, that the change reduces one multiplication instruction. The glCompileShader() API performs some optimizations on the shader. But the optimizations would be implementation specific. The difference of fps in different machines may be caused by difference of glCompileShader() implementation. On the shader side there are few ways of doing [glsl optimizations](https://www.khronos.org/opengl/wiki/GLSL_Optimizations). This may be worth for a quick try now or later as follow on. modules/javafx.graphics/src/main/java/javafx/scene/PointLight.java line 174: > 173: > 174: private static final double DEFAULT_LINEAR_CONSTANT = > NGPointLight.getDefaultLa(); > 175: Small correction needed, `DEFAULT_LINEAR_CONSTANT` -> `DEFAULT_LINEAR_ATTENUATION` similarly, `DEFAULT_QUADRATIC_CONSTANT` -> `DEFAULT_QUADRATIC_ATTENUATION` modules/javafx.graphics/src/main/resources/com/sun/prism/es2/glsl/main3Lights.frag line 92: > 91: d += clamp(dot(n,l), 0.0, 1.0) * (lights[0].color).rgb * att; > 92: s += pow(clamp(dot(-refl, l), 0.0, 1.0), power) * > lights[0].color.rgb * att; > 93: } These three lines can be changed as: float attenuatedColor = (lights[0].color).rgb / (lights[0].attn.x + lights[0].attn.y * dist + lights[0].attn.z * dist * dist); d += clamp(dot(n,l), 0.0, 1.0) * attenuatedColor; s += pow(clamp(dot(-refl, l), 0.0, 1.0), power) * attenuatedColor; Similar change in the 1 light and 2 lights shader. modules/javafx.graphics/src/main/native-prism-d3d/hlsl/psMath.h line 76: > 75: d += saturate(dot(n, l)) * gLightColor[i].xyz * attn; > 76: s += pow(saturate(dot(-refl, l)), power) * gLightColor[i].xyz > * attn; > 77: } The temporary variables ca, la, qa will be removed by shader compiler optmization, so has no extra cost involved here. But we can reduce one multiplication instruction by changing the rest of the code as, float attenuatedColor = gLightColor[i].xyz / (ca + la * dist + qa * dist * dist); float3 l = normalize(L[i].xyz); d += saturate(dot(n, l)) * attenuatedColor; s += pow(saturate(dot(-refl, l)), power) * attenuatedColor; ------------- Changes requested by arapte (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/43
