Re: RFR: 8217853: Cleanup in the D3D native pipeline [v2]
On Wed, 17 Aug 2022 17:38:31 GMT, Nir Lisker wrote: >> First of all, I found that the mistake was in the shortcut branch of the >> shader: it was using the light direction instead of the vector to the light >> (incident ray), so in the code I need to replace `dot(n, -lightDir)` with >> `dot(n, -l)`, like is done in the full computation branch. Then the test >> passes with the `0` input for no-attenuation. Thanks. >> >> Then I looked at the computation some more and I found something in the >> computation of the specular component. According to the theory, both in >> online sources and in the `PhongMaterial` class doc, the computation should >> be: >> `R . V` where `V` is the vector to the eye/cam and `R` is the reflection >> vector computed by `R = 2(N . L)N - L`, or using the >> [reflect](https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-reflect) >> HLSL function, `R = -reflect(N, L)`. So the shader files should look >> something like this: >> >> float3 refl = reflect(l, n); >> s = ... dot(-refl, e); // e is the vector to the eye, like V >> >> However, looking at the shader files, [already from >> legacy](https://github.com/openjdk/jfx/blob/c420248b9b459efcfbd3657170d9be0b96b5fb38/modules/javafx.graphics/src/main/native-prism-d3d/hlsl/psMath.h), >> the vectors are switched: >> >> float3 refl = reflect(e, n); >> s = ... dot(-refl, l); >> >> It looks like the specular computation was wrong from the start. >> >> I tested the visuals on the `master` branch before and after swapping the >> `l` and `e` vectors and I see no difference in the specular reflection. >> Rather odd. Will need to look into this more. >> >> The same mistakes are coded into the glsl shaders too, so I should fix the >> one you found at the very least. > > @kevinrushforth maybe you can take a look at this too. I would recommend to make only cleanup changes in this PR. So keep `isAttenuated` as 1, and file separate issue to handle the corrections you found. - PR: https://git.openjdk.org/jfx/pull/789
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v2]
On Mon, 15 Aug 2022 10:03:11 GMT, Nir Lisker wrote: >> Command to run the system test: >> `gradle -PFULL_TEST=true -PUSE_ROBOT=true systemTests:test --tests >> test.robot.test3d.PointLightIlluminationTest` >> >> I ran all the tests, only above test failed: >> Command to run all the system tests: `gradle -PFULL_TEST=true >> -PUSE_ROBOT=true systemTests:test` > > First of all, I found that the mistake was in the shortcut branch of the > shader: it was using the light direction instead of the vector to the light > (incident ray), so in the code I need to replace `dot(n, -lightDir)` with > `dot(n, -l)`, like is done in the full computation branch. Then the test > passes with the `0` input for no-attenuation. Thanks. > > Then I looked at the computation some more and I found something in the > computation of the specular component. According to the theory, both in > online sources and in the `PhongMaterial` class doc, the computation should > be: > `R . V` where `V` is the vector to the eye/cam and `R` is the reflection > vector computed by `R = 2(N . L)N - L`, or using the > [reflect](https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-reflect) > HLSL function, `R = -reflect(N, L)`. So the shader files should look > something like this: > > float3 refl = reflect(l, n); > s = ... dot(-refl, e); // e is the vector to the eye, like V > > However, looking at the shader files, [already from > legacy](https://github.com/openjdk/jfx/blob/c420248b9b459efcfbd3657170d9be0b96b5fb38/modules/javafx.graphics/src/main/native-prism-d3d/hlsl/psMath.h), > the vectors are switched: > > float3 refl = reflect(e, n); > s = ... dot(-refl, l); > > It looks like the specular computation was wrong from the start. > > I tested the visuals on the `master` branch before and after swapping the `l` > and `e` vectors and I see no difference in the specular reflection. Rather > odd. Will need to look into this more. > > The same mistakes are coded into the glsl shaders too, so I should fix the > one you found at the very least. @kevinrushforth maybe you can take a look at this too. - PR: https://git.openjdk.org/jfx/pull/789
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v2]
On Tue, 2 Aug 2022 10:57:25 GMT, Ambarish Rapte wrote: >> I'll have a look. What command did you run this test with? >> >> Can you also test the point light attenuation under >> `tests\system\src\test\java\test\javafx\scene\lighting3D\PointLightAttenuationTest.java`? >> (I think there's a bit of a mess in the tests folders.) > > Command to run the system test: > `gradle -PFULL_TEST=true -PUSE_ROBOT=true systemTests:test --tests > test.robot.test3d.PointLightIlluminationTest` > > I ran all the tests, only above test failed: > Command to run all the system tests: `gradle -PFULL_TEST=true > -PUSE_ROBOT=true systemTests:test` First of all, I found that the mistake was in the shortcut branch of the shader: it was using the light direction instead of the vector to the light (incident ray), so in the code I need to replace `dot(n, -lightDir)` with `dot(n, -l)`, like is done in the full computation branch. Then the test passes with the `0` input for no-attenuation. Thanks. Then I looked at the computation some more and I found something in the computation of the specular component. According to the theory, both in online sources and in the `PhongMaterial` class doc, the computation should be: `R . V` where `V` is the vector to the eye/cam and `R` is the reflection vector computed by `R = 2(N . L)N - L`, or using the [reflect](https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-reflect) HLSL function, `R = -reflect(N, L)`. So the shader files should look something like this: float3 refl = reflect(l, n); s = ... dot(-refl, e); // e is the vector to the eye, like V However, looking at the shader files, [already from legacy](https://github.com/openjdk/jfx/blob/c420248b9b459efcfbd3657170d9be0b96b5fb38/modules/javafx.graphics/src/main/native-prism-d3d/hlsl/psMath.h), the vectors are switched: float3 refl = reflect(e, n); s = ... dot(-refl, l); It looks like the specular computation was wrong from the start. I tested the visuals on the `master` branch before and after swapping the `l` and `e` vectors and I see no difference in the specular reflection. Rather odd. Will need to look into this more. The same mistakes are coded into the glsl shaders too, so I should fix the one you found at the very least. - PR: https://git.openjdk.org/jfx/pull/789
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v2]
On Sat, 30 Jul 2022 10:48:07 GMT, Nir Lisker wrote:
>> modules/javafx.graphics/src/main/native-prism-d3d/hlsl/psMath.h line 98:
>>
>>> 96: * specular component. The computation is done in world space.
>>> 97: */
>>> 98: void computeLight(float i, float3 n, float3 refl, float specPower,
>>> float3 toLight, float3 lightDir, in out float3 d, in out float3 s) {
>>
>> `toLight` -> `vertexToLightVec` ?
>
> The computation at this point is for pixels, and the argument type tells you
> if it's a vector (`float3`). When working with fields in the shaders'
> input/output it was convenient to be more descriptive with the names. Here
> everything is confined, so I don't think the descriptive names help much. Can
> still do a naming pass on these if you want.
Agreed, `vertexToLightVec` does not fit here. I am not able to convince myself
with naming pass. I guess the variable which have proper names are fine. May be
rename the variable that are single lettered, Leaving it to you.
-
PR: https://git.openjdk.org/jfx/pull/789
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v2]
On Tue, 2 Aug 2022 10:30:55 GMT, Nir Lisker wrote: >> 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. > > I'll have a look. What command did you run this test with? > > Can you also test the point light attenuation under > `tests\system\src\test\java\test\javafx\scene\lighting3D\PointLightAttenuationTest.java`? > (I think there's a bit of a mess in the tests folders.) Command to run the system test: `gradle -PFULL_TEST=true -PUSE_ROBOT=true systemTests:test --tests test.robot.test3d.PointLightIlluminationTest` I ran all the tests, only above test failed: Command to run all the system tests: `gradle -PFULL_TEST=true -PUSE_ROBOT=true systemTests:test` - PR: https://git.openjdk.org/jfx/pull/789
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v2]
On Tue, 2 Aug 2022 07:36:11 GMT, Ambarish Rapte wrote: >> 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. I'll have a look. What command did you run this test with? Can you also test the point light attenuation under `tests\system\src\test\java\test\javafx\scene\lighting3D\PointLightAttenuationTest.java`? (I think there's a bit of a mess in the tests folders.) - PR: https://git.openjdk.org/jfx/pull/789
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v2]
On Sat, 30 Jul 2022 11:01:59 GMT, Nir Lisker 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
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v2]
On Fri, 29 Jul 2022 21:48:00 GMT, Ambarish Rapte wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove unused comments, clean constructor > > modules/javafx.graphics/src/main/native-prism-d3d/D3DPhongShader.h line 43: > >> 41: #define VSR_LIGHT_POS 10 // 1 position = 5 * 1 = 5: c10-14 >> 42: // Registers 15-19 free >> 43: #define VSR_LIGHT_DIRS 20// 1 direction = 5 * 1 = 5: c20-24 > > Comments need to be changed as maxLights is now 3. > I would not recommend to change the macro values or registers in this PR. > Instead mention in the comment that 2 are reserved for future if we want to > increase the number of lights. These will also change when we remove the hard limit on 3 lights (we will be limited by these constant registers, but I don't think anyone needs 50 lights on one mesh...). There is already a comment saying that 3 are in use and 2 are reserved. Do you mean somewhere else? - PR: https://git.openjdk.org/jfx/pull/789
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v2]
On Fri, 17 Jun 2022 14:27:58 GMT, Ambarish Rapte wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove unused comments, clean constructor > > 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. - PR: https://git.openjdk.org/jfx/pull/789
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v2]
On Fri, 17 Jun 2022 09:04:41 GMT, Ambarish Rapte wrote:
>> Nir Lisker has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove unused comments, clean constructor
>
> modules/javafx.graphics/src/main/native-prism-d3d/D3DLight.h line 42:
>
>> 40: float position[3] = {0};
>> 41: float color[3] = {0};
>> 42: float lightOn = 0;
>
> how would `isOn` sound ?
The name is taken from `LighBase.java`. I can change it here, but we should
consider keeping the parallels.
-
PR: https://git.openjdk.org/jfx/pull/789
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v2]
On Mon, 2 May 2022 19:55:28 GMT, Nir Lisker wrote: >> Refactoring and renaming changes to some of the D3D pipeline files and a few >> changes on the Java side. These are various "leftovers" from previous issues >> that we didn't want to touch at the time in order to confine the scope of >> the changes. They will make future work easier. >> >> Since there are many small changes, I'm giving a full list here: >> >> **Java** >> >> * `NGShape3D.java` >> * Extracted methods to help with the cumbersome lighting loop: one method >> per light type + empty light (reset light) + default point light. This >> section of the code would benefit from the upcoming pattern matching on >> `switch`. >> * Normalized the direction here instead of in the native code. >> * Ambient light is now only set when it exists (and is not black). >> * `NGPointLight,java` - removed unneeded methods that were used by >> `NGShape3D` before the per-lighting methods were extracted (point light >> doesn't need spotlight-specific methods since they each have their own "add" >> method). >> * `NGSpotLight.java` - removed `@Override` annotations as a result of the >> above change. >> >> **Native C++** >> >> * Initialized the class members of `D3DLight`, `D3DMeshView` and >> `D3DPhongMaterial` in the header file instead of a more cumbersome >> initialization in the constructor (this is allowed since C++ 11). >> * `D3DLight` >> * Commented out unused methods. Were they supposed to be used at some >> point? >> * Renamed the `w` component to `lightOn` since it controls the number of >> lights for the special pixel shader variant and having it in the 4th >> position of the color array was confusing. >> * `D3DPhongShader.h` >> * Renamed some of the register constants for more clarity. >> * Moved the ambient light color constant from the vertex shader to the >> pixel shader (see the shader section below). I don't understand the >> calculation of the number of registers in the comment there: "8 ambient >> points + 2 coords = 10". There is 1 ambient light, what are the ambient >> points and coordinates? In `vsConstants` there is `gAmbinetData[10]`, but it >> is unused. >> * Reduced the number of assigned vertex register for the `VSR_LIGHTS` >> constant since it included both position and color, but color was unused >> there (it was used directly in the pixel shader), so now it's only the >> position. >> * `D3DMeshView.cc` >> * Unified the lighting loop that prepares the lights' 4-vetors that are >> passed to the shaders. >> * Removed the direction normalization as stated in the change for >> `NGShape3D.java`. >> * Reordered the shader constant assignment to be the same order as in >> `D3DPhongShader.h`. >> >> **Native shaders** >> * Renamed many of the variables to what I think are more descriptive names. >> This includes noting in which space they exist as some calculations are done >> in model space, some in world space, and we might need to do some in view >> space. For vectors, also noted if the vector is to or from (`eye` doesn't >> tell me if it's from or to the camera). >> * Commented out many unused functions. I don't know what they are for, so I >> didn't remove them. >> * `vsConstants` >> * Removed the light color from here since it's unused, only the position >> is. >> * Removed the ambient light color constant from here since it's unused, >> and added it to `psConstants` instead. >> * `vs2ps` >> * Removed the ambient color interpolation, which frees a register (no >> change in performance). >> * Simplified the structure (what is `LocalBumpOut` and why is it called >> `light` and contains `LocalBump?). >> * `Mtl1PS` and `psMath` >> * Moved the shader variant constants (`#ifndef`) to `Mtl1PS` where they >> are used for better clarity. >> * Moved the lights loop to `Mtl1PS`. The calculation itself remains in >> `psMath`. >> >> No changes in performance were measured and the behavior stayed the same. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Remove unused comments, clean constructor Providing few minor comments. I still need to do another pass. All looks good till now. 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` 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:
