Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed Matrix Multiplication Shaders #12349

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CONTRIBUTORS.md
Original file line number Diff line number Diff line change
Expand Up @@ -419,4 +419,5 @@ See [CONTRIBUTING.md](CONTRIBUTING.md) for details on how to contribute to Cesiu
- [Kirn Kim](https://github.com/squrki)
- [Emanuele Mastaglia](https://github.com/Masty88)
- [Connor Manning](https://github.com/connormanning)
- [Gajanan Gourshettiwar](https://github.com/GGajanan1)
- [胡文康](https://github.com/XiaoHu1994)
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

vec4 q = ...

q.xyz /= q.w; // normalized device coordinates
q.xyz = (czm_viewportTransformation * vec4(q.xyz, 1.0)).xyz; // window coordinates
return q;
Expand Down
2 changes: 2 additions & 0 deletions packages/engine/Source/Shaders/DepthPlaneVS.glsl
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ out vec4 positionEC;
void main()
{
positionEC = czm_modelView * position;
vec4 positionEC = czm_modelView * position;
gl_Position = czm_projection * positionEC;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

gl_Position = czm_projection * positionEC;

czm_vertexLogDepth();
Expand Down
4 changes: 2 additions & 2 deletions packages/engine/Source/Shaders/EllipsoidVS.glsl
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

gl_Position = czm_projection * positionEC;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

// With multi-frustum, when the ellipsoid primitive is positioned on the intersection of two frustums
// and close to terrain, the terrain (writes depth) in the closest frustum can overwrite part of the
// ellipsoid (does not write depth) that was rendered in the farther frustum.
Expand Down
3 changes: 2 additions & 1 deletion packages/engine/Source/Shaders/GlobeVS.glsl
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return czm_projection * positionEC;
}

#ifdef QUANTIZATION_BITS12
Expand Down
3 changes: 2 additions & 1 deletion packages/engine/Source/Shaders/SkyAtmosphereVS.glsl
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,6 @@ void main(void)
#endif

v_outerPositionWC = positionWC.xyz;
gl_Position = czm_modelViewProjection * position;
vec4 positionEC = czm_modelView * position;
gl_Position = czm_projection * positionEC;
}
6 changes: 5 additions & 1 deletion packages/engine/Specs/Scene/MultifrustumSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

vec4 positionEC = czm_modelView * position;
gl_Position = czm_projection * positionEC;
`;
vs += closestFrustum
? " gl_Position.z = clamp(gl_Position.z, gl_DepthRange.near, gl_DepthRange.far);"
: "";
Expand Down