Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on improving the structure and maintainability of GLSL shaders by consolidating similar shaders and introducing dynamic shader selection. This approach reduces redundancy and simplifies the codebase, making it easier to manage and update shaders in the future. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors shader management by introducing GetFrag and GetVert utility functions to dynamically load and preprocess GLSL shaders with an IS_FLAT directive. This consolidates shader logic, eliminating several redundant *Flat.glsl files and simplifying shader usage across components like FlatBlocks, FlatMap, Sphere, and SphereBlocks. The review identified a critical compilation error in sphereVertex.glsl where the normal variable is used conditionally without being universally defined, and suggested improving type safety in GetVert.ts by using keyof typeof shaders and abstracting common logic between GetVert and GetFrag to reduce code duplication.
| void main() { | ||
| vec2 uv = giveUV(position); // We can't just pass this as a varying because the fragment will try to interpoalte between the seems which looks bad | ||
| bool inBounds = all(greaterThanEqual(uv, vec2(0.0))) && | ||
| all(lessThanEqual(uv, vec2(1.0))); | ||
| aPosition = position; | ||
| if (inBounds){ | ||
| vec3 normal = normalize(position); | ||
| int zStepSize = int(textureDepths.y) * int(textureDepths.x); | ||
| int yStepSize = int(textureDepths.x); | ||
| vec3 texCoord = vec3(uv, animateProg); | ||
| ivec3 idx = clamp(ivec3(texCoord * textureDepths), ivec3(0), ivec3(textureDepths) - 1); // Ivec3 is like running a "floor" operation on all three at once. The clamp is because the very last idx is OOR | ||
| int textureIdx = idx.z * zStepSize + idx.y * yStepSize + idx.x; | ||
| vec3 localCoord = texCoord * textureDepths; // Scale up | ||
| int yStepSize = int(textureDepths.x); | ||
| #ifdef IS_FLAT | ||
| ivec2 idx = clamp(ivec2(uv * textureDepths.xy), ivec2(0), ivec2(textureDepths.xy) - 1); | ||
| int textureIdx = idx.y * yStepSize + idx.x; | ||
| vec2 localCoord = uv * (textureDepths.xy); // Scale up | ||
| localCoord = fract(localCoord); | ||
| float dispStrength = sample1(localCoord, textureIdx); | ||
| float noNan = float(dispStrength != 1.0); | ||
| vec3 newPos = position + (normal * (dispStrength-displaceZero) * noNan * displacement); | ||
| //Pass out position for sphere frag | ||
| aPosition = position; //Pass out position for sphere frag | ||
| vec4 worldPos = modelViewMatrix * vec4( newPos, 1.0 ); | ||
| gl_Position = projectionMatrix * worldPos; | ||
| } else { | ||
| vec4 worldPos = modelViewMatrix * vec4( position, 1.0 ); | ||
| gl_Position = projectionMatrix * worldPos; | ||
| } | ||
|
|
||
|
|
||
| #else | ||
| bool inBounds = all(greaterThanEqual(uv, vec2(0.0))) && | ||
| all(lessThanEqual(uv, vec2(1.0))); | ||
| aPosition = position; | ||
| if (inBounds){ | ||
| vec3 normal = normalize(position); | ||
| int zStepSize = int(textureDepths.y) * int(textureDepths.x); | ||
| vec3 texCoord = vec3(uv, animateProg); | ||
| ivec3 idx = clamp(ivec3(texCoord * textureDepths), ivec3(0), ivec3(textureDepths) - 1); // Ivec3 is like running a "floor" operation on all three at once. The clamp is because the very last idx is OOR | ||
| int textureIdx = idx.z * zStepSize + idx.y * yStepSize + idx.x; | ||
| vec3 localCoord = texCoord * textureDepths; // Scale up | ||
| localCoord = fract(localCoord); | ||
| float dispStrength = sample1(localCoord, textureIdx); | ||
| float noNan = float(dispStrength != 1.0); | ||
| vec3 newPos = position + (normal * (dispStrength-displaceZero) * noNan * displacement); | ||
| //Pass out position for sphere frag | ||
| vec4 worldPos = modelViewMatrix * vec4( newPos, 1.0 ); | ||
| gl_Position = projectionMatrix * worldPos; | ||
| } else { | ||
| vec4 worldPos = modelViewMatrix * vec4( position, 1.0 ); | ||
| gl_Position = projectionMatrix * worldPos; | ||
| } | ||
| #endif | ||
| } No newline at end of file |
There was a problem hiding this comment.
There's a compilation error in the #ifdef IS_FLAT branch. The normal variable is used on line 64, but it's only defined within the #else block (on line 73). This will cause the shader to fail to compile when isFlat is true.
To fix this, you should define normal before the #ifdef block, so it's available in both branches.
void main() {
vec2 uv = giveUV(position); // We can't just pass this as a varying because the fragment will try to interpoalte between the seems which looks bad
vec3 normal = normalize(position);
int yStepSize = int(textureDepths.x);
#ifdef IS_FLAT
ivec2 idx = clamp(ivec2(uv * textureDepths.xy), ivec2(0), ivec2(textureDepths.xy) - 1);
int textureIdx = idx.y * yStepSize + idx.x;
vec2 localCoord = uv * (textureDepths.xy); // Scale up
localCoord = fract(localCoord);
float dispStrength = sample1(localCoord, textureIdx);
float noNan = float(dispStrength != 1.0);
vec3 newPos = position + (normal * (dispStrength-displaceZero) * noNan * displacement);
aPosition = position; //Pass out position for sphere frag
vec4 worldPos = modelViewMatrix * vec4( newPos, 1.0 );
gl_Position = projectionMatrix * worldPos;
#else
bool inBounds = all(greaterThanEqual(uv, vec2(0.0))) &&
all(lessThanEqual(uv, vec2(1.0)));
aPosition = position;
if (inBounds){
int zStepSize = int(textureDepths.y) * int(textureDepths.x);
vec3 texCoord = vec3(uv, animateProg);
ivec3 idx = clamp(ivec3(texCoord * textureDepths), ivec3(0), ivec3(textureDepths) - 1); // Ivec3 is like running a "floor" operation on all three at once. The clamp is because the very last idx is OOR
int textureIdx = idx.z * zStepSize + idx.y * yStepSize + idx.x;
vec3 localCoord = texCoord * textureDepths; // Scale up
localCoord = fract(localCoord);
float dispStrength = sample1(localCoord, textureIdx);
float noNan = float(dispStrength != 1.0);
vec3 newPos = position + (normal * (dispStrength-displaceZero) * noNan * displacement);
//Pass out position for sphere frag
vec4 worldPos = modelViewMatrix * vec4( newPos, 1.0 );
gl_Position = projectionMatrix * worldPos;
} else {
vec4 worldPos = modelViewMatrix * vec4( position, 1.0 );
gl_Position = projectionMatrix * worldPos;
}
#endif
}
src/components/textures/GetVert.ts
Outdated
| export const GetVert = (shader:string, isFlat: boolean) => { | ||
| const vert = shaders[shader as keyof typeof shaders] | ||
| const prefix = isFlat ? "#define IS_FLAT\n" : ""; | ||
| const output = prefix + vert; | ||
| return ( | ||
| output | ||
| ) | ||
| } |
There was a problem hiding this comment.
The function signature for GetVert uses shader: string and then casts it, which is not type-safe. You can improve this by using keyof typeof shaders directly in the function signature for compile-time checking of the shader name.
Additionally, the logic inside GetVert is identical to the logic in GetFrag. You could abstract this into a single generic function to reduce code duplication and improve maintainability.
| export const GetVert = (shader:string, isFlat: boolean) => { | |
| const vert = shaders[shader as keyof typeof shaders] | |
| const prefix = isFlat ? "#define IS_FLAT\n" : ""; | |
| const output = prefix + vert; | |
| return ( | |
| output | |
| ) | |
| } | |
| export const GetVert = (shader: keyof typeof shaders, isFlat: boolean) => { | |
| const vert = shaders[shader]; | |
| const prefix = isFlat ? "#define IS_FLAT\n" : ""; | |
| const output = prefix + vert; | |
| return ( | |
| output | |
| ) | |
| } |
1 participant
For shaders that needed different logic for if the texture fed into it were 2D or 3D,
I learned you can use
#ifdefto create conditional compilation. So I consolidated those shaders into one shader, and made a function to append the IS_FLAT method if I need to use the 2D variant code