On Wed, 17 Aug 2022 17:38:31 GMT, Nir Lisker <nlis...@openjdk.org> 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
