On Sat, 14 Aug 2021 13:00:52 GMT, Nir Lisker <[email protected]> wrote:
>> Adds a directional light as a subclass of `LightBase`. I think that this is
>> the correct hierarchy for it.
>>
>> I tried to simulate a directional light by putting a point light far away,
>> but I got artifacts when the distance was large. Instead, I added an on/off
>> attenuation flag as the 4th component of the attenuation 4-vector. When it
>> is 0, a simpler computation is used in the pixel/fragment shader that
>> calculates the illumination based on the light direction only (making the
>> position variables meaningless). When it is 1, the point/spot light
>> computation is used. It's possible that the vertex shader can also be
>> simplified in this case since it does not need to transform the position
>> vectors, but I left this optimization avenue for another time.
>>
>> I noticed a drop of ~1 fps in the stress test of 5000 meshes.
>>
>> I added a system test that verifies the correct color result from a few
>> directions. I also updated the lighting sample application to include 3
>> directional lights and tested them on all the models visually. The lights
>> seem to behave the way I would expect.
>
> Nir Lisker has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update copyright year
I tested this on all three platforms and it looks good. There are a couple of
copy/paste typos where `SpotLight` should be `DirectionalLight`. I also left a
question / suggestion for dealing with the boolean. You can either change it,
or leave it and add a comment.
modules/javafx.graphics/src/main/java/com/sun/javafx/scene/DirectionalLightHelper.java
line 2:
> 1: /*
> 2: * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
2021
modules/javafx.graphics/src/main/java/com/sun/javafx/scene/DirectionalLightHelper.java
line 35:
> 33:
> 34: /**
> 35: * Used to access internal methods of SpotLight.
Typo: `SpotLight` --> `DirectionalLight`.
modules/javafx.graphics/src/main/java/com/sun/prism/es2/ES2Light.java line 68:
> 66:
> 67: boolean isDirectionalLight() {
> 68: return isAttenuated < 0.5;
Minor: you can just test for 0 or not 0 since you only ever set them as
constants. It's up to you...I don't object to rounding, which is effectively
what you've done.
modules/javafx.graphics/src/main/java/javafx/scene/DirectionalLight.java line
43:
> 41: * A light that illuminates an object from a specific direction.
> 42: * The direction is defined by the {@link #directionProperty() direction}
> vector property of the light. The direction
> 43: * can be rotated by setting a rotation transform on the {@code
> SpotLight}. For example, if the direction vector is
Typo: `SpotLight` --> `DirectionalLight`
modules/javafx.graphics/src/main/native-prism-d3d/D3DLight.cc line 54:
> 52:
> 53: bool D3DLight::isDirectionalLight() {
> 54: return attenuation[3] < 0.5;
Same comment as for `ES2Light`.
modules/javafx.graphics/src/main/native-prism-d3d/hlsl/psMath.h line 106:
> 104:
> 105: void computeLight(float i, float3 n, float3 refl, float specPower,
> float3 L, float3 lightDir, in out float3 d, in out float3 s) {
> 106: if (gLightAttenuation[i].w < 0.5) {
Is there any performance difference between `< 0.5` and `!= 0` ? I suspect not,
but in any case, you might consider the latter (as I also mentioned in the java
files). The latter is more clear, so if you choose to stick with what you have,
I'd like to see a comment added.
modules/javafx.graphics/src/main/resources/com/sun/prism/es2/glsl/main1Light.frag
line 122:
> 120: Light light = lights[i];
> 121: vec3 lightDir = lightTangentSpaceDirections[i].xyz;
> 122: if (light.attn.w < 0.5) {
Same comment as D3D shaders.
tests/performance/3DLighting/attenuation/Environment.java line 85:
> 83: pointLight2.setTranslateX(-LIGHT_X_DIST);
> 84: spotLight2.setTranslateX(-LIGHT_X_DIST);
> 85:
I recommend setting the direction of `directionalLight1` and
`directionalLight2` as follows:
directionalLight1.setDirection(new Point3D(-LIGHT_X_DIST, 0, LIGHT_Z_DIST));
directionalLight2.setDirection(new Point3D(LIGHT_X_DIST, 0, LIGHT_Z_DIST));
tests/system/src/test/java/test/javafx/scene/lighting3D/DirectionalLightTest.java
line 71:
> 69:
> 70: @Test
> 71: public void testSpotlightAttenuation() {
Should be `testDirectionalLight`.
-------------
PR: https://git.openjdk.java.net/jfx/pull/548
