Fixed Matrix Multiplication Shaders #12349

GGajanan1 · 2024-12-01T09:10:02Z

Description

Fixed Matrix Multiplication in shaders
fixes #4250

Issue number and link

Testing plan

Author checklist

I have submitted a Contributor License Agreement
I have added my name to CONTRIBUTORS.md
I have updated CHANGES.md with a short summary of my change
I have added or updated unit tests to ensure consistent code coverage
I have updated the inline documentation, and included code examples where relevant
I have performed a self-review of my code

github-actions · 2024-12-01T09:10:25Z

Thank you for the pull request, @GGajanan1! Welcome to the Cesium community!

In order for us to review your PR, please complete the following steps:

Send in a Contributor License Agreement (CLA)
Add yourself to the contributors file

Review Pull Request Guidelines to make sure your PR gets accepted quickly.

GGajanan1 · 2024-12-03T07:40:25Z

Thank you for the pull request, @GGajanan1! Welcome to the Cesium community!

In order for us to review your PR, please complete the following steps:

Send in a Contributor License Agreement (CLA)

Add yourself to the contributors file

Review Pull Request Guidelines to make sure your PR gets accepted quickly.

ggetz · 2024-12-09T19:12:23Z

Thanks @GGajanan1! I can confirm we have a CLA on file for you now.

ggetz

Thanks for the changes so far @GGajanan1!

I think there are a few other places where this update is needed as well:

DepthPlaneVS.glsl
EllipsoidVS.glsl
GlobeVS.glsl
modelToWindowCoordinates.glsl

GGajanan1 · 2024-12-10T10:35:37Z

@ggetz Thanks , I have did the changes in the following PR:#12367

ggetz · 2025-01-02T16:25:28Z

Thanks @GGajanan1!

Is it possible to bring your changes from Fix/matrix multiplication shaders #12367 into this branch as well?
Please update CONTRIBUTORS.md to include your username

GGajanan1 · 2025-01-02T16:47:58Z

@ggetz I have merged the changes from #12367 in this current branch.

GGajanan1 · 2025-01-02T16:55:35Z

Also added my name in CONTRIBUTORS.md
and can you add me in the Assignees

ggetz · 2025-01-06T16:20:42Z

Perfect, thanks @GGajanan1!

We typically don't assign issues except for our own internal issue on the Cesium team.

ggetz · 2025-01-06T16:21:44Z

@jjhembd I think this is all correct, but could you please take a pass to confirm these updates are valid?

Please go ahead and merge if happy.

jjhembd

@GGajanan1 thanks a lot for this work!
I'm still testing on my end to verify the rendering result. Meanwhile, I have one comment about a typo in MultifrustumSpec below.

packages/engine/Specs/Scene/MultifrustumSpec.js

GGajanan1 · 2025-01-08T19:20:07Z

I have added the changes

jjhembd

Hi @GGajanan1, thanks for the update!
I looked through a little more closely, and noticed a few things in the code. Once these are fixed, I can run the rendering tests to confirm.

jjhembd · 2025-01-10T02:15:27Z

packages/engine/Source/Shaders/Builtin/Functions/modelToWindowCoordinates.glsl

