Open shader core API by ShaderLab #274
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range, codebase verification and nitpick comments (8)
packages/shaderlab/src/shaders/shadingPBR/VaryingsPBR.glsl (1)
1-36: Ensure documentation and testing of conditional compilation blocks.The search did not find any dedicated documentation or tests for the conditional compilation blocks in the shader files. Proper documentation and testing are crucial to ensure these blocks function correctly across different platforms and rendering scenarios. Please ensure that these blocks are well-documented and covered by tests.
- Files affected:
packages/shaderlab/src/shaders/shadingPBR/VaryingsPBR.glslpackages/shaderlab/src/shaders/Vertex.glslpackages/shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glslpackages/shaderlab/src/shaders/shadingPBR/AttributesPBR.glslpackages/shaderlab/src/shaders/shadingPBR/MaterialInputPBR.glslpackages/shaderlab/src/shaders/Normal.glslpackages/shaderlab/src/shaders/shadingPBR/LightIndirectPBR.glslpackages/shaderlab/src/shaders/Fog.glslpackages/shaderlab/src/shaders/BlendShape.glslpackages/geometry-sketch/src/material/TBNMaterial.tspackages/geometry-sketch/src/material/GeometryShader.tspackages/custom-material/src/bake-pbr/fragment.tsAnalysis chain
Ensure documentation and testing of conditional compilation blocks.
This file introduces several conditional compilation blocks which can significantly affect the shader's behavior on different hardware. Please ensure that these blocks are well-documented and covered by tests, especially since they can lead to different rendering outcomes on various platforms.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for documentation and tests related to these conditional blocks. # Test: Search for documentation and tests. Expect: Documentation and test files related to these conditions. rg --type md 'RENDERER_HAS_UV1|RENDERER_ENABLE_VERTEXCOLOR|SCENE_FOG_MODE|RENDERER_HAS_NORMAL'Length of output: 96
Script:
#!/bin/bash # Description: Check for documentation and tests related to these conditional blocks in a broader range of files. # Search for the conditional compilation terms in markdown files for documentation. rg --type md 'RENDERER_HAS_UV1|RENDERER_ENABLE_VERTEXCOLOR|SCENE_FOG_MODE|RENDERER_HAS_NORMAL' # Search for the conditional compilation terms in GLSL files for shader tests. rg --type glsl 'RENDERER_HAS_UV1|RENDERER_ENABLE_VERTEXCOLOR|SCENE_FOG_MODE|RENDERER_HAS_NORMAL' # Search for the conditional compilation terms in JavaScript files for test scripts. rg --type js 'RENDERER_HAS_UV1|RENDERER_ENABLE_VERTEXCOLOR|SCENE_FOG_MODE|RENDERER_HAS_NORMAL'Length of output: 321
Script:
#!/bin/bash # Description: Check for documentation and tests related to these conditional blocks across all files. # Search for the conditional compilation terms in all files. rg 'RENDERER_HAS_UV1|RENDERER_ENABLE_VERTEXCOLOR|SCENE_FOG_MODE|RENDERER_HAS_NORMAL'Length of output: 2939
packages/dynamic-bone/package.json (1)
3-3: Versioning Change Documentation MissingThe version number has been updated to an experimental version (
0.0.0-experimental-shaderlab-pbr.5). However, there is no documentation found that reflects this change. It is crucial to update the documentation to inform users about the experimental nature of this release.
- Ensure the version change is documented in
README.md,CHANGELOG.md, or other relevant documentation files.- Clearly communicate the experimental nature of this release to users.
Analysis chain
Versioning Change to Experimental Noted
The version number has been updated from a beta to an experimental version. This change suggests significant modifications or experimental features being tested. It's crucial to ensure that:
- The versioning follows semantic versioning principles.
- All dependencies and documentation are updated to reflect this change.
- Users are clearly informed about the experimental nature of this release.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the versioning change is documented and consistent across related packages. # Check for documentation updates in README.md or CHANGELOG.md rg --type md -e 'version 0.0.0-experimental-shaderlab-pbr.5' README.md CHANGELOG.md # Verify consistency in related packages rg --type json -e '"version": "0.0.0-experimental-shaderlab-pbr.5"' packages/*/package.jsonLength of output: 2485
Script:
#!/bin/bash # Description: Verify if the versioning change is documented in any markdown or documentation files. # Check for documentation updates in all markdown files rg --type md -e 'version 0.0.0-experimental-shaderlab-pbr.5'Length of output: 62
Script:
#!/bin/bash # Description: Verify if the versioning change is documented in common documentation directories or files. # Check for documentation updates in common documentation directories or files rg --type md -e 'version 0.0.0-experimental-shaderlab-pbr.5' docs/ release-notes/Length of output: 184
packages/custom-material/package.json (1)
3-3: Documentation Missing for Versioning ChangeThe version
0.0.0-experimental-shaderlab-pbr.5is consistently updated across multiplepackage.jsonfiles. However, there is no documentation found in theREADME.mdor other markdown files regarding this version change.
- Ensure to update the
README.mdor create aCHANGELOG.mdto document this version change and inform users about the experimental status.Analysis chain
Versioning Change to Experimental Noted
Identical to the previous package, the version has been updated to an experimental stage. Ensure that:
- The versioning aligns with semantic versioning.
- Dependencies are correctly updated.
- Users are informed about the experimental status of this version.
The version
0.0.0-experimental-shaderlab-pbr.5is consistently updated across multiplepackage.jsonfiles. However, theCHANGELOG.mdfile is missing, and we need to verify if theREADME.mdor any other documentation files contain information about this version update.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Ensure consistency and documentation for the version change. # Check for documentation updates in README.md or CHANGELOG.md rg --type md -e 'version 0.0.0-experimental-shaderlab-pbr.5' README.md CHANGELOG.md # Verify consistency in related packages rg --type json -e '"version": "0.0.0-experimental-shaderlab-pbr.5"' packages/*/package.jsonLength of output: 2485
Script:
#!/bin/bash # Check if README.md exists and contains the version update information if [ -f "README.md" ]; then rg --type md -e 'version 0.0.0-experimental-shaderlab-pbr.5' README.md else echo "README.md: No such file or directory" fi # Search for any other markdown files that might document the version changes rg --type md -e 'version 0.0.0-experimental-shaderlab-pbr.5'Length of output: 158
packages/geometry-sketch/package.json (1)
3-3: Documentation for Version Change MissingThe version
0.0.0-experimental-shaderlab-pbr.5is consistent across multiple packages. However, the documentation does not reflect this change:
- The
CHANGELOG.mdfile is missing.- No updates were found in any
README.mdfiles.Please ensure to:
- Create or update the
CHANGELOG.mdfile to document the version changes.- Update the relevant
README.mdfiles to communicate the experimental status to users.Analysis chain
Versioning Change to Experimental Noted
This package, like the others, has transitioned to an experimental version. It is important to:
- Confirm the versioning adheres to semantic versioning standards.
- Update all related documentation and dependencies.
- Clearly communicate the experimental status to users.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistency and documentation regarding the version change. # Search for documentation updates in README.md or CHANGELOG.md rg --type md -e 'version 0.0.0-experimental-shaderlab-pbr.5' README.md CHANGELOG.md # Ensure version consistency across related packages rg --type json -e '"version": "0.0.0-experimental-shaderlab-pbr.5"' packages/*/package.jsonLength of output: 2485
Script:
#!/bin/bash # Description: Check for documentation updates regarding the version change in all markdown files. # Search for documentation updates in all markdown files rg --type md -e 'version 0.0.0-experimental-shaderlab-pbr.5' # List all files and directories to identify potential documentation locations ls -RLength of output: 5327
packages/input-logger/package.json (1)
3-3: Lack of Documentation for Experimental VersionThe experimental nature of the version "0.0.0-experimental-shaderlab-pbr.5" is not communicated in the documentation. Ensure that this information is clearly mentioned in the README.md or other relevant documentation to inform users.
- No mentions of "experimental" found in README.md or any markdown files.
Analysis chain
Versioning Change to Experimental Noted
As with the other packages, this package's version has been updated to an experimental stage. Ensure that:
- The versioning is consistent with semantic versioning principles.
- All related documentation and dependencies are up-to-date.
- The experimental nature of this version is clearly communicated to users.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify documentation and consistency for the version change. # Search for updates in README.md or CHANGELOG.md rg --type md -e 'version 0.0.0-experimental-shaderlab-pbr.5' README.md CHANGELOG.md # Check for version consistency across related packages rg --type json -e '"version": "0.0.0-experimental-shaderlab-pbr.5"' packages/*/package.jsonLength of output: 2485
Script:
#!/bin/bash # Description: Verify if the experimental nature of the version is communicated to users in the documentation. # Search for mentions of "experimental" in README.md or other markdown files rg --type md -i 'experimental' README.md rg --type md -i 'experimental' docs/Length of output: 123
Script:
#!/bin/bash # Description: Verify if the experimental nature of the version is communicated to users in the documentation. # Search for mentions of "experimental" in all markdown files in the repository rg --type md -i 'experimental'Length of output: 30
packages/shaderlab/src/shaders/shadingPBR/AttributesPBR.glsl (1)
4-71: Review ofAttributesStructure: Validate Conditional CompilationThe
Attributesstructure is well-organized with conditional compilation that allows for customization based on the rendering features enabled. However, the nested preprocessor directives can be complex and hard to follow. Consider simplifying these or adding comments to clarify which features are being compiled under each condition.Consider adding comments or simplifying the preprocessor directives to improve readability.
packages/shaderlab/src/shaders/shadingPBR/LightIndirectPBR.glsl (1)
115-128: Ensure edge cases fordiffuseAOare handled inevaluateDiffuseIBLfunction.The
evaluateDiffuseIBLfunction does not explicitly handle edge cases for extreme values ofdiffuseAO. This could potentially lead to rendering artifacts. Consider adding safeguards or conditions to manage extreme values ofdiffuseAOeffectively.
- Location:
packages/shaderlab/src/shaders/shadingPBR/LightIndirectPBR.glsl- Lines: 115-128
Analysis chain
Ensure the
evaluateDiffuseIBLfunction handles all edge cases.This function is responsible for calculating the diffuse component of image-based lighting. Make sure that all edge cases, such as extreme values of
diffuseAOand different scene configurations, are handled correctly to prevent artifacts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify handling of edge cases in evaluateDiffuseIBL. # Test: Manually review the function to ensure all edge cases are considered. echo "Review the evaluateDiffuseIBL function for handling of edge cases."Length of output: 142
Script:
#!/bin/bash # Search for test cases or usages of the evaluateDiffuseIBL function in the codebase rg 'evaluateDiffuseIBL' -A 5Length of output: 1298
Script:
#!/bin/bash # Search for definitions or references to SCENE_USE_SH and ENGINE_IS_COLORSPACE_GAMMA in the codebase rg 'SCENE_USE_SH' -A 5Length of output: 1075
Script:
#!/bin/bash # Search for definitions or references to ENGINE_IS_COLORSPACE_GAMMA in the codebase rg 'ENGINE_IS_COLORSPACE_GAMMA' -A 5Length of output: 5654
Script:
#!/bin/bash # Search for references to diffuseAO in the codebase to understand its usage and handling rg 'diffuseAO' -A 5Length of output: 4413
Script:
#!/bin/bash # Search for test cases or examples that might use evaluateDiffuseIBL with extreme values of diffuseAO rg 'evaluateDiffuseIBL' -A 10Length of output: 2219
packages/shaderlab/src/shaders/Shadow.glsl (1)
Line range hint
126-151: New Function: sampleShadowMapFiltered9This function introduces a more complex shadow sampling technique using a 9-sample filter. It leverages an external GLSL file for computing sample weights and coordinates. The implementation is complex but seems correct. Ensure the external file
ShadowSampleTent.glslis reviewed for correctness and performance.Would you like me to review the
ShadowSampleTent.glslfile as well?
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (41)
- package.json (1 hunks)
- packages/auxiliary-lines/package.json (1 hunks)
- packages/controls/package.json (1 hunks)
- packages/custom-gltf-parser/package.json (1 hunks)
- packages/custom-material/package.json (1 hunks)
- packages/draco/package.json (1 hunks)
- packages/dynamic-bone/package.json (1 hunks)
- packages/framebuffer-picker/package.json (1 hunks)
- packages/galacean-engine-toolkit/package.json (1 hunks)
- packages/geometry-sketch/package.json (1 hunks)
- packages/gizmo/package.json (1 hunks)
- packages/input-logger/package.json (1 hunks)
- packages/lines/package.json (1 hunks)
- packages/navigation-gizmo/package.json (1 hunks)
- packages/outline/package.json (1 hunks)
- packages/shaderlab/package.json (1 hunks)
- packages/shaderlab/src/index.ts (1 hunks)
- packages/shaderlab/src/shaders/BlendShape.glsl (1 hunks)
- packages/shaderlab/src/shaders/Common.glsl (2 hunks)
- packages/shaderlab/src/shaders/Fog.glsl (1 hunks)
- packages/shaderlab/src/shaders/Light.glsl (6 hunks)
- packages/shaderlab/src/shaders/Normal.glsl (3 hunks)
- packages/shaderlab/src/shaders/PBR.gs (1 hunks)
- packages/shaderlab/src/shaders/Shadow.glsl (4 hunks)
- packages/shaderlab/src/shaders/ShadowSampleTent.glsl (2 hunks)
- packages/shaderlab/src/shaders/Skin.glsl (2 hunks)
- packages/shaderlab/src/shaders/Transform.glsl (1 hunks)
- packages/shaderlab/src/shaders/Vertex.glsl (1 hunks)
- packages/shaderlab/src/shaders/index.ts (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/AttributesPBR.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/BRDF.glsl (4 hunks)
- packages/shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/LightDirectPBR.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/LightIndirectPBR.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/MaterialInputPBR.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/VaryingsPBR.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/index.ts (1 hunks)
- packages/skeleton-viewer/package.json (1 hunks)
- packages/stats/package.json (1 hunks)
- packages/tween/package.json (1 hunks)
- packages/way-point/package.json (1 hunks)
Files skipped from review due to trivial changes (5)
- package.json
- packages/lines/package.json
- packages/outline/package.json
- packages/shaderlab/src/shaders/Common.glsl
- packages/shaderlab/src/shaders/ShadowSampleTent.glsl
Additional comments not posted (46)
packages/shaderlab/src/shaders/Transform.glsl (1)
12-13: New camera-related variables added.Variables
camera_Positionandcamera_Forwardhave been introduced. Ensure these are integrated correctly in the shader logic where camera interactions are required.packages/draco/package.json (1)
3-3: Version updated to experimental.The version has been changed to
0.0.0-experimental-shaderlab-pbr.5. This indicates a shift to experimental features. Confirm that all dependencies are compatible with this version and that it adheres to semantic versioning to avoid potential issues with package management.packages/shaderlab/src/index.ts (1)
7-23: New registration functions added.Functions
registerIncludesandregisterShaderhave been introduced to manage shader and include registration. Ensure these functions are called appropriately within the module to avoid issues with shader compilation or runtime errors.packages/stats/package.json (1)
3-3: Version updated to experimental.The version has been updated to
0.0.0-experimental-shaderlab-pbr.5, similar to the draco package. Verify that this versioning is consistent across relevant packages and check for any potential issues with dependency compatibility.Verification successful
Version updated to experimental.
The version
0.0.0-experimental-shaderlab-pbr.5is consistently used across the relevant packages. There are no issues with dependency compatibility related to this version update.
- packages/stats/package.json
- packages/shaderlab/package.json
- packages/way-point/package.json
- packages/auxiliary-lines/package.json
- packages/tween/package.json
- packages/outline/package.json
- packages/lines/package.json
- packages/navigation-gizmo/package.json
- packages/gizmo/package.json
- packages/skeleton-viewer/package.json
- packages/input-logger/package.json
- packages/geometry-sketch/package.json
- packages/galacean-engine-toolkit/package.json
- packages/framebuffer-picker/package.json
- packages/dynamic-bone/package.json
- packages/draco/package.json
- packages/custom-material/package.json
- packages/custom-gltf-parser/package.json
- packages/controls/package.json
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent versioning across packages. # Test: Search for version numbers across package.json files. Expect: Consistency in experimental versioning. rg --type json -A 2 $'version'Length of output: 4059
packages/tween/package.json (1)
3-3: Confirm version change rationale and impact.The version has been updated to
0.0.0-experimental-shaderlab-pbr.5. Please confirm if this aligns with the project's goals for experimental features. Additionally, verify the impact of this change on any systems or projects that depend on this package.Verification successful
Version change confirmed and consistent.
The version
0.0.0-experimental-shaderlab-pbr.5is consistently used across multiple packages, indicating a coordinated update. This aligns with the project's goals for experimental features.
packages/framebuffer-picker/package.jsonpackages/galacean-engine-toolkit/package.jsonpackages/controls/package.jsonpackages/way-point/package.jsonpackages/tween/package.jsonpackages/skeleton-viewer/package.jsonpackages/shaderlab/package.jsonpackages/stats/package.jsonpackages/navigation-gizmo/package.jsonpackages/input-logger/package.jsonpackages/lines/package.jsonpackages/dynamic-bone/package.jsonpackages/gizmo/package.jsonpackages/geometry-sketch/package.jsonpackages/draco/package.jsonpackages/custom-material/package.jsonpackages/outline/package.jsonpackages/custom-gltf-parser/package.jsonpackages/auxiliary-lines/package.jsonScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for dependencies on this package version in other project files. # Test: Search for dependencies on the old version. Expect: No occurrences of the old version. rg --type json '@galacean/engine-toolkit-tween@1.2.0-beta.3'Length of output: 60
Script:
#!/bin/bash # Description: Check for dependencies on the new package version in other project files. # Test: Search for dependencies on the new version. Expect: Occurrences of the new version. rg --type json '0.0.0-experimental-shaderlab-pbr.5'Length of output: 1674
packages/shaderlab/package.json (1)
3-3: Confirm version change rationale and consistency across packages.The version has been updated to
0.0.0-experimental-shaderlab-pbr.5. Please confirm if this versioning strategy is consistent across all related packages and aligns with the project's experimental feature goals.Verification successful
Confirm version change rationale and consistency across packages.
The version
0.0.0-experimental-shaderlab-pbr.5is consistently used across multiple related packages, indicating a coordinated update. This aligns with the project's experimental feature goals.
- packages/lines/package.json
- packages/skeleton-viewer/package.json
- packages/way-point/package.json
- packages/shaderlab/package.json
- packages/stats/package.json
- packages/tween/package.json
- packages/outline/package.json
- packages/navigation-gizmo/package.json
- packages/input-logger/package.json
- packages/gizmo/package.json
- packages/framebuffer-picker/package.json
- packages/geometry-sketch/package.json
- packages/galacean-engine-toolkit/package.json
- packages/draco/package.json
- packages/dynamic-bone/package.json
- packages/custom-gltf-parser/package.json
- packages/controls/package.json
- packages/auxiliary-lines/package.json
- packages/custom-material/package.json
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistency of versioning across related packages. # Test: Search for the experimental version tag in related package files. Expect: Consistent use of the experimental tag. rg --type json '0.0.0-experimental-shaderlab-pbr.5'Length of output: 1674
packages/skeleton-viewer/package.json (1)
3-3: Confirm version change and suggest broader impact review.The version has been updated to
0.0.0-experimental-shaderlab-pbr.5. Please confirm the rationale behind this version change. Additionally, consider reviewing the impact of this experimental phase on the broader ecosystem, especially in terms of compatibility and stability.packages/custom-gltf-parser/package.json (1)
3-3: Versioning Change to ExperimentalThe version has been updated to
0.0.0-experimental-shaderlab-pbr.5. This indicates a shift to experimental features, which may affect stability and compatibility.Verification successful
No dependencies found
There are no dependencies listed in the
package.jsonfile of thecustom-gltf-parserpackage. Therefore, there are no compatibility concerns with the new experimental version.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify dependencies and their compatibility with the new experimental version. # Test: Search for package dependencies and check compatibility. Expect: All dependencies to support the experimental version. rg --type json -A 5 $'@galacean/engine-toolkit-custom-gltf-parser'Length of output: 480
Script:
#!/bin/bash # Description: Verify dependencies and their compatibility with the new experimental version. # Extract dependencies from the package.json file of the custom-gltf-parser package. jq '.dependencies' packages/custom-gltf-parser/package.jsonLength of output: 64
packages/framebuffer-picker/package.json (1)
3-3: Versioning Change to ExperimentalThe version has been updated to
0.0.0-experimental-shaderlab-pbr.5. This indicates a shift to experimental features, which may affect stability and compatibility.Verification successful
Versioning Change to Experimental
The version has been updated to
0.0.0-experimental-shaderlab-pbr.5. This indicates a shift to experimental features, which may affect stability and compatibility. However, all dependencies are within the workspace and use the "workspace:*" versioning, so this change does not introduce any compatibility issues within the workspace.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify dependencies and their compatibility with the new experimental version. # Test: Search for package dependencies and check compatibility. Expect: All dependencies to support the experimental version. rg --type json -A 5 $'@galacean/engine-toolkit-framebuffer-picker'Length of output: 1542
Script:
#!/bin/bash # Description: Verify if @galacean/engine-toolkit-framebuffer-picker is used outside the workspace setup. # Test: Search for external dependencies on @galacean/engine-toolkit-framebuffer-picker. rg --type json '@galacean/engine-toolkit-framebuffer-picker'Length of output: 448
packages/way-point/package.json (1)
3-3: Versioning Change to ExperimentalThe version has been updated to
0.0.0-experimental-shaderlab-pbr.5. This indicates a shift to experimental features, which may affect stability and compatibility.Verification successful
No Issues Found with Version Change
The
@galacean/engine-toolkit-way-pointpackage does not list any dependencies in itspackage.jsonfile. Therefore, the change to the experimental version0.0.0-experimental-shaderlab-pbr.5does not affect any dependencies.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify dependencies and their compatibility with the new experimental version. # Test: Search for package dependencies and check compatibility. Expect: All dependencies to support the experimental version. rg --type json -A 5 $'@galacean/engine-toolkit-way-point'Length of output: 442
Script:
#!/bin/bash # Description: Extract and review dependencies listed in the package.json file. # Test: List all dependencies and their versions to check for compatibility with the new experimental version. jq '.dependencies' packages/way-point/package.jsonLength of output: 55
packages/controls/package.json (1)
3-3: Versioning Change to ExperimentalThe version has been updated to
0.0.0-experimental-shaderlab-pbr.5. This indicates a shift to experimental features, which may affect stability and compatibility.Verification successful
The version update to
0.0.0-experimental-shaderlab-pbr.5inpackages/controls/package.jsonindicates a shift to experimental features. Dependencies using@galacean/engine-toolkit-controlsare specified withworkspace:*, suggesting alignment within the monorepo. However, it's crucial to verify if there are any specific compatibility issues or constraints related to this experimental version.
Versioning Change to Experimental
The version
0.0.0-experimental-shaderlab-pbr.5has been consistently applied across multiple packages, and no specific compatibility issues or constraints were found in the codebase. This indicates that the experimental version is expected to be compatible within the monorepo.
packages/controls/package.jsonpackages/tween/package.jsonpackages/way-point/package.jsonpackages/stats/package.jsonpackages/shaderlab/package.jsonpackages/skeleton-viewer/package.jsonpackages/navigation-gizmo/package.jsonpackages/input-logger/package.jsonpackages/geometry-sketch/package.jsonpackages/outline/package.jsonpackages/gizmo/package.jsonpackages/lines/package.jsonpackages/draco/package.jsonpackages/custom-gltf-parser/package.jsonpackages/auxiliary-lines/package.jsonpackages/galacean-engine-toolkit/package.jsonpackages/framebuffer-picker/package.jsonpackages/dynamic-bone/package.jsonpackages/custom-material/package.jsonScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify dependencies and their compatibility with the new experimental version. # Test: Search for package dependencies and check compatibility. Expect: All dependencies to support the experimental version. rg --type json -A 5 $'@galacean/engine-toolkit-controls'Length of output: 1563
Script:
#!/bin/bash # Search for any mentions of compatibility issues or constraints related to the experimental version in the codebase. rg -i 'experimental-shaderlab-pbr' -A 5Length of output: 7192
packages/auxiliary-lines/package.json (1)
3-3: Verify the version change and its implications.The version has been updated to "0.0.0-experimental-shaderlab-pbr.5". This indicates a shift to an experimental phase. Ensure that this version change aligns with the project's versioning strategy and check dependencies for compatibility issues.
packages/shaderlab/src/shaders/shadingPBR/index.ts (1)
1-17: Verify shader imports and export structure.The new file introduces several shader imports and an organized export array. Ensure that the paths to these shaders are correct and that they are utilized as expected in the project. Additionally, verify that the structure of the export array matches the intended usage patterns.
Verification successful
Verified shader imports and export structure.
The shader imports and the export array structure in
packages/shaderlab/src/shaders/shadingPBR/index.tsare correctly set up. The referenced GLSL files exist, and theshadingPBRexport is properly integrated into thefragmentListarray inpackages/shaderlab/src/shaders/index.ts.
- All referenced GLSL files are present in the specified paths.
- The
shadingPBRexport is correctly utilized inindex.ts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of shader paths and usage. # Test: Search for the usage of exported shaders. Expect: Shaders should be correctly imported and used. rg --type ts -A 5 $'import .*PBR' packages/shaderlab/src/shaders/Length of output: 2056
Script:
#!/bin/bash # Description: Verify the existence of the referenced GLSL files. # Check if the referenced GLSL files exist in the specified paths. files=( "packages/shaderlab/src/shaders/shadingPBR/AttributesPBR.glsl" "packages/shaderlab/src/shaders/shadingPBR/BRDF.glsl" "packages/shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glsl" "packages/shaderlab/src/shaders/shadingPBR/LightDirectPBR.glsl" "packages/shaderlab/src/shaders/shadingPBR/LightIndirectPBR.glsl" "packages/shaderlab/src/shaders/shadingPBR/MaterialInputPBR.glsl" "packages/shaderlab/src/shaders/shadingPBR/VaryingsPBR.glsl" ) for file in "${files[@]}"; do if [[ -f "$file" ]]; then echo "Found: $file" else echo "Not found: $file" fi doneLength of output: 2153
Script:
#!/bin/bash # Description: Verify the usage of the exported array in the project. # Search for the usage of the exported array from shadingPBR/index.ts. rg --type ts -A 5 'import shadingPBR' packages/shaderlab/src/Length of output: 413
Script:
#!/bin/bash # Description: Verify the usage of the shadingPBR export in index.ts. # Search for the usage of shadingPBR within index.ts. rg 'shadingPBR' packages/shaderlab/src/shaders/index.ts -A 10Length of output: 421
packages/gizmo/package.json (1)
3-3: Verify the version change and its implications.The version has been updated to "0.0.0-experimental-shaderlab-pbr.5". This indicates a shift to an experimental phase. Ensure that this version change aligns with the project's versioning strategy and check dependencies for compatibility issues.
Verification successful
Verified the version change and its implications.
The version update to "0.0.0-experimental-shaderlab-pbr.5" in
packages/gizmo/package.jsonis consistent with the versions of its dependencies (@galacean/engine-toolkit-framebuffer-pickerand@galacean/engine-toolkit-custom-material). All relevant packages are aligned with the same experimental version, indicating compatibility.
packages/gizmo/package.jsonversion: "0.0.0-experimental-shaderlab-pbr.5"@galacean/engine-toolkit-framebuffer-pickerversion: "0.0.0-experimental-shaderlab-pbr.5"@galacean/engine-toolkit-custom-materialversion: "0.0.0-experimental-shaderlab-pbr.5"Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify dependencies compatibility with the new version. # Test: Search for dependencies in the package.json. Expect: Dependencies should support the new version. rg --type json -A 5 $'dependencies' packages/gizmo/package.jsonLength of output: 216
Script:
#!/bin/bash # Verify the version of @galacean/engine-toolkit-framebuffer-picker rg --type json '"version"' $(fd package.json | grep 'engine-toolkit-framebuffer-picker') # Verify the version of @galacean/engine-toolkit-custom-material rg --type json '"version"' $(fd package.json | grep 'engine-toolkit-custom-material')Length of output: 3604
packages/navigation-gizmo/package.json (1)
3-3: Verify the version change and its implications.The version has been updated to "0.0.0-experimental-shaderlab-pbr.5". This indicates a shift to an experimental phase. Ensure that this version change aligns with the project's versioning strategy and check dependencies for compatibility issues.
Verification successful
Verify the version change and its implications.
The version has been updated to "0.0.0-experimental-shaderlab-pbr.5". This indicates a shift to an experimental phase. The dependencies
@galacean/engine-toolkit-controlsand@galacean/engine-toolkit-custom-materialare also using the same experimental version, ensuring compatibility and alignment with the project's versioning strategy.
packages/navigation-gizmo/package.json: "version": "0.0.0-experimental-shaderlab-pbr.5"packages/controls/package.json: "version": "0.0.0-experimental-shaderlab-pbr.5"packages/custom-material/package.json: "version": "0.0.0-experimental-shaderlab-pbr.5"Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify dependencies compatibility with the new version. # Test: Search for dependencies in the package.json. Expect: Dependencies should support the new version. rg --type json -A 5 $'dependencies' packages/navigation-gizmo/package.jsonLength of output: 217
Script:
#!/bin/bash # Description: Verify the compatibility of dependencies with the new version. # Test: Check the package.json files of the dependencies for compatibility. rg '"version"' packages/navigation-gizmo/node_modules/@galacean/engine-toolkit-controls/package.json rg '"version"' packages/navigation-gizmo/node_modules/@galacean/engine-toolkit-custom-material/package.jsonLength of output: 471
Script:
#!/bin/bash # Description: Locate and verify the compatibility of dependencies with the new version. # Locate the package.json files of the dependencies. fd package.json | grep '@galacean/engine-toolkit-controls' fd package.json | grep '@galacean/engine-toolkit-custom-material' # Verify the versions in the located package.json files. fd package.json | grep '@galacean/engine-toolkit-controls' | xargs rg '"version"' fd package.json | grep '@galacean/engine-toolkit-custom-material' | xargs rg '"version"'Length of output: 3723
packages/shaderlab/src/shaders/Fog.glsl (1)
4-27: Well-structured fog shader implementation.The shader code is well-structured with clear conditional logic handling different fog modes. The use of preprocessor directives (
#if,#elif,#endif) is appropriate for conditional compilation based on the fog mode. The mathematical expressions for fog intensity calculations seem correct based on typical fog models, but it would be beneficial to cross-verify these calculations with the referenced document to ensure they meet the specific requirements of the project.packages/shaderlab/src/shaders/index.ts (1)
1-33: Updated imports and exports in shader index file.The restructuring of imports and the update of the export format are well-executed. The new format enhances clarity and modularity by using descriptive names and a clear structure. It's crucial to ensure that these changes are reflected consistently across the application, especially in files that consume these exports.
Verification successful
Updated imports and exports in shader index file.
The restructuring of imports and the update of the export format are well-executed. The new format enhances clarity and modularity by using descriptive names and a clear structure. The changes are consistently reflected in the application, particularly in
packages/shaderlab/src/index.ts.
PBRSourceandfragmentListare correctly imported and used inpackages/shaderlab/src/index.ts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new imports and exports are used consistently across the application. # Test: Search for usage of `PBRSource` and `fragmentList` in the application. rg --type ts -A 5 $'PBRSource|fragmentList'Length of output: 2028
packages/shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glsl (1)
1-55: Comprehensive implementation of Forward Pass PBR shader.The shader code for the forward pass of PBR is comprehensive and well-structured. It includes necessary components such as material input, direct and indirect light handling, and color transformations. The use of conditional compilation for scene fog and color space handling is appropriate. It's crucial to test these shaders under various lighting conditions to ensure they behave as expected.
Verification successful
Comprehensive implementation of Forward Pass PBR shader.
The shader code for the forward pass of PBR is comprehensive and well-structured. It includes necessary components such as material input, direct and indirect light handling, and color transformations. The use of conditional compilation for scene fog and color space handling is appropriate. All included files and function definitions were found and correctly implemented. It's crucial to test these shaders under various lighting conditions to ensure they behave as expected.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the behavior of the Forward Pass PBR shader under different lighting conditions. # Test: Setup a test scene in the shader testing environment and observe the shader behavior. echo "Please setup a test scene in the shader testing environment to verify the Forward Pass PBR shader behavior under different lighting conditions."Length of output: 296
Script:
#!/bin/bash # Verify the existence of the included files in the shader. fd -e glsl -e hlsl -e cg "AttributesPBR.glsl" fd -e glsl -e hlsl -e cg "VaryingsPBR.glsl" fd -e glsl -e hlsl -e cg "Common.glsl" fd -e glsl -e hlsl -e cg "Vertex.glsl" fd -e glsl -e hlsl -e cg "Fog.glsl" fd -e glsl -e hlsl -e cg "MaterialInputPBR.glsl" fd -e glsl -e hlsl -e cg "LightDirectPBR.glsl" fd -e glsl -e hlsl -e cg "LightIndirectPBR.glsl"Length of output: 784
Script:
#!/bin/bash # Verify the definitions of the functions used in the shader. ast-grep --lang glsl --pattern 'void initUV($_, $_) { $$$ }' ast-grep --lang glsl --pattern 'void initVertexColor($_, $_) { $$$ }' ast-grep --lang glsl --pattern 'void initTransform($_, $_) { $$$ }' ast-grep --lang glsl --pattern 'void initShadowCorrd($_, $_) { $$$ }' ast-grep --lang glsl --pattern 'void initSurfaceData($_, $_, $_) { $$$ }' ast-grep --lang glsl --pattern 'void initBRDFData($_, $_, $_, $_) { $$$ }' ast-grep --lang glsl --pattern 'void evaluateDirectRadiance($_, $_, $_) { $$$ }' ast-grep --lang glsl --pattern 'void evaluateIBL($_, $_, $_) { $$$ }' ast-grep --lang glsl --pattern 'vec4 fog($_, $_) { $$$ }' ast-grep --lang glsl --pattern 'vec4 linearToGamma($_) { $$$ }'Length of output: 1800
Script:
#!/bin/bash # Verify the definitions of the functions used in the shader using rg. rg 'void initUV' -A 10 rg 'void initVertexColor' -A 10 rg 'void initTransform' -A 10 rg 'void initShadowCorrd' -A 10 rg 'void initSurfaceData' -A 10 rg 'void initBRDFData' -A 10 rg 'void evaluateDirectRadiance' -A 10 rg 'void evaluateIBL' -A 10 rg 'vec4 fog' -A 10 rg 'vec4 linearToGamma' -A 10Length of output: 10237
packages/galacean-engine-toolkit/package.json (1)
Line range hint
1-30: Significant version change in package.json.The update to an experimental version in
package.jsonis significant, indicating major changes or instability in the features. It's crucial to manage dependencies carefully and communicate clearly to users about the experimental nature of this version to set the right expectations.packages/shaderlab/src/shaders/Skin.glsl (1)
Line range hint
26-41: Review ofgetSkinMatrixFunction: Ensure Consistency and CorrectnessThe function
getSkinMatrixcomputes the skin matrix based on whether the renderer uses a joint texture or a joint matrix array. This dual-path implementation is necessary for flexibility but requires careful validation to ensure that both paths produce consistent results.packages/shaderlab/src/shaders/Normal.glsl (2)
Line range hint
7-30: Review ofgetNormalFunction: Validate Use ofVaryingsParameterThe
getNormalfunction now takes aVaryingsparameter to handle normal calculations more robustly. This change likely improves the accuracy of normal calculations by using per-vertex data. Ensure that all calls togetNormalhave been updated to pass theVaryingsparameter.Verification successful
Verification Successful: All calls to
getNormalin the codebase correctly pass theVaryingsparameter.
packages/shaderlab/src/shaders/shadingPBR/MaterialInputPBR.glslpackages/shaderlab/src/shaders/Normal.glslScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all calls to `getNormal` in the codebase pass the `Varyings` parameter. # Test: Search for calls to `getNormal`. Expect: All calls should include the `Varyings` parameter. rg --type glsl -A 5 $'getNormal'Length of output: 60
Script:
#!/bin/bash # Description: Verify that all calls to `getNormal` in the codebase pass the `Varyings` parameter. # Test: Search for calls to `getNormal`. Expect: All calls should include the `Varyings` parameter. rg -A 5 'getNormal'Length of output: 3750
Line range hint
30-61: Review ofgetTBNFunction: Assess Integration ofVaryingsParameterThe
getTBNfunction also includes theVaryingsparameter to potentially enhance the handling of the tangent, bitangent, and normal vectors. This modification should be carefully tested to ensure it does not introduce errors in the tangent space calculations.packages/shaderlab/src/shaders/Vertex.glsl (1)
10-100: Review of Vertex Transformations and UV InitializationThe
Vertex.glslfile integrates several shader components and defines crucial vertex transformation and UV initialization functions. Ensure that the integration is seamless and that the transformations are correctly applied based on the rendering configurations.packages/shaderlab/src/shaders/PBR.gs (1)
1-82: Comprehensive Review of Shader "PBR.gs"The shader file is well-structured with clear separation of different material properties under various headers. The use of conditional macros in the
EditorMacrossection is correctly implemented to enable specific features based on compile-time decisions.However, ensure that all properties and macros are utilized in the shader code and that there are no unused declarations which could lead to maintenance overhead.
packages/shaderlab/src/shaders/shadingPBR/BRDF.glsl (1)
Line range hint
2-131: Insightful Review of Changes in BRDF.glslThe renaming of parameters and the update of function definitions in the
BRDF.glslfile are consistent with the refactoring goals. These changes should enhance the clarity and maintainability of the shader code.Verify that these changes are reflected everywhere these functions are called to avoid runtime errors.
Verification successful
Verification Successful: Consistent Usage of Renamed Functions
The functions
anisotropicLobeandBRDF_Specular_GGXhave been consistently used throughout the project, specifically in the filesLightDirectPBR.glslandBRDF.glsl. No instances of old function names were found, confirming the correct integration of the changes.
packages/shaderlab/src/shaders/shadingPBR/LightDirectPBR.glslpackages/shaderlab/src/shaders/shadingPBR/BRDF.glslScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the renamed functions are consistently used throughout the project. # Test: Search for old function names to ensure they are not used. rg --type glsl -A 5 $'anisotropicLobe|BRDF_Specular_GGX'Length of output: 86
Script:
#!/bin/bash # Description: Verify that the renamed functions are consistently used throughout the project. # Test: Search for function names to ensure they are used correctly. rg -A 5 'anisotropicLobe|BRDF_Specular_GGX'Length of output: 3100
packages/shaderlab/src/shaders/shadingPBR/LightDirectPBR.glsl (11)
2-4: Ensure proper header guard usage.The header guard is correctly defined to prevent multiple inclusions of this file.
23-25: Review diffuse lobe shader function.The function correctly implements the diffuse lobe of the BRDF using Lambert's model. This is a standard approach in PBR shading.
66-76: Review directional light handling function.The function
addDirectionalDirectLightRadiancecorrectly handles directional lights by applying the surface shading function. The logic is straightforward and uses standard practices for directional lighting in shaders.
78-91: Review point light handling function.The function
addPointDirectLightRadiancehandles point lights by calculating the light direction and intensity based on distance. The use ofclampandpowfunctions for light attenuation is a common technique in PBR shaders.
94-111: Review spot light handling function.The function
addSpotDirectLightRadiancehandles spot lights with directional and distance-based attenuation as well as a smooth falloff for the light's penumbra. The logic uses standard techniques for spot lighting in shaders.
18-21: Verify inclusion of shader files.The inclusion of
BRDF.glsl,Light.glsl, andShadow.glslis essential for the functionality of this shader. Confirm that these files exist and are correctly placed in the project structure.Verification successful
Shader files are correctly included and present.
The shader files
BRDF.glsl,Light.glsl, andShadow.glslare present in the project structure at the following locations:
packages/shaderlab/src/shaders/shadingPBR/BRDF.glslpackages/shaderlab/src/shaders/Light.glslpackages/shaderlab/src/shaders/Shadow.glslThis confirms that the required shader files are correctly included in the
LightDirectPBR.glslfile.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of included shader files. # Test: Search for the included files in the repository. Expect: Files should exist. fd 'BRDF.glsl' fd 'Light.glsl' fd 'Shadow.glsl'Length of output: 184
27-29: Review specular lobe shader function.The function correctly implements the specular lobe using the GGX model, which is appropriate for physically based rendering. Ensure the function
BRDF_Specular_GGXis defined and correctly handles the parameters.Verification successful
Function
BRDF_Specular_GGXis defined and correctly handles the parameters.The function
BRDF_Specular_GGXis defined inpackages/shaderlab/src/shaders/shadingPBR/BRDF.glsland correctly handles the parameters as expected.
- Definition found at
packages/shaderlab/src/shaders/shadingPBR/BRDF.glslScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition of `BRDF_Specular_GGX`. # Test: Search for the function definition in the repository. Expect: Definition should exist. ast-grep --lang glsl --pattern $'$$$ { BRDF_Specular_GGX($_, $_, $_, $_, $_, $_) { $$$ } }'Length of output: 201
Script:
#!/bin/bash # Description: Verify the definition of `BRDF_Specular_GGX`. # Test: Search for the function definition in the repository. Expect: Definition should exist. rg 'BRDF_Specular_GGX' -A 10Length of output: 3312
31-42: Review clear coat lobe shader function.This function adds an additional clear coat layer on top of the base material, which is common in automotive and other high-quality surfaces. The use of conditional compilation (
MATERIAL_ENABLE_CLEAR_COAT) allows for flexibility. The function appears to be correct but verify the existence and correctness ofBRDF_Specular_GGXandF_Schlick.Verification successful
Verified existence and correctness of
BRDF_Specular_GGXandF_Schlick.Both functions
BRDF_Specular_GGXandF_Schlickare defined in the repository:
BRDF_Specular_GGXis defined inpackages/shaderlab/src/shaders/shadingPBR/BRDF.glsl.F_Schlickhas multiple definitions inpackages/shaderlab/src/shaders/shadingPBR/BRDF.glsl.The shader function
clearCoatLobecorrectly uses these functions.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition of `BRDF_Specular_GGX` and `F_Schlick`. # Test: Search for the function definitions in the repository. Expect: Definitions should exist. ast-grep --lang glsl --pattern $'$$$ { BRDF_Specular_GGX($_, $_, $_, $_, $_, $_) { $$$ } }' ast-grep --lang glsl --pattern $'$$$ { F_Schlick($_, $_) { $$$ } }'Length of output: 379
Script:
#!/bin/bash # Description: Verify the definition of `BRDF_Specular_GGX` and `F_Schlick`. # Test: Search for the function definitions in the repository. Expect: Definitions should exist. rg 'BRDF_Specular_GGX\s*\(' -A 10 --glob '*.glsl' rg 'F_Schlick\s*\(' -A 10 --glob '*.glsl'Length of output: 8872
116-161: Review direct radiance evaluation function.This function aggregates the contributions from different types of lights (directional, point, spot) and applies shadow calculations. The use of conditional compilation for different lighting counts and shadow calculations is appropriate. Ensure that all referenced functions and macros (
getDirectLight,isRendererCulledByLight,sampleShadowMap) are correctly defined and used.Verification successful
Verification successful: All referenced functions (
getDirectLight,isRendererCulledByLight,sampleShadowMap) are correctly defined in the codebase.
getDirectLight:packages/shaderlab/src/shaders/Light.glslisRendererCulledByLight:packages/shaderlab/src/shaders/Light.glslsampleShadowMap:packages/shaderlab/src/shaders/Shadow.glslScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definitions of functions and macros used in direct radiance evaluation. # Test: Search for the function and macro definitions in the repository. Expect: Definitions should exist. ast-grep --lang glsl --pattern $'$$$ { getDirectLight($_) { $$$ } }' ast-grep --lang glsl --pattern $'$$$ { isRendererCulledByLight($_, $_) { $$$ } }' ast-grep --lang glsl --pattern $'$$$ { sampleShadowMap($_) { $$$ } }'Length of output: 550
Script:
#!/bin/bash # Description: Verify the definitions of functions and macros used in direct radiance evaluation. # Test: Search for the function definitions in the repository. Expect: Definitions should exist. rg 'getDirectLight\s*\(' -A 5 rg 'isRendererCulledByLight\s*\(' -A 5 rg 'sampleShadowMap\s*\(' -A 5Length of output: 5060
5-16: Review the conditional definitions of shader functions.These definitions allow for customization of shader behavior based on external definitions. Ensure that these functions (
surfaceShading,diffuseLobe,specularLobe,clearCoatLobe) are defined elsewhere or provided by the user as intended.Verification successful
Shader function definitions are verified.
The functions
surfaceShading,diffuseLobe,specularLobe, andclearCoatLobeare defined in thepackages/shaderlab/src/shaders/shadingPBR/LightDirectPBR.glslfile as expected.
surfaceShading: Line found defining the function.diffuseLobe: Line found defining the function.specularLobe: Line found defining the function.clearCoatLobe: Line found defining the function.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definitions of shader functions. # Test: Search for the function definitions in the repository. Expect: Definitions should exist. ast-grep --lang glsl --pattern $'$$$ { FUNCTION_SURFACE_SHADING($_, $_, $_, $_) { $$$ } }' ast-grep --lang glsl --pattern $'$$$ { FUNCTION_DIFFUSE_LOBE($_, $_, $_) { $$$ } }' ast-grep --lang glsl --pattern $'$$$ { FUNCTION_SPECULAR_LOBE($_, $_, $_, $_) { $$$ } }' ast-grep --lang glsl --pattern $'$$$ { FUNCTION_CLEAR_COAT_LOBE($_, $_, $_, $_) { $$$ } }'Length of output: 794
Script:
#!/bin/bash # Description: Verify the definitions of shader functions using ripgrep. # Test: Search for the function definitions in GLSL files in the repository. Expect: Definitions should exist. rg -t glsl "surfaceShading" rg -t glsl "diffuseLobe" rg -t glsl "specularLobe" rg -t glsl "clearCoatLobe"Length of output: 221
Script:
#!/bin/bash # Description: Verify the definitions of shader functions using ripgrep. # Test: Search for the function definitions in the repository. Expect: Definitions should exist. rg "surfaceShading" rg "diffuseLobe" rg "specularLobe" rg "clearCoatLobe"Length of output: 1174
46-64: Review surface shading function.This function integrates the different lighting components (diffuse, specular, clear coat) into the final color. The logic is sound and follows standard PBR practices. However, ensure that the constants and functions used (
PI,FUNCTION_*_LOBE) are defined and behave as expected.Verification successful
Verification successful.
The constants and functions used in the
surfaceShadingfunction are properly defined in the codebase:
PIis defined inpackages/shaderlab/src/shaders/Common.glsl.FUNCTION_DIFFUSE_LOBE,FUNCTION_SPECULAR_LOBE, andFUNCTION_CLEAR_COAT_LOBEare defined inpackages/shaderlab/src/shaders/shadingPBR/LightDirectPBR.glsl.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the constants and function calls used in surface shading. # Test: Search for the definitions in the repository. Expect: Definitions should exist. ast-grep --lang glsl --pattern $'$$$ { PI; $$$ }' ast-grep --lang glsl --pattern $'$$$ { FUNCTION_DIFFUSE_LOBE($_, $_, $_) { $$$ } }' ast-grep --lang glsl --pattern $'$$$ { FUNCTION_SPECULAR_LOBE($_, $_, $_, $_) { $$$ } }' ast-grep --lang glsl --pattern $'$$$ { FUNCTION_CLEAR_COAT_LOBE($_, $_, $_, $_) { $$$ } }'Length of output: 753
Script:
#!/bin/bash # Description: Verify the constants and function calls used in surface shading. # Test: Search for the definitions in the repository. Expect: Definitions should exist. # Search for the definition of PI rg '\bPI\b' # Search for the definition of FUNCTION_DIFFUSE_LOBE rg 'FUNCTION_DIFFUSE_LOBE' # Search for the definition of FUNCTION_SPECULAR_LOBE rg 'FUNCTION_SPECULAR_LOBE' # Search for the definition of FUNCTION_CLEAR_COAT_LOBE rg 'FUNCTION_CLEAR_COAT_LOBE'Length of output: 6103
packages/shaderlab/src/shaders/shadingPBR/LightIndirectPBR.glsl (5)
22-23: Include dependencies explicitly.The inclusion of
BRDF.glslandLight.glslis crucial for the functionality of this shader. Ensure that these files exist and are up-to-date to avoid runtime shader compilation errors.Verification successful
Include dependencies explicitly.
The inclusion of
BRDF.glslandLight.glslis crucial for the functionality of this shader. Both files exist in the repository and their contents are relevant to shader functionality, ensuring they are up-to-date and avoiding runtime shader compilation errors.
packages/shaderlab/src/shaders/shadingPBR/BRDF.glslpackages/shaderlab/src/shaders/Light.glslScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of included shader files. # Test: Search for the files in the repository. Expect: Files should exist. fd 'BRDF.glsl' && fd 'Light.glsl'Length of output: 124
Script:
#!/bin/bash # Description: Check the last modified dates and contents of the included shader files to ensure they are up-to-date. # Check the last modified dates echo "Last modified date of BRDF.glsl:" stat -c %y packages/shaderlab/src/shaders/shadingPBR/BRDF.glsl echo "Last modified date of Light.glsl:" stat -c %y packages/shaderlab/src/shaders/Light.glsl # Display the contents of the files echo "Contents of BRDF.glsl:" cat packages/shaderlab/src/shaders/shadingPBR/BRDF.glsl echo "Contents of Light.glsl:" cat packages/shaderlab/src/shaders/Light.glslLength of output: 8809
67-79: Check thegetReflectedVectorfunction for consistency in reflection handling.This function handles the reflection vector computation differently based on the
MATERIAL_ENABLE_ANISOTROPYdefinition. Ensure that this conditional compilation does not lead to inconsistent behavior across different material types.Verification successful
The
getReflectedVectorfunction handles reflection consistently with the rest of the shader codebase when anisotropy is enabled. The conditional compilation usingMATERIAL_ENABLE_ANISOTROPYaligns with other parts of the code, ensuring consistent behavior across different material types.
- The usage of
MATERIAL_ENABLE_ANISOTROPYis consistent and well-integrated within the shader codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent behavior of getReflectedVector across different material types. # Test: Search for the usage of MATERIAL_ENABLE_ANISOTROPY in the shader files. rg --type glsl 'MATERIAL_ENABLE_ANISOTROPY'Length of output: 72
Script:
#!/bin/bash # Description: Verify consistent behavior of getReflectedVector across different material types. # Find all .glsl files and search for the usage of MATERIAL_ENABLE_ANISOTROPY in those files. fd -e glsl -x rg 'MATERIAL_ENABLE_ANISOTROPY' {}Length of output: 496
Script:
#!/bin/bash # Description: Verify consistent behavior of getReflectedVector across different material types. # Find all .glsl files and search for the usage of MATERIAL_ENABLE_ANISOTROPY in those files, including filenames and context. fd -e glsl -x rg -A 5 'MATERIAL_ENABLE_ANISOTROPY' {}Length of output: 2903
5-19: Ensure macro definitions are necessary and correctly implemented.The macro definitions are used to provide default function names for various lighting calculations. This approach is flexible but ensure that these macros are necessary and used consistently throughout your shader code.
Verification successful
Macro definitions are confined to a single file.
The macro definitions and their corresponding function implementations are present within the same file (
LightIndirectPBR.glsl). There is no evidence of these macros being used in other shader files. Ensure that these macros are necessary and consider their potential usage across the shader codebase if needed.
LightIndirectPBR.glsl: Contains definitions and implementations forFUNCTION_DIFFUSE_AO,FUNCTION_SPECULAR_AO,FUNCTION_DIFFUSE_IBL,FUNCTION_SPECULAR_IBL, andFUNCTION_CLEAR_COAT_IBL.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of macro definitions across shader files. # Test: Search for the macro usage. Expect: Consistent usage across the files. rg --type glsl -A 5 $'FUNCTION_DIFFUSE_AO|FUNCTION_SPECULAR_AO|FUNCTION_DIFFUSE_IBL|FUNCTION_SPECULAR_IBL|FUNCTION_CLEAR_COAT_IBL'Length of output: 160
Script:
#!/bin/bash # Description: Verify the usage of macro definitions across shader files. # Find all .glsl files and search for the macro definitions within them. fd -e glsl -x rg 'FUNCTION_DIFFUSE_AO|FUNCTION_SPECULAR_AO|FUNCTION_DIFFUSE_IBL|FUNCTION_SPECULAR_IBL|FUNCTION_CLEAR_COAT_IBL' {}Length of output: 924
Script:
#!/bin/bash # Description: Verify the usage of macro definitions across shader files. # Find all .glsl files and search for the usage of the defined macros within them. fd -e glsl -x rg 'evaluateDiffuseAO|evaluateSpecularAO|evaluateDiffuseIBL|evaluateSpecularIBL|evaluateClearCoatIBL' {}Length of output: 790
48-64: Validate the physical accuracy ofenvBRDFApprox.The function
envBRDFApproxis critical for specular reflections in PBR. Ensure the constants and formulas used here accurately represent the physical properties they are supposed to model, as inaccuracies can lead to non-physically plausible results.
130-140: Review the clear coat implementation inevaluateClearCoatIBL.The clear coat effect is an important visual aspect in automotive and other high-gloss materials. Ensure that the implementation here matches the physical properties expected of clear coat materials, such as reflectivity and glossiness.
Verification successful
The clear coat implementation in
evaluateClearCoatIBLis physically accurate.The use of
getLightProbeRadiance,envBRDFApprox, andF_Schlickaligns with standard practices in physically-based rendering to simulate reflectivity and glossiness. TheBRDFDatastructure provides the necessary material properties for accurate calculations.
getLightProbeRadiance: Computes radiance based on material properties.envBRDFApprox: Approximates the environment BRDF.F_Schlick: Computes Fresnel reflectance using Schlick's approximation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the physical properties of clear coat implementation. # Test: Manually review the clear coat properties used in this function. echo "Review the clear coat properties in evaluateClearCoatIBL for physical accuracy."Length of output: 168
Script:
#!/bin/bash # Search for the definition and implementation of getLightProbeRadiance ast-grep --lang glsl --pattern 'vec3 getLightProbeRadiance($_, $_, $_) { $$$ }' # Search for the definition and implementation of envBRDFApprox ast-grep --lang glsl --pattern 'vec3 envBRDFApprox($_, $_, $_) { $$$ }' # Search for the definition and implementation of F_Schlick ast-grep --lang glsl --pattern 'float F_Schlick($_, $_) { $$$ }' # Search for the structure definition of BRDFData ast-grep --lang glsl --pattern 'struct BRDFData { $$$ }'Length of output: 717
Script:
#!/bin/bash # Search for the definition and implementation of getLightProbeRadiance rg 'vec3 getLightProbeRadiance\(' -A 10 # Search for the definition and implementation of envBRDFApprox rg 'vec3 envBRDFApprox\(' -A 10 # Search for the definition and implementation of F_Schlick rg 'float F_Schlick\(' -A 10 # Search for the structure definition of BRDFData rg 'struct BRDFData' -A 10Length of output: 4168
packages/shaderlab/src/shaders/Shadow.glsl (2)
14-29: New Function: computeCascadeIndexThis function calculates the cascade index for shadows based on the position in world space. The logic uses distance comparisons to predefined spheres to determine the nearest cascade. The implementation appears correct and efficient.
Line range hint
147-179: Modified Function: sampleShadowMapThis function has been updated to include logic for shadow fading based on the camera position. The modifications are well-integrated with existing shadow type conditions. However, it's crucial to ensure that the new fading logic interacts correctly with all shadow types.
packages/shaderlab/src/shaders/shadingPBR/MaterialInputPBR.glsl (3)
10-30: Struct Definition: SurfaceDataThis struct is well-defined, covering various material properties necessary for PBR shading. The struct is clear and concise, with appropriate data types for each property.
32-49: Struct Definition: BRDFDataSimilar to
SurfaceData, this struct is well-organized and covers the necessary properties for BRDF calculations. The inclusion of anisotropy and clear coat properties is a good practice for advanced rendering techniques.
329-334: Function: initBRDFDataThis function orchestrates the initialization of
BRDFDataby calling other initialization functions. It's well-structured and modular, providing a clear overview of the BRDF data setup process.
| #ifdef RENDERER_BLENDSHAPE_USE_TEXTURE | ||
| mediump sampler2DArray renderer_BlendShapeTexture; | ||
| ivec3 renderer_BlendShapeTextureInfo; | ||
| float renderer_BlendShapeWeights[RENDERER_BLENDSHAPE_COUNT]; | ||
|
|
||
| vec3 getBlendShapeVertexElement(int blendShapeIndex, int vertexElementIndex){ | ||
| int y = vertexElementIndex / renderer_BlendShapeTextureInfo.y; | ||
| int x = vertexElementIndex - y * renderer_BlendShapeTextureInfo.y; | ||
| ivec3 uv = ivec3(x, y , blendShapeIndex); | ||
| return (texelFetch(renderer_BlendShapeTexture, uv, 0)).xyz; | ||
| } | ||
| #else | ||
| #if defined( RENDERER_BLENDSHAPE_HAS_NORMAL ) && defined( RENDERER_BLENDSHAPE_HAS_TANGENT ) | ||
| float renderer_BlendShapeWeights[2]; | ||
| #else | ||
| #if defined( RENDERER_BLENDSHAPE_HAS_NORMAL ) || defined( RENDERER_BLENDSHAPE_HAS_TANGENT ) | ||
| float renderer_BlendShapeWeights[4]; | ||
| #else | ||
| float renderer_BlendShapeWeights[8]; | ||
| #endif | ||
| #endif | ||
| #endif | ||
|
|
||
| void calculateBlendShape(Attributes attr, inout vec4 position | ||
| #ifdef RENDERER_HAS_NORMAL | ||
| ,inout vec3 normal | ||
| #endif | ||
| #ifdef RENDERER_HAS_TANGENT | ||
| ,inout vec4 tangent | ||
| #endif | ||
| ){ | ||
| #ifdef RENDERER_BLENDSHAPE_USE_TEXTURE | ||
| int vertexOffset = gl_VertexID * renderer_BlendShapeTextureInfo.x; | ||
| for(int i = 0; i < RENDERER_BLENDSHAPE_COUNT; i++){ | ||
| int vertexElementOffset = vertexOffset; | ||
| float weight = renderer_BlendShapeWeights[i]; | ||
| // Warnning: Multiplying by 0 creates weird precision issues, causing rendering anomalies in Ace2 Android13 | ||
| if(weight != 0.0){ | ||
| position.xyz += getBlendShapeVertexElement(i, vertexElementOffset) * weight; | ||
|
|
||
| #if defined( RENDERER_HAS_NORMAL ) && defined( RENDERER_BLENDSHAPE_HAS_NORMAL ) | ||
| vertexElementOffset += 1; | ||
| normal += getBlendShapeVertexElement(i, vertexElementOffset) * weight; | ||
| #endif | ||
|
|
||
| #if defined( RENDERER_HAS_TANGENT ) && defined(RENDERER_BLENDSHAPE_HAS_TANGENT) | ||
| vertexElementOffset += 1; | ||
| tangent.xyz += getBlendShapeVertexElement(i, vertexElementOffset) * weight; | ||
| #endif | ||
| } | ||
|
|
||
| } | ||
| #else | ||
| position.xyz += attr.POSITION_BS0 * renderer_BlendShapeWeights[0]; | ||
| position.xyz += attr.POSITION_BS1 * renderer_BlendShapeWeights[1]; | ||
|
|
||
| #if defined( RENDERER_BLENDSHAPE_HAS_NORMAL ) && defined( RENDERER_BLENDSHAPE_HAS_TANGENT ) | ||
| #ifdef RENDERER_HAS_NORMAL | ||
| normal += attr.NORMAL_BS0 * renderer_BlendShapeWeights[0]; | ||
| normal += attr.NORMAL_BS1 * renderer_BlendShapeWeights[1]; | ||
| #endif | ||
|
|
||
| #ifdef RENDERER_HAS_TANGENT | ||
| tangent.xyz += attr.TANGENT_BS0 * renderer_BlendShapeWeights[0]; | ||
| tangent.xyz += attr.TANGENT_BS1 * renderer_BlendShapeWeights[1]; | ||
| #endif | ||
| #else | ||
| #if defined( RENDERER_BLENDSHAPE_HAS_NORMAL ) || defined( RENDERER_BLENDSHAPE_HAS_TANGENT ) | ||
| position.xyz += attr.POSITION_BS2 * renderer_BlendShapeWeights[2]; | ||
| position.xyz += attr.POSITION_BS3 * renderer_BlendShapeWeights[3]; | ||
|
|
||
| #if defined( RENDERER_BLENDSHAPE_HAS_NORMAL ) && defined( RENDERER_HAS_NORMAL ) | ||
| normal += attr.NORMAL_BS0 * renderer_BlendShapeWeights[0]; | ||
| normal += attr.NORMAL_BS1 * renderer_BlendShapeWeights[1]; | ||
| normal += attr.NORMAL_BS2 * renderer_BlendShapeWeights[2]; | ||
| normal += attr.NORMAL_BS3 * renderer_BlendShapeWeights[3]; | ||
| #endif | ||
|
|
||
| #if defined(RENDERER_BLENDSHAPE_HAS_TANGENT) && defined( RENDERER_HAS_TANGENT ) | ||
| tangent.xyz += attr.TANGENT_BS0 * renderer_BlendShapeWeights[0]; | ||
| tangent.xyz += attr.TANGENT_BS1 * renderer_BlendShapeWeights[1]; | ||
| tangent.xyz += attr.TANGENT_BS2 * renderer_BlendShapeWeights[2]; | ||
| tangent.xyz += attr.TANGENT_BS3 * renderer_BlendShapeWeights[3]; | ||
| #endif | ||
| #else | ||
| position.xyz += attr.POSITION_BS2 * renderer_BlendShapeWeights[2]; | ||
| position.xyz += attr.POSITION_BS3 * renderer_BlendShapeWeights[3]; | ||
| position.xyz += attr.POSITION_BS4 * renderer_BlendShapeWeights[4]; | ||
| position.xyz += attr.POSITION_BS5 * renderer_BlendShapeWeights[5]; | ||
| position.xyz += attr.POSITION_BS6 * renderer_BlendShapeWeights[6]; | ||
| position.xyz += attr.POSITION_BS7 * renderer_BlendShapeWeights[7]; | ||
| #endif | ||
| #endif | ||
| #endif | ||
| } | ||
|
|
||
| #endif | ||
|
|
||
|
|
||
| #endif |
There was a problem hiding this comment.
Thorough Analysis of BlendShape.glsl
The file effectively handles various configurations for blend shapes, allowing for a flexible shader setup that can adapt based on the renderer's capabilities. The use of conditional compilation is appropriate and well-implemented.
However, consider adding comments explaining the blend shape calculations and their expected effects, especially for complex conditional paths, to enhance maintainability and readability.
Would you like me to help document these complex sections for better clarity?
| mat4 getJointMatrix(sampler2D smp, float index){ | ||
| float base = index / renderer_JointCount; | ||
| float hf = 0.5 / renderer_JointCount; | ||
| float v = base + hf; | ||
|
|
||
| vec4 m0 = texture2D(smp, vec2(0.125, v )); | ||
| vec4 m1 = texture2D(smp, vec2(0.375, v )); | ||
| vec4 m2 = texture2D(smp, vec2(0.625, v )); | ||
| vec4 m3 = texture2D(smp, vec2(0.875, v )); | ||
|
|
||
| return mat4(m0, m1, m2, m3); | ||
| } |
There was a problem hiding this comment.
Review of getJointMatrix Function: Optimize Texture Sampling
The function getJointMatrix retrieves joint matrices using texture sampling. The use of hardcoded values like 0.125, 0.375, etc., for the s coordinate in texture2D calls might be specific to a layout expectation of the joint data in the texture. However, these values are magic numbers and should be defined as constants with descriptive names to improve readability and maintainability.
+ #define FIRST_COLUMN 0.125
+ #define SECOND_COLUMN 0.375
+ #define THIRD_COLUMN 0.625
+ #define FOURTH_COLUMN 0.875
- vec4 m0 = texture2D(smp, vec2(0.125, v));
- vec4 m1 = texture2D(smp, vec2(0.375, v));
- vec4 m2 = texture2D(smp, vec2(0.625, v));
- vec4 m3 = texture2D(smp, vec2(0.875, v));
+ vec4 m0 = texture2D(smp, vec2(FIRST_COLUMN, v));
+ vec4 m1 = texture2D(smp, vec2(SECOND_COLUMN, v));
+ vec4 m2 = texture2D(smp, vec2(THIRD_COLUMN, v));
+ vec4 m3 = texture2D(smp, vec2(FOURTH_column, v));Committable suggestion was skipped due to low confidence.
| void evaluateSpecularIBL(BRDFData brdfData, float specularAO, float radianceAttenuation, inout vec3 specularColor){ | ||
| vec3 radiance = getLightProbeRadiance(brdfData, brdfData.normal, brdfData.roughness); | ||
| specularColor += specularAO * radianceAttenuation * radiance * envBRDFApprox(brdfData.specularColor, brdfData.roughness, brdfData.dotNV ); | ||
| } |
There was a problem hiding this comment.
Optimize specular IBL calculations in evaluateSpecularIBL.
This function is critical for rendering specular highlights from image-based lighting. Consider optimizing the calculations for better performance, especially on platforms with limited GPU resources.
- specularColor += specularAO * radianceAttenuation * radiance * envBRDFApprox(brdfData.specularColor, brdfData.roughness, brdfData.dotNV );
+ // Optimize the calculation of specular highlights for better performance.Committable suggestion was skipped due to low confidence.
| vec3 getLightProbeRadiance(BRDFData brdfData, vec3 normal, float roughness) { | ||
|
|
||
| #ifndef SCENE_USE_SPECULAR_ENV | ||
| return vec3(0); | ||
| #else | ||
| vec3 reflectVec = getReflectedVector(brdfData, normal); | ||
| reflectVec.x = -reflectVec.x; // TextureCube is left-hand,so x need inverse | ||
|
|
||
| float specularMIPLevel = getSpecularMIPLevel(roughness, int(scene_EnvMapLight.mipMapLevel) ); | ||
|
|
||
| #ifdef HAS_TEX_LOD | ||
| vec4 envMapColor = textureCubeLodEXT( scene_EnvSpecularSampler, reflectVec, specularMIPLevel ); | ||
| #else | ||
| vec4 envMapColor = textureCube( scene_EnvSpecularSampler, reflectVec, specularMIPLevel ); | ||
| #endif | ||
|
|
||
| #ifdef SCENE_IS_DECODE_ENV_RGBM | ||
| envMapColor.rgb = (RGBMToLinear(envMapColor, 5.0)).rgb; | ||
| #ifdef ENGINE_IS_COLORSPACE_GAMMA | ||
| envMapColor = linearToGamma(envMapColor); | ||
| #endif | ||
| #else | ||
| #ifndef ENGINE_IS_COLORSPACE_GAMMA | ||
| envMapColor = gammaToLinear(envMapColor); | ||
| #endif | ||
| #endif | ||
|
|
||
| return envMapColor.rgb * scene_EnvMapLight.specularIntensity; | ||
|
|
||
| #endif | ||
|
|
||
| } |
There was a problem hiding this comment.
Optimize texture sampling in getLightProbeRadiance.
The function performs multiple conditional checks and texture sampling operations, which can be performance-intensive. Consider optimizing these operations, especially for platforms where texture sampling is a bottleneck.
- #ifdef HAS_TEX_LOD
- vec4 envMapColor = textureCubeLodEXT( scene_EnvSpecularSampler, reflectVec, specularMIPLevel );
- #else
- vec4 envMapColor = textureCube( scene_EnvSpecularSampler, reflectVec, specularMIPLevel );
- #endif
+ // Use a unified approach to texture sampling that minimizes conditional directives.Committable suggestion was skipped due to low confidence.
| vec3 getLightProbeIrradiance(vec3 sh[9], vec3 normal){ | ||
| normal.x = -normal.x; | ||
| vec3 result = sh[0] + | ||
|
|
||
| sh[1] * (normal.y) + | ||
| sh[2] * (normal.z) + | ||
| sh[3] * (normal.x) + | ||
|
|
||
| sh[4] * (normal.y * normal.x) + | ||
| sh[5] * (normal.y * normal.z) + | ||
| sh[6] * (3.0 * normal.z * normal.z - 1.0) + | ||
| sh[7] * (normal.z * normal.x) + | ||
| sh[8] * (normal.x * normal.x - normal.y * normal.y); | ||
|
|
||
| return max(result, vec3(0.0)); | ||
|
|
||
| } |
There was a problem hiding this comment.
Review the getLightProbeIrradiance function for potential optimizations.
This function computes irradiance using spherical harmonics. The manual flipping of the x component (line 29) and the complex arithmetic could be optimized or simplified for better performance, especially on mobile platforms.
- normal.x = -normal.x;
+ // Consider optimizing the computation of the result using precomputed values or more efficient formulas.Committable suggestion was skipped due to low confidence.
| vec3 getShadowCoord(vec3 positionWS) { | ||
| #if SCENE_SHADOW_CASCADED_COUNT == 1 | ||
| mediump int cascadeIndex = 0; | ||
| #else | ||
| mediump int cascadeIndex = computeCascadeIndex(positionWS); | ||
| #endif | ||
|
|
||
| #ifdef GRAPHICS_API_WEBGL2 | ||
| mat4 shadowMatrix = scene_ShadowMatrices[cascadeIndex]; | ||
| #else | ||
| mat4 shadowMatrix; | ||
| #if SCENE_SHADOW_CASCADED_COUNT == 4 | ||
| if (cascadeIndex == 0) { | ||
| shadowMatrix = scene_ShadowMatrices[0]; | ||
| } else if (cascadeIndex == 1) { | ||
| shadowMatrix = scene_ShadowMatrices[1]; | ||
| } else if (cascadeIndex == 2) { | ||
| shadowMatrix = scene_ShadowMatrices[2]; | ||
| } else if (cascadeIndex == 3) { | ||
| shadowMatrix = scene_ShadowMatrices[3]; | ||
| } else { | ||
| shadowMatrix = scene_ShadowMatrices[4]; | ||
| } | ||
| #endif | ||
| #if SCENE_SHADOW_CASCADED_COUNT == 2 | ||
| if (cascadeIndex == 0) { | ||
| shadowMatrix = scene_ShadowMatrices[0]; | ||
| } else if (cascadeIndex == 1) { | ||
| shadowMatrix = scene_ShadowMatrices[1]; | ||
| } else { | ||
| shadowMatrix = scene_ShadowMatrices[2]; | ||
| } | ||
| #endif | ||
| #if SCENE_SHADOW_CASCADED_COUNT == 1 | ||
| if (cascadeIndex == 0) { | ||
| shadowMatrix = scene_ShadowMatrices[0]; | ||
| } else { | ||
| shadowMatrix = scene_ShadowMatrices[1]; | ||
| } | ||
| #endif | ||
| #endif | ||
|
|
||
| vec4 shadowCoord = shadowMatrix * vec4(positionWS, 1.0); | ||
| return shadowCoord.xyz; | ||
| } |
There was a problem hiding this comment.
New Function: getShadowCoord
This function retrieves the shadow coordinates. It handles different counts of cascaded shadows and adjusts the shadow matrix accordingly. The use of conditional compilation for different graphics APIs and shadow counts is appropriate. However, there is potential for simplification in the handling of different SCENE_SHADOW_CASCADED_COUNT values.
- #if SCENE_SHADOW_CASCADED_COUNT == 4
- if (cascadeIndex == 0) {
- shadowMatrix = scene_ShadowMatrices[0];
- } else if (cascadeIndex == 1) {
- shadowMatrix = scene_ShadowMatrices[1];
- } else if (cascadeIndex == 2) {
- shadowMatrix = scene_ShadowMatrices[2];
- } else if (cascadeIndex == 3) {
- shadowMatrix = scene_ShadowMatrices[3];
- } else {
- shadowMatrix = scene_ShadowMatrices[4];
- }
- #endif
+ // Simplify the assignment of shadowMatrix for any SCENE_SHADOW_CASCADED_COUNT
+ shadowMatrix = scene_ShadowMatrices[min(cascadeIndex, SCENE_SHADOW_CASCADED_COUNT)];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.
| vec3 getShadowCoord(vec3 positionWS) { | |
| #if SCENE_SHADOW_CASCADED_COUNT == 1 | |
| mediump int cascadeIndex = 0; | |
| #else | |
| mediump int cascadeIndex = computeCascadeIndex(positionWS); | |
| #endif | |
| #ifdef GRAPHICS_API_WEBGL2 | |
| mat4 shadowMatrix = scene_ShadowMatrices[cascadeIndex]; | |
| #else | |
| mat4 shadowMatrix; | |
| #if SCENE_SHADOW_CASCADED_COUNT == 4 | |
| if (cascadeIndex == 0) { | |
| shadowMatrix = scene_ShadowMatrices[0]; | |
| } else if (cascadeIndex == 1) { | |
| shadowMatrix = scene_ShadowMatrices[1]; | |
| } else if (cascadeIndex == 2) { | |
| shadowMatrix = scene_ShadowMatrices[2]; | |
| } else if (cascadeIndex == 3) { | |
| shadowMatrix = scene_ShadowMatrices[3]; | |
| } else { | |
| shadowMatrix = scene_ShadowMatrices[4]; | |
| } | |
| #endif | |
| #if SCENE_SHADOW_CASCADED_COUNT == 2 | |
| if (cascadeIndex == 0) { | |
| shadowMatrix = scene_ShadowMatrices[0]; | |
| } else if (cascadeIndex == 1) { | |
| shadowMatrix = scene_ShadowMatrices[1]; | |
| } else { | |
| shadowMatrix = scene_ShadowMatrices[2]; | |
| } | |
| #endif | |
| #if SCENE_SHADOW_CASCADED_COUNT == 1 | |
| if (cascadeIndex == 0) { | |
| shadowMatrix = scene_ShadowMatrices[0]; | |
| } else { | |
| shadowMatrix = scene_ShadowMatrices[1]; | |
| } | |
| #endif | |
| #endif | |
| vec4 shadowCoord = shadowMatrix * vec4(positionWS, 1.0); | |
| return shadowCoord.xyz; | |
| } | |
| vec3 getShadowCoord(vec3 positionWS) { | |
| #if SCENE_SHADOW_CASCADED_COUNT == 1 | |
| mediump int cascadeIndex = 0; | |
| #else | |
| mediump int cascadeIndex = computeCascadeIndex(positionWS); | |
| #endif | |
| #ifdef GRAPHICS_API_WEBGL2 | |
| mat4 shadowMatrix = scene_ShadowMatrices[cascadeIndex]; | |
| #else | |
| mat4 shadowMatrix; | |
| // Simplify the assignment of shadowMatrix for any SCENE_SHADOW_CASCADED_COUNT | |
| shadowMatrix = scene_ShadowMatrices[min(cascadeIndex, SCENE_SHADOW_CASCADED_COUNT)]; | |
| #endif | |
| vec4 shadowCoord = shadowMatrix * vec4(positionWS, 1.0); | |
| return shadowCoord.xyz; | |
| } |
| void initSurfaceData(Varyings v, out SurfaceData surfaceData, bool isFrontFacing){ | ||
| // common | ||
| vec4 baseColor = material_BaseColor; | ||
| float metallic = material_Metal; | ||
| float roughness = material_Roughness; | ||
| vec3 specularColor = material_PBRSpecularColor; | ||
| float glossiness = material_Glossiness; | ||
| float f0 = pow2( (material_IOR - 1.0) / (material_IOR + 1.0) ); | ||
| vec3 emissiveRadiance = material_EmissiveColor; | ||
|
|
||
| #ifdef MATERIAL_HAS_BASETEXTURE | ||
| vec4 baseTextureColor = texture2D(material_BaseTexture, v.v_uv); | ||
| #ifndef ENGINE_IS_COLORSPACE_GAMMA | ||
| baseTextureColor = gammaToLinear(baseTextureColor); | ||
| #endif | ||
| baseColor *= baseTextureColor; | ||
| #endif | ||
|
|
||
| #ifdef RENDERER_ENABLE_VERTEXCOLOR | ||
| baseColor *= v.v_color; | ||
| #endif | ||
|
|
||
|
|
||
| #ifdef MATERIAL_IS_ALPHA_CUTOFF | ||
| if( baseColor.a < material_AlphaCutoff ) { | ||
| discard; | ||
| } | ||
| #endif | ||
|
|
||
| #ifdef MATERIAL_HAS_ROUGHNESS_METALLIC_TEXTURE | ||
| vec4 metalRoughMapColor = texture2D( material_RoughnessMetallicTexture, v.v_uv ); | ||
| roughness *= metalRoughMapColor.g; | ||
| metallic *= metalRoughMapColor.b; | ||
| #endif | ||
|
|
||
| #ifdef MATERIAL_HAS_SPECULAR_GLOSSINESS_TEXTURE | ||
| vec4 specularGlossinessColor = texture2D(material_SpecularGlossinessTexture, v.v_uv ); | ||
| #ifndef ENGINE_IS_COLORSPACE_GAMMA | ||
| specularGlossinessColor = gammaToLinear(specularGlossinessColor); | ||
| #endif | ||
| specularColor *= specularGlossinessColor.rgb; | ||
| glossiness *= specularGlossinessColor.a; | ||
| roughness = 1.0 - glossiness; | ||
| #endif | ||
|
|
||
| #ifdef MATERIAL_HAS_EMISSIVETEXTURE | ||
| vec4 emissiveColor = texture2D(material_EmissiveTexture, v.v_uv); | ||
| #ifndef ENGINE_IS_COLORSPACE_GAMMA | ||
| emissiveColor = gammaToLinear(emissiveColor); | ||
| #endif | ||
| emissiveRadiance *= emissiveColor.rgb; | ||
| #endif | ||
|
|
||
| surfaceData.albedoColor = baseColor.rgb; | ||
| surfaceData.specularColor = specularColor; | ||
| surfaceData.emissiveColor = emissiveRadiance; | ||
| surfaceData.metallic = metallic; | ||
| surfaceData.roughness = roughness; | ||
| surfaceData.f0 = f0; | ||
|
|
||
| #ifdef MATERIAL_IS_TRANSPARENT | ||
| surfaceData.opacity = baseColor.a; | ||
| #else | ||
| surfaceData.opacity = 1.0; | ||
| #endif | ||
|
|
||
|
|
||
| // geometry | ||
| surfaceData.position = v.v_pos; | ||
|
|
||
| #ifdef CAMERA_ORTHOGRAPHIC | ||
| surfaceData.viewDir = -camera_Forward; | ||
| #else | ||
| surfaceData.viewDir = normalize(camera_Position - v.v_pos); | ||
| #endif | ||
|
|
||
| #ifdef NEED_TANGENT | ||
| mat3 tbn = getTBN(v, isFrontFacing); | ||
| surfaceData.tangent = tbn[0]; | ||
| surfaceData.bitangent = tbn[1]; | ||
| #ifdef MATERIAL_HAS_NORMALTEXTURE | ||
| surfaceData.normal = getNormalByNormalTexture(tbn, material_NormalTexture, material_NormalIntensity, v.v_uv, isFrontFacing); | ||
| #else | ||
| surfaceData.normal = tbn[2]; | ||
| #endif | ||
| #else | ||
| surfaceData.normal = getNormal(v, isFrontFacing); | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Function: initSurfaceData
This function initializes SurfaceData from varying inputs and material properties. It handles textures and conditional compilation effectively. However, the function is quite long and handles multiple concerns.
- void initSurfaceData(Varyings v, out SurfaceData surfaceData, bool isFrontFacing){
+ void initSurfaceData(Varyings v, out SurfaceData surfaceData, bool isFrontFacing) {
+ initBaseColor(v, surfaceData);
+ initMetallicRoughness(v, surfaceData);
+ initEmissiveColor(v, surfaceData);
+ // Additional initialization functions can be defined here
+ }Splitting this function into smaller, more focused functions would improve readability and maintainability.
Committable suggestion was skipped due to low confidence.
| #endif | ||
| #endif | ||
|
|
||
| void calculateBlendShape(Attributes attr, inout vec4 position |
|
|
||
|
|
||
| vec4 material_TilingOffset; | ||
| void initUV(Attributes attr, inout Varyings v){ |
| #ifndef ATTRIBUTES_PBR_INCLUDED | ||
| #define ATTRIBUTES_PBR_INCLUDED | ||
|
|
||
| struct Attributes{ |
There was a problem hiding this comment.
Define in forwardPass is more clearly
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
packages/shaderlab/src/shaders/Vertex.glsl (2)
4-10: Add documentation formaterial_TilingOffset.The variable
material_TilingOffsetis used in the shader but lacks documentation. It's important to describe its purpose and usage to maintain code clarity and assist other developers.
11-18: Clarify UV transformation logic.The function
getUV0performs UV transformations based on renderer capabilities. Adding comments within the function to explain the transformation logic, especially howmaterial_TilingOffsetaffects the UV coordinates, would enhance readability and maintainability.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- packages/shaderlab/src/shaders/BlendShape.glsl (1 hunks)
- packages/shaderlab/src/shaders/Normal.glsl (3 hunks)
- packages/shaderlab/src/shaders/Skin.glsl (1 hunks)
- packages/shaderlab/src/shaders/Vertex.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/VaryingsPBR.glsl (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- packages/shaderlab/src/shaders/BlendShape.glsl
- packages/shaderlab/src/shaders/Normal.glsl
- packages/shaderlab/src/shaders/Skin.glsl
- packages/shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glsl
- packages/shaderlab/src/shaders/shadingPBR/VaryingsPBR.glsl
| void initTransform(Attributes attributes, inout Varyings varyings){ | ||
| vec4 position = vec4( attributes.POSITION , 1.0 ); | ||
|
|
||
| #ifdef RENDERER_HAS_NORMAL | ||
| vec3 normal = vec3( attributes.NORMAL ); | ||
| #endif | ||
|
|
||
| #ifdef RENDERER_HAS_TANGENT | ||
| vec4 tangent = vec4( attributes.TANGENT ); | ||
| #endif | ||
|
|
||
|
|
||
| // blendShape | ||
| #ifdef RENDERER_HAS_BLENDSHAPE | ||
| calculateBlendShape(attributes, position | ||
| #ifdef RENDERER_HAS_NORMAL | ||
| ,normal | ||
| #endif | ||
| #ifdef RENDERER_HAS_TANGENT | ||
| ,tangent | ||
| #endif | ||
| ); | ||
| #endif | ||
|
|
||
| // skin | ||
| #ifdef RENDERER_HAS_SKIN | ||
| mat4 skinMatrix = getSkinMatrix(attributes); | ||
| position = skinMatrix * position; | ||
|
|
||
| #if defined(RENDERER_HAS_NORMAL) | ||
| mat3 skinNormalMatrix = INVERSE_MAT(mat3(skinMatrix)); | ||
| normal = normal * skinNormalMatrix; | ||
| #ifdef RENDERER_HAS_TANGENT | ||
| tangent.xyz = tangent.xyz * skinNormalMatrix; | ||
| #endif | ||
| #endif | ||
| #endif | ||
|
|
||
|
|
||
| // fog | ||
| #if SCENE_FOG_MODE != 0 | ||
| vec4 positionVS = renderer_MVMat * position; | ||
| varyings.v_positionVS = positionVS.xyz / positionVS.w; | ||
| #endif | ||
|
|
||
|
|
||
| // normal and tangent | ||
| #ifdef RENDERER_HAS_NORMAL | ||
| varyings.v_normal = normalize( mat3(renderer_NormalMat) * normal ); | ||
|
|
||
| #ifdef RENDERER_HAS_TANGENT | ||
| vec3 tangentW = normalize( mat3(renderer_NormalMat) * tangent.xyz ); | ||
| vec3 bitangentW = cross( varyings.v_normal, tangentW ) * tangent.w; | ||
|
|
||
| varyings.v_tangent = tangentW; | ||
| varyings.v_bitangent = bitangentW; | ||
| #endif | ||
| #endif | ||
|
|
||
|
|
||
| // worldpos_vert | ||
| vec4 temp_pos = renderer_ModelMat * position; | ||
| varyings.v_pos = temp_pos.xyz / temp_pos.w; | ||
|
|
||
|
|
||
| // position_vert | ||
| gl_Position = renderer_MVPMat * position; | ||
| } |
There was a problem hiding this comment.
Improve documentation and consider refactoring initTransform.
The initTransform function is central to applying various geometric transformations but is complex due to multiple conditional compilations and transformation steps. Improving documentation, especially explaining the sequence of transformations (blend shapes, skinning, fog calculations, etc.), would greatly benefit understanding and maintenance. Consider refactoring to separate concerns, possibly by breaking down into smaller, more focused functions.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- packages/shaderlab/src/shaders/BlendShape.glsl (1 hunks)
- packages/shaderlab/src/shaders/Shadow.glsl (4 hunks)
- packages/shaderlab/src/shaders/Vertex.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/VaryingsPBR.glsl (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- packages/shaderlab/src/shaders/BlendShape.glsl
- packages/shaderlab/src/shaders/Shadow.glsl
- packages/shaderlab/src/shaders/Vertex.glsl
- packages/shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glsl
- packages/shaderlab/src/shaders/shadingPBR/VaryingsPBR.glsl
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- packages/shaderlab/src/shaders/Macros.glsl (1 hunks)
- packages/shaderlab/src/shaders/Shadow.glsl (4 hunks)
- packages/shaderlab/src/shaders/index.ts (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/AttributesPBR.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/MaterialInputPBR.glsl (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/shaderlab/src/shaders/Macros.glsl
Files skipped from review as they are similar to previous changes (5)
- packages/shaderlab/src/shaders/Shadow.glsl
- packages/shaderlab/src/shaders/index.ts
- packages/shaderlab/src/shaders/shadingPBR/AttributesPBR.glsl
- packages/shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glsl
- packages/shaderlab/src/shaders/shadingPBR/MaterialInputPBR.glsl
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/shaderlab/src/shaders/Light.glsl (6 hunks)
- packages/shaderlab/src/shaders/Vertex.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/LightDirectPBR.glsl (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- packages/shaderlab/src/shaders/Light.glsl
- packages/shaderlab/src/shaders/Vertex.glsl
- packages/shaderlab/src/shaders/shadingPBR/LightDirectPBR.glsl
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- packages/shaderlab/src/shaders/Macros.glsl (1 hunks)
- packages/shaderlab/src/shaders/Shadow.glsl (4 hunks)
- packages/shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/LightDirectPBR.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/VaryingsPBR.glsl (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- packages/shaderlab/src/shaders/Macros.glsl
- packages/shaderlab/src/shaders/Shadow.glsl
- packages/shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glsl
- packages/shaderlab/src/shaders/shadingPBR/LightDirectPBR.glsl
- packages/shaderlab/src/shaders/shadingPBR/VaryingsPBR.glsl
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (4)
packages/shaderlab/src/shaders/shadingPBR/VertexPBR.glsl (2)
10-25: Approved struct definition with a suggestion for documentation.The struct
VertexInputsis well-defined, utilizing preprocessor directives to conditionally include attributes. This approach optimizes shader performance by including only necessary data.Consider adding comments to describe each condition and its relevance to the rendering context, which would improve the maintainability and understandability of the shader code.
28-35: Approved UV calculation with a suggestion for robustness.The function
getUV0correctly calculates UV coordinates based on the renderer's capabilities. This conditional approach is efficient but could be made more robust.Consider adding a default case or error handling for configurations where
RENDERER_HAS_UVis not defined, to prevent potential issues with uninitialized UV coordinates.packages/shaderlab/src/shaders/shadingPBR/FragmentPBR.glsl (2)
6-67: Approved material property definitions with a suggestion for documentation.The definitions of material properties such as
material_BaseColor,material_Metal, and various texture samplers are well-organized and crucial for the shader's functionality.However, adding comments to explain the purpose of each property and the conditions under which textures are sampled would greatly enhance the readability and maintainability of the code.
69-160: Review of complex surface data calculation function.The function
getSurfaceDataefficiently calculates surface data based on material properties and varyings. Each conditional path and texture sampling operation appears to be handled correctly.Given the complexity of this function, consider adding detailed comments to explain the logic behind each conditional path and calculation, which would aid in future maintenance and modifications.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- packages/shaderlab/src/shaders/Shadow.glsl (4 hunks)
- packages/shaderlab/src/shaders/index.ts (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/BRDF.glsl (4 hunks)
- packages/shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/FragmentPBR.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/LightDirectPBR.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/LightIndirectPBR.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/VertexPBR.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/index.ts (1 hunks)
Files skipped from review as they are similar to previous changes (7)
- packages/shaderlab/src/shaders/Shadow.glsl
- packages/shaderlab/src/shaders/index.ts
- packages/shaderlab/src/shaders/shadingPBR/BRDF.glsl
- packages/shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glsl
- packages/shaderlab/src/shaders/shadingPBR/LightDirectPBR.glsl
- packages/shaderlab/src/shaders/shadingPBR/LightIndirectPBR.glsl
- packages/shaderlab/src/shaders/shadingPBR/index.ts
Additional comments not posted (3)
packages/shaderlab/src/shaders/shadingPBR/VertexPBR.glsl (2)
4-7: Approved shader component inclusions.The inclusion of
Transform.glsl,Skin.glsl,BlendShape.glsl, andShadow.glslensures that the vertex shader has access to necessary transformations and operations. This setup is crucial for the functionality of the shader.
1-3: Approved use of preprocessor guards.The use of
#ifndef VERTEX_INCLUDEand#define VERTEX_INCLUDEat the beginning of the file is a standard practice to prevent multiple inclusions of the same file, which is crucial in shader programming to avoid redefinition errors.packages/shaderlab/src/shaders/shadingPBR/FragmentPBR.glsl (1)
1-3: Approved use of preprocessor guards.The use of
#ifndef MATERIAL_INPUT_PBR_INCLUDEDand#define MATERIAL_INPUT_PBR_INCLUDEDat the beginning of the file is a standard practice to prevent multiple inclusions of the same file, which is crucial in shader programming to avoid redefinition errors.
| VertexInputs getVertexInputs(Attributes attributes){ | ||
| VertexInputs inputs; | ||
| vec4 position = vec4(attributes.POSITION, 1.0); | ||
|
|
||
| #ifdef RENDERER_HAS_NORMAL | ||
| vec3 normal = vec3( attributes.NORMAL ); | ||
| #ifdef RENDERER_HAS_TANGENT | ||
| vec4 tangent = vec4( attributes.TANGENT ); | ||
| #endif | ||
| #endif | ||
|
|
||
|
|
||
| // BlendShape | ||
| #ifdef RENDERER_HAS_BLENDSHAPE | ||
| calculateBlendShape(attributes, position | ||
| #ifdef RENDERER_HAS_NORMAL | ||
| ,normal | ||
| #ifdef RENDERER_HAS_TANGENT | ||
| ,tangent | ||
| #endif | ||
| #endif | ||
| ); | ||
| #endif | ||
|
|
||
| // Skin | ||
| #ifdef RENDERER_HAS_SKIN | ||
| mat4 skinMatrix = getSkinMatrix(attributes); | ||
| position = skinMatrix * position; | ||
|
|
||
| #if defined(RENDERER_HAS_NORMAL) | ||
| mat3 skinNormalMatrix = INVERSE_MAT(mat3(skinMatrix)); | ||
| normal = normal * skinNormalMatrix; | ||
| #ifdef RENDERER_HAS_TANGENT | ||
| tangent.xyz = tangent.xyz * skinNormalMatrix; | ||
| #endif | ||
| #endif | ||
| #endif | ||
|
|
||
| // TBN world space | ||
| #ifdef RENDERER_HAS_NORMAL | ||
| inputs.normalWS = normalize( mat3(renderer_NormalMat) * normal ); | ||
|
|
||
| #ifdef RENDERER_HAS_TANGENT | ||
| vec3 tangentWS = normalize( mat3(renderer_NormalMat) * tangent.xyz ); | ||
| vec3 bitangentWS = cross( inputs.normalWS, tangentWS ) * tangent.w; | ||
|
|
||
| inputs.tangentWS = tangentWS; | ||
| inputs.bitangentWS = bitangentWS; | ||
| #endif | ||
| #endif | ||
|
|
||
|
|
||
| inputs.positionOS = position; | ||
| vec4 positionWS = renderer_ModelMat * position; | ||
| inputs.positionWS = positionWS.xyz / positionWS.w; | ||
|
|
||
| #if SCENE_FOG_MODE != 0 | ||
| vec4 positionVS = renderer_MVMat * position; | ||
| inputs.positionVS = positionVS.xyz / positionVS.w; | ||
| #endif | ||
|
|
||
| return inputs; |
There was a problem hiding this comment.
Review of complex vertex input function.
The function getVertexInputs is complex and handles multiple transformations based on the rendering context. Each transformation appears to be applied correctly.
However, the complexity of this function could impact maintainability. Consider refactoring to improve clarity, possibly by breaking it down into smaller, more manageable functions.
| brdfData.viewDir = surfaceData.viewDir; | ||
|
|
||
| brdfData.dotNV = saturate( dot(brdfData.normal, brdfData.viewDir) ); | ||
| } | ||
|
|
||
| void initCommonBRDFData(SurfaceData surfaceData, inout BRDFData brdfData){ | ||
| vec3 albedoColor = surfaceData.albedoColor; | ||
| vec3 specularColor = surfaceData.specularColor; | ||
| float metallic = surfaceData.metallic; | ||
| float roughness = surfaceData.roughness; | ||
| float f0 = surfaceData.f0; | ||
|
|
||
| #ifdef IS_METALLIC_WORKFLOW | ||
| brdfData.diffuseColor = albedoColor * ( 1.0 - metallic ); | ||
| brdfData.specularColor = mix( vec3(f0), albedoColor, metallic ); | ||
| #else | ||
| float specularStrength = max( max( specularColor.r, specularColor.g ), specularColor.b ); | ||
| brdfData.diffuseColor = albedoColor * ( 1.0 - specularStrength ); | ||
| brdfData.specularColor = specularColor; | ||
| #endif | ||
|
|
||
| brdfData.roughness = max(MIN_PERCEPTUAL_ROUGHNESS, min(roughness + getAARoughnessFactor(brdfData.normal), 1.0)); | ||
| } | ||
|
|
||
| void initClearCoatBRDFData(Varyings v, inout BRDFData brdfData, bool isFrontFacing){ | ||
| #ifdef MATERIAL_ENABLE_CLEAR_COAT | ||
| #ifdef MATERIAL_HAS_CLEAR_COAT_NORMAL_TEXTURE | ||
| brdfData.clearCoatNormal = getNormalByNormalTexture(mat3(brdfData.tangent, brdfData.bitangent, brdfData.normal), material_ClearCoatNormalTexture, material_NormalIntensity, v.v_uv, isFrontFacing); | ||
| #else | ||
| brdfData.clearCoatNormal = getNormal(v, isFrontFacing); | ||
| #endif | ||
| brdfData.clearCoatDotNV = saturate( dot(brdfData.clearCoatNormal, brdfData.viewDir) ); | ||
|
|
||
| brdfData.clearCoat = material_ClearCoat; | ||
| brdfData.clearCoatRoughness = material_ClearCoatRoughness; | ||
|
|
||
| #ifdef MATERIAL_HAS_CLEAR_COAT_TEXTURE | ||
| brdfData.clearCoat *= (texture2D( material_ClearCoatTexture, v.v_uv )).r; | ||
| #endif | ||
|
|
||
| #ifdef MATERIAL_HAS_CLEAR_COAT_ROUGHNESS_TEXTURE | ||
| brdfData.clearCoatRoughness *= (texture2D( material_ClearCoatRoughnessTexture, v.v_uv )).g; | ||
| #endif | ||
|
|
||
| brdfData.clearCoat = saturate( brdfData.clearCoat ); | ||
|
|
||
| brdfData.clearCoatRoughness = max(MIN_PERCEPTUAL_ROUGHNESS, min(brdfData.clearCoatRoughness + getAARoughnessFactor(brdfData.clearCoatNormal), 1.0)); | ||
|
|
||
| #endif | ||
|
|
||
| } | ||
|
|
||
| void initAnisotropyBRDFData(Varyings v, inout BRDFData brdfData){ | ||
| #ifdef MATERIAL_ENABLE_ANISOTROPY | ||
| float anisotropy = material_AnisotropyInfo.z; | ||
| vec3 anisotropicDirection = vec3(material_AnisotropyInfo.xy, 0.0); | ||
| #ifdef MATERIAL_HAS_ANISOTROPY_TEXTURE | ||
| vec3 anisotropyTextureInfo = (texture2D( material_AnisotropyTexture, v.v_uv )).rgb; | ||
| anisotropy *= anisotropyTextureInfo.b; | ||
| anisotropicDirection.xy *= anisotropyTextureInfo.rg * 2.0 - 1.0; | ||
| #endif | ||
|
|
||
| brdfData.anisotropy = anisotropy; | ||
| brdfData.anisotropicT = normalize(mat3(brdfData.tangent, brdfData.bitangent, brdfData.normal) * anisotropicDirection); | ||
| brdfData.anisotropicB = normalize(cross(brdfData.normal, brdfData.anisotropicT)); | ||
| brdfData.anisotropicN = getAnisotropicBentNormal(brdfData); | ||
|
|
||
| #endif | ||
|
|
||
| } | ||
|
|
||
| void initAO(Varyings v, inout BRDFData brdfData){ | ||
| float diffuseAO = 1.0; | ||
| float specularAO = 1.0; | ||
|
|
||
| #ifdef MATERIAL_HAS_OCCLUSION_TEXTURE | ||
| vec2 aoUV = v.v_uv; | ||
| #ifdef RENDERER_HAS_UV1 | ||
| if(material_OcclusionTextureCoord == 1.0){ | ||
| aoUV = v.v_uv1; | ||
| } | ||
| #endif | ||
| diffuseAO = ((texture2D(material_OcclusionTexture, aoUV)).r - 1.0) * material_OcclusionIntensity + 1.0; | ||
| #endif | ||
|
|
||
| #if defined(MATERIAL_HAS_OCCLUSION_TEXTURE) && defined(SCENE_USE_SPECULAR_ENV) | ||
| specularAO = saturate( pow( brdfData.dotNV + diffuseAO, exp2( - 16.0 * brdfData.roughness - 1.0 ) ) - 1.0 + diffuseAO ); | ||
| #endif | ||
|
|
||
| brdfData.diffuseAO = diffuseAO; | ||
| brdfData.specularAO = specularAO; | ||
| } | ||
|
|
||
| void initBRDFData(Varyings v, SurfaceData surfaceData, out BRDFData brdfData, bool isFrontFacing){ | ||
| initGeometryData(surfaceData, brdfData); | ||
| initCommonBRDFData(surfaceData, brdfData); | ||
| initClearCoatBRDFData(v, brdfData, isFrontFacing); | ||
| initAnisotropyBRDFData(v, brdfData); | ||
| initAO(v, brdfData); | ||
| } |
There was a problem hiding this comment.
Review of BRDF data initialization functions.
The functions initGeometryData, initCommonBRDFData, initClearCoatBRDFData, initAnisotropyBRDFData, and initAO are well-designed and handle various aspects of BRDF data initialization effectively.
These functions are complex and handle a lot of conditional logic. To improve maintainability, consider refactoring these functions into smaller, more focused functions that handle specific aspects of the BRDF data.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- packages/shaderlab/src/shaders/Normal.glsl (3 hunks)
- packages/shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/FragmentPBR.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/VaryingsPBR.glsl (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- packages/shaderlab/src/shaders/Normal.glsl
- packages/shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glsl
- packages/shaderlab/src/shaders/shadingPBR/FragmentPBR.glsl
- packages/shaderlab/src/shaders/shadingPBR/VaryingsPBR.glsl
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- packages/shaderlab/src/shaders/Normal.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/BRDF.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glsl (1 hunks)
- packages/shaderlab/src/shaders/shadingPBR/FragmentPBR.glsl (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- packages/shaderlab/src/shaders/Normal.glsl
- packages/shaderlab/src/shaders/shadingPBR/BRDF.glsl
- packages/shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glsl
- packages/shaderlab/src/shaders/shadingPBR/FragmentPBR.glsl
GuoLei1990
changed the title
Please check if the PR fulfills these requirements
refs to: https://yuque.antfin-inc.com/shensi.zxd/ftoum6/gd91in8ldb1u1ba6?inner=Gpptb
shadingThinrefs to: https://yuque.antfin-inc.com/shensi.zxd/ftoum6/gd91in8ldb1u1ba6#nwOdKWhat 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
Refactor