On Sat, 30 Jul 2022 11:01:59 GMT, Nir Lisker <[email protected]> wrote:
>> modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGShape3D.java
>> line 201:
>>
>>> 199:
>>> 200: /**
>>> 201: * If no lights are in the scene, add a default white point light
>>> at the camera's. The light uses the default
>>
>> minor: `at the camera's` -> `at the camera's (eye) position`
>>
>> Additionally would recommend to move the first line `If no lights are in the
>> scene, add a default white point light at the camera's position.` above line
>> number 128 before calling `createDefaultLight`
>
> I thought that the code
>
> if (noLights(lights)) {
> createDefaultLight(g);
>
> speaks for itself: if there are no lights, add a default light. The details
> of what that light is are in the method doc. Can still add a comment there.
Sounds good. minor: On another thought, how about naming the method as
`setDefaultLight`.
>> modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGShape3D.java
>> line 211:
>>
>>> 209: (float) cameraPos.x, (float) cameraPos.y, (float)
>>> cameraPos.z,
>>> 210: 1.0f, 1.0f, 1.0f, 1.0f,
>>> 211: NGPointLight.getDefaultCa(),
>>> NGPointLight.getDefaultLa(), NGPointLight.getDefaultQa(), 0,
>>
>> `isAttenuated` was passed as 1 earlier. Is changing it to `0` works same ?
>
> Yes, in `PsMath.h::computeLight`, there is a check if the light requests
> attenuation. In general, this is done only for directional lights (that are
> not attenuated), but in this case we know that this point light is not
> attenuated so passing 0 skips the redundant calculation. In theory this would
> improve performance, but because this light can only exist alone I don't
> expect anything measurable.
I see 4 tests in `PointLightIlluminationTest` fail due to this change, on MacOS
and Windows.
Tests:
sphereLowerRightPixelColorShouldBeDarkRed
sphereUpperRightPixelColorShouldBeDarkRed
sphereUpperLeftPixelColorShouldBeDarkRed
sphereLowerLeftPixelColorShouldBeDarkRed
Error:
expected:rgba(139,0,0,255) but was:rgba(180,0,0,255)
The tests pass if isAttenuated is 1.
>> modules/javafx.graphics/src/main/native-prism-d3d/hlsl/vsConstants.h line 36:
>>
>>> 34: static const int isSkinned = Skin;
>>> 35:
>>> 36: static const int maxLights = 3;
>>
>> May be name as `MAX_LIGHTS` similar to `static const int MAX_BONES = 70;` on
>> line 28 in same file. ?
>> of preferably use `MAX_NUM_LIGHTS` which we already use in .h, .cpp files.
>
> The max lights limitation is going to be removed in the following change set
> (at least I intend to do it), so I don't think it's worth touching all those
> constants.
Ok, that change would touch glsl shaders too. Sounds good, we can talk about it
with that change.
-------------
PR: https://git.openjdk.org/jfx/pull/789
