Migrate shader-shaderlab from engine-toolkit
#2467
Conversation
WalkthroughThis pull request introduces a new shader-shaderlab package for the Galacean engine, focusing on physically-based rendering (PBR) shader management. The package includes comprehensive shader configurations, TypeScript declarations for GLSL and GS files, and a structured approach to registering and managing shader includes and shader components. The implementation provides a flexible system for defining material properties, textures, and rendering passes with support for advanced rendering techniques. Changes
Sequence DiagramsequenceDiagram
participant Index as Shader Index
participant Factory as ShaderFactory
participant Includes as Shader Includes
participant Shader as PBR Shader
Index->>Factory: registerIncludes()
Factory-->>Includes: Register fragment includes
Index->>Shader: registerShader()
Shader-->>Factory: Create and register PBR shader
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/shader-shaderlab/src/shaders/shadingPBR/index.ts (2)
1-10: Add documentation for shader module relationships.Consider adding JSDoc comments to document the relationships and dependencies between these shader modules, making it easier for developers to understand the PBR pipeline structure.
12-23: Consider organizing shader modules by pipeline stage.The shader modules could be organized based on their role in the rendering pipeline (vertex processing, fragment processing, lighting calculations, etc.) to improve maintainability.
Example organization:
export default [ + // Vertex Stage { source: AttributesPBR, includeKey: "AttributesPBR.glsl" }, { source: VaryingsPBR, includeKey: "VaryingsPBR.glsl" }, { source: VertexPBR, includeKey: "VertexPBR.glsl" }, + // Fragment Stage { source: FragmentPBR, includeKey: "FragmentPBR.glsl" }, + // Lighting { source: LightDirectPBR, includeKey: "LightDirectPBR.glsl" }, { source: LightIndirectPBR, includeKey: "LightIndirectPBR.glsl" }, { source: LightIndirectFunctions, includeKey: "LightIndirectFunctions.glsl" }, + // BRDF & Reflection { source: BRDF, includeKey: "BRDF.glsl" }, { source: ReflectionLobe, includeKey: "ReflectionLobe.glsl" }, + // Pipeline Configuration { source: ForwardPassPBR, includeKey: "ForwardPassPBR.glsl" }, ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (20)
packages/shader-shaderlab/src/shaders/BlendShape.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/Common.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/Fog.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/Light.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/Normal.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/Shadow.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/ShadowSampleTent.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/Skin.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/Transform.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/AttributesPBR.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/BRDF.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/FragmentPBR.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/LightDirectPBR.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/LightIndirectFunctions.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/LightIndirectPBR.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/ReflectionLobe.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/VaryingsPBR.glslis excluded by!**/*.glslpackages/shader-shaderlab/src/shaders/shadingPBR/VertexPBR.glslis excluded by!**/*.glslpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
packages/shader-shaderlab/README.md(1 hunks)packages/shader-shaderlab/package.json(1 hunks)packages/shader-shaderlab/src/global.d.ts(1 hunks)packages/shader-shaderlab/src/index.ts(1 hunks)packages/shader-shaderlab/src/shaders/PBR.gs(1 hunks)packages/shader-shaderlab/src/shaders/index.ts(1 hunks)packages/shader-shaderlab/src/shaders/shadingPBR/index.ts(1 hunks)packages/shader-shaderlab/tsconfig.json(1 hunks)rollup.config.js(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/shader-shaderlab/README.md
- packages/shader-shaderlab/tsconfig.json
- packages/shader-shaderlab/package.json
🔇 Additional comments (5)
packages/shader-shaderlab/src/shaders/PBR.gs (1)
31-33: Verify the type of material_OcclusionTextureCoord
The property material_OcclusionTextureCoord is declared as a Float, but texture coordinates are typically represented as Vector2 or Vector3. Please verify if a single float value is intended for this property.
Run the following script to check the usage of material_OcclusionTextureCoord in the codebase:
✅ Verification successful
Using Float for texture coordinate selection is correct
Based on the code usage, material_OcclusionTextureCoord is intentionally defined as a Float because it's used as a selector between different UV sets (UV0 vs UV1), not as actual texture coordinates. This is evident from its usage in the shader code:
if(material_OcclusionTextureCoord == 1.0){
aoUV = varyings.uv1; // or v_uv1 in other files
}The property acts as a switch where:
- 0 (default) uses the first UV set (UV0)
- 1 uses the second UV set (UV1)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the type and usage of 'material_OcclusionTextureCoord'
# Search for the property usage. Expect: Consistent use as a Float.
rg -A 5 'material_OcclusionTextureCoord'
Length of output: 3487
packages/shader-shaderlab/src/global.d.ts (1)
1-9: LGTM!
The module declarations for .glsl and .gs files correctly enable TypeScript to recognize shader imports as strings.
packages/shader-shaderlab/src/shaders/index.ts (1)
1-31: LGTM!
Shader fragments are properly imported and assembled into fragmentList. The structure and export statements are correctly set up.
packages/shader-shaderlab/src/shaders/shadingPBR/index.ts (1)
1-23: Verify shader file existence and imports.
Let's verify that all referenced shader files exist in the correct locations.
✅ Verification successful
Let me generate a new script to verify the files using a different approach.
This will list all .glsl files in the directory, giving us a clearer picture of what files actually exist there.
All shader files are present and correctly located
All 10 shader files referenced in the imports exist in the correct location under packages/shader-shaderlab/src/shaders/shadingPBR/. The file structure matches exactly with the import statements and the exported array configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all shader files exist in the correct location
# Get the directory of the current file
cd packages/shader-shaderlab/src/shaders/shadingPBR
# Check each shader file
for shader in AttributesPBR.glsl BRDF.glsl ForwardPassPBR.glsl FragmentPBR.glsl \
LightDirectPBR.glsl LightIndirectFunctions.glsl LightIndirectPBR.glsl \
ReflectionLobe.glsl VaryingsPBR.glsl VertexPBR.glsl; do
if [ ! -f "$shader" ]; then
echo "Missing shader file: $shader"
fi
done
Length of output: 2451
Script:
#!/bin/bash
# Let's use fd to find these files and check their existence
cd packages/shader-shaderlab/src/shaders/shadingPBR
fd -e glsl --type f --max-depth 1
Length of output: 277
rollup.config.js (1)
37-37: Document the purpose of .gs files and verify shader processing.
The inclusion of .gs files in the glslify plugin configuration needs documentation to explain their purpose in the shader system.
✅ Verification successful
Let me verify the content of the PBR.gs file to understand its purpose and relationship with other shader files.
Based on the analysis of the codebase, I can now provide a final response:
The .gs file extension is used for ShaderLab shader files in the PBR system
The .gs file (PBR.gs) is a ShaderLab shader file that defines the PBR (Physically Based Rendering) material properties and shader configuration. This is evidenced by:
- The file contains ShaderLab syntax for defining material properties like metallic, roughness, clear coat, etc.
- It's properly imported and used in the shader system alongside other .glsl files
- The file is part of the shader-shaderlab package's PBR implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify shader file types and their usage
# Search for .gs files and their imports
echo "Checking .gs file usage:"
fd -e gs
# Check if .gs files are properly imported in TypeScript files
echo -e "\nChecking .gs imports in TypeScript files:"
rg -t ts "import.*\.gs"
# Verify glslify is properly handling both file types
echo -e "\nChecking shader imports:"
rg "import.*\.(glsl|gs)"
Length of output: 13289
Script:
#!/bin/bash
# Check the content and context of PBR.gs file
echo "Content of PBR.gs:"
cat packages/shader-shaderlab/src/shaders/PBR.gs
# Check if there are any references to this file in documentation
echo -e "\nChecking documentation references:"
rg -g "*.md" -i "\.gs|shaderlab|pbr"
Length of output: 28897
| ui_SheenIntensity("Intensity", Range(0, 1, 0.01)) = 0; | ||
| ui_SheenColor("Color", Color ) = (0, 0, 0, 0); | ||
| material_SheenRoughness("Roughness", Range(0, 1, 0.01)) = 0; | ||
| material_SheenTexture("ColorTexture", Texture2D); | ||
| material_SheenRoughnessTexture("RoughnessTexture", Texture2D); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent naming convention in 'Sheen' properties
The properties in the "Sheen" section use the prefix ui_ instead of material_, which is inconsistent with the rest of the properties. For consistency and clarity, consider renaming ui_SheenIntensity and ui_SheenColor to material_SheenIntensity and material_SheenColor.
Apply this diff to fix the inconsistency:
- ui_SheenIntensity("Intensity", Range(0, 1, 0.01)) = 0;
- ui_SheenColor("Color", Color ) = (0, 0, 0, 0);
+ material_SheenIntensity("Intensity", Range(0, 1, 0.01)) = 0;
+ material_SheenColor("Color", Color ) = (0, 0, 0, 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ui_SheenIntensity("Intensity", Range(0, 1, 0.01)) = 0; | |
| ui_SheenColor("Color", Color ) = (0, 0, 0, 0); | |
| material_SheenRoughness("Roughness", Range(0, 1, 0.01)) = 0; | |
| material_SheenTexture("ColorTexture", Texture2D); | |
| material_SheenRoughnessTexture("RoughnessTexture", Texture2D); | |
| material_SheenIntensity("Intensity", Range(0, 1, 0.01)) = 0; | |
| material_SheenColor("Color", Color ) = (0, 0, 0, 0); | |
| material_SheenRoughness("Roughness", Range(0, 1, 0.01)) = 0; | |
| material_SheenTexture("ColorTexture", Texture2D); | |
| material_SheenRoughnessTexture("RoughnessTexture", Texture2D); |
| export function registerShader() { | ||
| if (shaderRegistered) return; | ||
|
|
||
| Shader.create(PBRSource); | ||
|
|
||
| shaderRegistered = true; | ||
| } |
There was a problem hiding this comment.
Ensure includes are registered before creating shaders
The function registerShader() calls Shader.create(PBRSource) without ensuring that registerIncludes() has been invoked. If registerShader() is called before registerIncludes(), it may lead to missing shader includes and runtime errors. To prevent this, consider invoking registerIncludes() at the beginning of registerShader().
Apply this diff to fix the issue:
export function registerShader() {
+ registerIncludes();
if (shaderRegistered) return;
Shader.create(PBRSource);
shaderRegistered = true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function registerShader() { | |
| if (shaderRegistered) return; | |
| Shader.create(PBRSource); | |
| shaderRegistered = true; | |
| } | |
| export function registerShader() { | |
| registerIncludes(); | |
| if (shaderRegistered) return; | |
| Shader.create(PBRSource); | |
| shaderRegistered = true; | |
| } |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev/1.4 #2467 +/- ##
===========================================
+ Coverage 67.91% 68.27% +0.35%
===========================================
Files 908 911 +3
Lines 94505 94585 +80
Branches 7937 7937
===========================================
+ Hits 64187 64574 +387
+ Misses 30068 29760 -308
- Partials 250 251 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
New Features
@galacean/engine-shader-shaderlabwith version1.4.0-alpha.0..glsland.gsfiles.Documentation
README.mdto indicate the package's subpackage status.Chores