On Sat, 30 Jul 2022 11:01:59 GMT, Nir Lisker <nlis...@openjdk.org> 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