On Mon, 2 May 2022 19:55:28 GMT, Nir Lisker <nlis...@openjdk.org> 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: NGPointLight.getDefaultCa(), > NGPointLight.getDefaultLa(), NGPointLight.getDefaultQa(), 0, `isAttenuated` was passed as 1 earlier. Is changing it to `0` works same ? 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 ? modules/javafx.graphics/src/main/native-prism-d3d/D3DPhongMaterial.h line 56: > 54: D3DContext *context = NULL; > 55: float diffuseColor[4] = {0}; > 56: float specularColor[4] = {1 , 1, 1, 32}; minor: remove space before comma. 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. modules/javafx.graphics/src/main/native-prism-d3d/hlsl/psConstants.h line 28: > 26: // see D3DPhongShader.h for register assignments > 27: > 28: static const int numMaxLights = 3; May be use same as used in vsConstants.h : `MAX_NUM_LIGHTS` or `MAX_LIGHTS` as recommended in review comment. 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` ? modules/javafx.graphics/src/main/native-prism-d3d/hlsl/vs2ps.h line 28: > 26: struct PsInput { > 27: > 28: static const float nLights = 3; May be use same as used in `vsConstants.h` : `MAX_NUM_LIGHTS` or `MAX_LIGHTS` as recommended in review comment. 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. ------------- Changes requested by arapte (Reviewer). PR: https://git.openjdk.org/jfx/pull/789