@@ -30,7 +30,8 @@
 */
 vec4 czm_modelToWindowCoordinates(vec4 position)
 {
-    vec4 q = czm_modelViewProjection * position;                // clip coordinates
+    vec4 positionEC = czm_modelView * position;
+    q = czm_projection * positionEC;


q was not declared. The first use of it should include the type:

vec4 q = ...

jjhembd · 2025-01-10T02:16:44Z

packages/engine/Source/Shaders/DepthPlaneVS.glsl

@@ -5,6 +5,8 @@ out vec4 positionEC;
 void main()
 {
    positionEC = czm_modelView * position;
+    vec4 positionEC = czm_modelView * position;
+    gl_Position = czm_projection * positionEC;


I don't think we need this change. Now we are redeclaring positionEC, and defining gl_Position twice.

jjhembd · 2025-01-10T02:20:02Z

packages/engine/Source/Shaders/EllipsoidVS.glsl

@@ -13,8 +13,8 @@ void main()
    vec4 p = vec4(u_radii * position, 1.0);

    v_positionEC = (czm_modelView * p).xyz;     // position in eye coordinates
-    gl_Position = czm_modelViewProjection * p;  // position in clip coordinates
-
+    vec4 positionEC = czm_modelView * position;


This change is now multiplying by czm_modelView twice, which will affect performance. We could do the multiplication once, then re-use the result for the vec3 variable.

vec4 pEC = czm_modelView * p; v_positionEC = pEC.xyz;

jjhembd · 2025-01-10T02:21:05Z

packages/engine/Source/Shaders/EllipsoidVS.glsl

-    gl_Position = czm_modelViewProjection * p;  // position in clip coordinates
-
+    vec4 positionEC = czm_modelView * position;
+    gl_Position = czm_projection * positionEC;


positionEC is not the same as p, so this is changing the result.

With the change from the previous comment, this line could be:

gl_Position = czm_projection * pEC;

jjhembd · 2025-01-10T02:24:53Z

packages/engine/Source/Shaders/GlobeVS.glsl

@@ -114,7 +114,8 @@ vec4 getPositionMorphingMode(vec3 position, float height, vec2 textureCoordinate
    float yPositionFraction = get2DYPositionFraction(textureCoordinates);
    vec4 position2DWC = vec4(height, mix(u_tileRectangle.st, u_tileRectangle.pq, vec2(textureCoordinates.x, yPositionFraction)), 1.0);
    vec4 morphPosition = czm_columbusViewMorph(position2DWC, vec4(position3DWC, 1.0), czm_morphTime);
-    return czm_modelViewProjection * morphPosition;
+    vec4 positionEC = czm_modelView * position;


This is significantly changing the output. For a consistent result, this could be:

vec4 positionEC = czm_modelView * morphPosition;

However, we may want to avoid using the positionEC name for modified positions, because that name has a very specific meaning in most of the rest of the code.

jjhembd · 2025-01-10T02:27:20Z

packages/engine/Specs/Scene/MultifrustumSpec.js

@@ -237,7 +237,11 @@ describe(
          vs += "in vec4 position;";
          vs += "void main()";
          vs += "{";
-          vs += "    gl_Position = czm_modelViewProjection * position;";
+          vs = `
+              gl_Position = czm_modelViewProjection * position;


I think this is a copy-pasting error. gl_Position is now defined twice.

Fixed Matrix Multiplication Shaders

8e24055

jjspace added the PR - Needs Signed CLA label Dec 2, 2024

Merge branch 'main' into fix/matrix-multiplication-in-shaders

fbba27b

ggetz removed the PR - Needs Signed CLA label Dec 9, 2024

ggetz reviewed Dec 9, 2024

View reviewed changes

ggetz added the PR - Changes Requested label Dec 9, 2024

Gajanan and others added 2 commits December 10, 2024 15:52

Fixed all

8196a22

Merge branch 'main' into fix/matrix-multiplication-in-shaders

10dc76c

GGajanan1 closed this Dec 18, 2024

GGajanan1 deleted the fix/matrix-multiplication-in-shaders branch December 18, 2024 19:41

GGajanan1 restored the fix/matrix-multiplication-in-shaders branch December 18, 2024 19:41

GGajanan1 reopened this Dec 18, 2024

ggetz mentioned this pull request Jan 2, 2025

Fix/matrix multiplication shaders #12367

Closed

6 tasks

ggetz and others added 2 commits January 2, 2025 11:26

Merge branch 'main' into fix/matrix-multiplication-in-shaders

c077b52

FIRST

766b189

Add name to CONTRIBUTORS.md

e214560

Merge branch 'main' into fix/matrix-multiplication-in-shaders

6b2eccf

jjhembd requested changes Jan 8, 2025

View reviewed changes

packages/engine/Specs/Scene/MultifrustumSpec.js Outdated Show resolved Hide resolved

Update MultifrustumSpec.js

88e11a6

jjhembd requested changes Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed Matrix Multiplication Shaders #12349

Fixed Matrix Multiplication Shaders #12349

GGajanan1 commented Dec 1, 2024 •

edited

Loading

github-actions bot commented Dec 1, 2024

GGajanan1 commented Dec 3, 2024

ggetz commented Dec 9, 2024

ggetz left a comment

GGajanan1 commented Dec 10, 2024

ggetz commented Jan 2, 2025 •

edited

Loading

GGajanan1 commented Jan 2, 2025

GGajanan1 commented Jan 2, 2025 •

edited

Loading

ggetz commented Jan 6, 2025

ggetz commented Jan 6, 2025

jjhembd left a comment

GGajanan1 commented Jan 8, 2025

jjhembd left a comment

jjhembd Jan 10, 2025

jjhembd Jan 10, 2025

jjhembd Jan 10, 2025

jjhembd Jan 10, 2025

jjhembd Jan 10, 2025

jjhembd Jan 10, 2025

Fixed Matrix Multiplication Shaders #12349

Are you sure you want to change the base?

Fixed Matrix Multiplication Shaders #12349

Conversation

GGajanan1 commented Dec 1, 2024 • edited Loading

Description

Issue number and link

Testing plan

Author checklist

github-actions bot commented Dec 1, 2024

GGajanan1 commented Dec 3, 2024

ggetz commented Dec 9, 2024

ggetz left a comment

Choose a reason for hiding this comment

GGajanan1 commented Dec 10, 2024

ggetz commented Jan 2, 2025 • edited Loading

GGajanan1 commented Jan 2, 2025

GGajanan1 commented Jan 2, 2025 • edited Loading

ggetz commented Jan 6, 2025

ggetz commented Jan 6, 2025

jjhembd left a comment

Choose a reason for hiding this comment

GGajanan1 commented Jan 8, 2025

jjhembd left a comment

Choose a reason for hiding this comment

jjhembd Jan 10, 2025

Choose a reason for hiding this comment

jjhembd Jan 10, 2025

Choose a reason for hiding this comment

jjhembd Jan 10, 2025

Choose a reason for hiding this comment

jjhembd Jan 10, 2025

Choose a reason for hiding this comment

jjhembd Jan 10, 2025

Choose a reason for hiding this comment

jjhembd Jan 10, 2025

Choose a reason for hiding this comment

GGajanan1 commented Dec 1, 2024 •

edited

Loading

ggetz commented Jan 2, 2025 •

edited

Loading

GGajanan1 commented Jan 2, 2025 •

edited

Loading