Path in #include directive support relative path against to shader's path AND fix code generation error when target language is gles 100
#2241
Conversation
…t/shaderlab-relative-include
…t/shaderlab-relative-include
…t/shaderlab-relative-include
|
Warning Rate limit exceeded@Sway007 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 38 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe updates introduce enhanced shader path handling and configurability. Changes span across several files, primarily in shader-related classes, interfaces, and methods. Key modifications include additional parameters for path management in constructors and methods, adjustments to shader compilation processes, and improved path resolution for shader includes. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
#include directive support relative path against to shader's path#include directive support relative path against to shader's path AND fix code generation error when target language is gles 100
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
packages/design/src/shader-lab/IShaderLab.ts (1)
17-18: Ensure all calls to_parseShaderPassinclude the new parameters.The addition of
pathOriginandbasePathForIncludeKeyparameters enhances the functionality of_parseShaderPass. However, the following instances need to be updated to include these new parameters:
tests/src/shader-lab/ShaderValidate.tspackages/design/src/shader-lab/IShaderLab.tspackages/core/src/shader/ShaderPass.tspackages/shader-lab/src/ShaderLab.tsPlease update these calls to ensure compatibility with the new parameters.
Analysis chain
LGTM! Ensure compatibility with the new parameters.
The addition of
pathOriginandbasePathForIncludeKeyparameters to_parseShaderPassenhances the functionality by allowing relative paths in shader includes.Verify that all calls to
_parseShaderPassin the codebase are updated to include these new parameters.Also applies to: 27-28
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `_parseShaderPass` include the new parameters. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type typescript -A 5 $'_parseShaderPass'Length of output: 79
Script:
#!/bin/bash # Description: Verify all calls to `_parseShaderPass` include the new parameters. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type ts -A 5 _parseShaderPassLength of output: 2633
packages/core/src/shader/Shader.ts (1)
47-47: Some usages ofShader.createdo not provide the newpathparameter.The following instances of
Shader.createneed to be updated to include thepathparameter:
examples/buffer-mesh-particle-shader-effect.tsexamples/outline-multi-pass.tsexamples/sprite-material-dissolve.tsexamples/video-transparent.tsexamples/video-background.tsexamples/transparent-shadow.tsexamples/shader-water.tsexamples/shader-lab.tsexamples/text-ktv-subtitle.tsexamples/shader-lab-multi-pass.tsexamples/shader-replacement.tsexamples/sprite-material-blur.tsexamples/mrt.tsexamples/model-mesh.tsexamples/sprite-material-glitch-rgbSplit.tsexamples/shader-lab-simple.tsexamples/material-head-distort.tsexamples/ibl-baker.tsexamples/camera-depth-texture.tsexamples/cascaded-shadow.tsexamples/buffer-mesh-instance.tstests/src/core/Shader.test.tstests/src/core/Camera.test.tspackages/core/src/postProcess/index.tspackages/core/src/postProcess/effects/BloomEffect.tspackages/core/src/trail/TrailMaterial.tspackages/core/src/shader/ShaderPool.tstests/src/shader-lab/ShaderLab.test.tse2e/case/material-shaderLab.tse2e/case/material-shaderReplacement.tse2e/case/camera-opaque-texture.tsPlease update these instances to ensure the
pathparameter is provided where necessary.Analysis chain
Verify the usage of the new
pathparameter.Ensure that all usages of the
Shader.createmethod provide the newpathparameter when needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `Shader.create` method provide the new `path` parameter when needed. # Test: Search for the method usage. Expect: Only occurrences with the new parameter. rg --type ts -A 5 $'Shader.create'Length of output: 15969
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- packages/core/src/shader/Shader.ts (4 hunks)
- packages/core/src/shader/ShaderPass.ts (7 hunks)
- packages/design/src/shader-lab/IShaderLab.ts (1 hunks)
- packages/shader-lab/src/ShaderLab.ts (2 hunks)
- packages/shader-lab/src/codeGen/GLES100.ts (1 hunks)
- packages/shader-lab/src/codeGen/GLES300.ts (1 hunks)
- packages/shader-lab/src/codeGen/GLESVisitor.ts (3 hunks)
- packages/shader-lab/src/preprocessor/PpParser.ts (2 hunks)
- packages/shader-lab/src/preprocessor/Preprocessor.ts (1 hunks)
- tests/src/shader-lab/ShaderValidate.ts (1 hunks)
- tests/src/shader-lab/shaders/unlit.shader (1 hunks)
Additional context used
Biome
packages/shader-lab/src/preprocessor/PpParser.ts
[error] 35-35: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 36-36: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 37-37: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 38-38: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 39-39: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 40-40: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 41-41: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 42-42: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Additional comments not posted (19)
packages/shader-lab/src/preprocessor/Preprocessor.ts (1)
10-12: LGTM! Ensure compatibility with the new parameters.The addition of
pathOriginandbasePathForIncludeKeyparameters toresetenhances the functionality by allowing relative paths in shader includes.Verify that all calls to
resetin the codebase are updated to include these new parameters.Also applies to: 13-14
packages/shader-lab/src/codeGen/GLES100.ts (1)
7-15: LGTM!The addition of
_versionTextand_extensionsproperties toGLES100Visitorenhances the functionality by defining the GLSL version and enabling specific GL extensions. The implementation is clear and follows best practices.tests/src/shader-lab/ShaderValidate.ts (1)
89-91: LGTM!The addition of
shaderPathandfragmentPathparameters toglslValidateenhances the functionality by allowing the specification of paths for the shader and fragment files. The implementation is clear and follows best practices.packages/shader-lab/src/codeGen/GLES300.ts (1)
14-14: LGTM! Encapsulation of version text.The change encapsulates the
versionTextproperty by overriding it with_versionText, which is a good practice.packages/shader-lab/src/ShaderLab.ts (3)
56-58: Addition of new parameters to_parseShaderPass.The new parameters
pathOriginandbasePathForIncludeKeyhave been added to the_parseShaderPassmethod. Ensure these parameters are correctly passed and used in the method.
61-61: Update toPreprocessor.resetmethod call.The
Preprocessor.resetmethod call has been updated to include the new parameterspathOriginandbasePathForIncludeKey. Ensure that thePreprocessorclass handles these parameters correctly.
123-125: Addition of new parameters to_parsemethod.The new parameters
pathOriginandbasePathForIncludeKeyhave been added to the_parsemethod. Ensure these parameters are correctly passed and used in the method.tests/src/shader-lab/shaders/unlit.shader (1)
57-57: LGTM! Support for relative paths in#includedirective.The change updates the
#includedirective to use a relative path, enhancing flexibility.packages/shader-lab/src/codeGen/GLESVisitor.ts (3)
14-15: Addition of_versionTextand_extensionsproperties.The new protected properties
_versionTextand_extensionshave been added to theGLESVisitorclass. These properties encapsulate the version text and extensions, respectively.
85-85: Update toreturnstatement invertexMainmethod.The
returnstatement in thevertexMainmethod now includes_versionText, encapsulating the version text.
105-105: Update toreturnstatement in_fragmentMainmethod.The
returnstatement in the_fragmentMainmethod now includes_versionTextand_extensions, encapsulating the version text and extensions.packages/core/src/shader/ShaderPass.ts (4)
Line range hint
164-192: Ensure platform macros are correctly handled.The platform macros are being reset and set based on hardware capabilities. This looks good, but ensure that the macros are correctly handled in all cases.
Line range hint
108-123: LGTM!The
_getShaderProgrammethod handles shader program retrieval and compilation correctly.
Line range hint
164-192: LGTM!The
_compileShaderProgrammethod handles shader program compilation correctly.
43-43: Verify the usage of the newpathparameter.Ensure that all usages of the
ShaderPassconstructor provide the newpathparameter.packages/core/src/shader/Shader.ts (2)
88-92: Ensure all cases for shader creation are correctly handled.The updated logic handles cases where
fragmentSourceis not provided. Ensure that all cases for shader creation are correctly handled.
152-162: Ensure all cases for shader creation are correctly handled.The updated logic handles cases where
vertexSourceOrShaderPassesOrSubShadersOrPathis a string. Ensure that all cases for shader creation are correctly handled.packages/shader-lab/src/preprocessor/PpParser.ts (2)
34-42: Ensure the new parameters are correctly handled.The
resetmethod has been updated to include new parameterspathOriginandbasePathForIncludeKey. Ensure that these parameters are correctly handled in all cases.Tools
Biome
[error] 35-35: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 36-36: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 37-37: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 38-38: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 39-39: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 40-40: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 41-41: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 42-42: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
101-108: Ensure the new parameters are correctly handled.The
_parseIncludemethod has been updated to handle the newbasePathForIncludeKeyandpathOriginparameters. Ensure that these parameters are correctly handled in all cases.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/shader-lab/src/preprocessor/PpParser.ts (2 hunks)
Additional context used
Biome
packages/shader-lab/src/preprocessor/PpParser.ts
[error] 35-35: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 36-36: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 37-37: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 38-38: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 39-39: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 40-40: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 41-41: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 42-42: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Additional comments not posted (2)
packages/shader-lab/src/preprocessor/PpParser.ts (2)
31-32: New static variables added for path handling.The static variables
_basePathForIncludeKeyand_pathOriginhave been added to handle the base path and origin for include directives.
101-108: Updated_parseIncludemethod to handle relative paths.The
_parseIncludemethod now correctly handles relative paths using the_basePathForIncludeKeyand_pathOriginvariables. Ensure that the path handling logic is thoroughly tested.
| const chunk = this._includeMap[includedPath]; | ||
| if (!chunk) { | ||
| ParserUtils.throw(id.location, `Shader slice "${id.lexeme}" not founded.`); | ||
| ParserUtils.throw(id.location, `Shader slice "${includedPath}" not founded.`); |
There was a problem hiding this comment.
Fix grammatical issue in error message.
The error message has a minor grammatical issue. Change "not founded" to "not found".
- ParserUtils.throw(id.location, `Shader slice "${includedPath}" not founded.`);
+ ParserUtils.throw(id.location, `Shader slice "${includedPath}" not found.`);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.
| const chunk = this._includeMap[includedPath]; | |
| if (!chunk) { | |
| ParserUtils.throw(id.location, `Shader slice "${id.lexeme}" not founded.`); | |
| ParserUtils.throw(id.location, `Shader slice "${includedPath}" not founded.`); | |
| const chunk = this._includeMap[includedPath]; | |
| if (!chunk) { | |
| ParserUtils.throw(id.location, `Shader slice "${includedPath}" not found.`); |
| static reset(includeMap: Record<string, string>, pathOrigin: string, basePathForIncludeKey: string) { | ||
| this._definedMacros.clear(); | ||
| this._expandSegmentsStack.length = 0; | ||
| this._expandSegmentsStack.push([]); | ||
| this._branchMacros.clear(); | ||
| this.addPredefinedMacro("GL_ES"); | ||
| this._includeMap = includeMap; | ||
| this._basePathForIncludeKey = basePathForIncludeKey; | ||
| this._pathOrigin = pathOrigin; |
There was a problem hiding this comment.
Replace this with the class name in static context.
Using this in a static context can be confusing. Replace this with the class name PpParser.
- this._definedMacros.clear();
- this._expandSegmentsStack.length = 0;
- this._expandSegmentsStack.push([]);
- this._branchMacros.clear();
- this.addPredefinedMacro("GL_ES");
- this._includeMap = includeMap;
- this._basePathForIncludeKey = basePathForIncludeKey;
- this._pathOrigin = pathOrigin;
+ PpParser._definedMacros.clear();
+ PpParser._expandSegmentsStack.length = 0;
+ PpParser._expandSegmentsStack.push([]);
+ PpParser._branchMacros.clear();
+ PpParser.addPredefinedMacro("GL_ES");
+ PpParser._includeMap = includeMap;
+ PpParser._basePathForIncludeKey = basePathForIncludeKey;
+ PpParser._pathOrigin = pathOrigin;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.
| static reset(includeMap: Record<string, string>, pathOrigin: string, basePathForIncludeKey: string) { | |
| this._definedMacros.clear(); | |
| this._expandSegmentsStack.length = 0; | |
| this._expandSegmentsStack.push([]); | |
| this._branchMacros.clear(); | |
| this.addPredefinedMacro("GL_ES"); | |
| this._includeMap = includeMap; | |
| this._basePathForIncludeKey = basePathForIncludeKey; | |
| this._pathOrigin = pathOrigin; | |
| static reset(includeMap: Record<string, string>, pathOrigin: string, basePathForIncludeKey: string) { | |
| PpParser._definedMacros.clear(); | |
| PpParser._expandSegmentsStack.length = 0; | |
| PpParser._expandSegmentsStack.push([]); | |
| PpParser._branchMacros.clear(); | |
| PpParser.addPredefinedMacro("GL_ES"); | |
| PpParser._includeMap = includeMap; | |
| PpParser._basePathForIncludeKey = basePathForIncludeKey; | |
| PpParser._pathOrigin = pathOrigin; |
Tools
Biome
[error] 35-35: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 36-36: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 37-37: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 38-38: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 39-39: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 40-40: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 41-41: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 42-42: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
packages/shader-lab/src/ShaderLab.ts
Outdated
| [] | ||
| [], | ||
| "shaders://root/", | ||
| new URL(pass.name, "shaders://root/").href |
| const compiledPass = shaderLab._parseShaderPass( | ||
| pass.contents, | ||
| pass.vertexEntry, | ||
| pass.fragmentEntry, | ||
| [], | ||
| ShaderPlatformTarget.GLES300, | ||
| [] | ||
| [], | ||
| "shaders://root/", |
There was a problem hiding this comment.
Why not defeine a static
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- packages/core/src/shader/ShaderPass.ts (7 hunks)
- packages/design/src/shader-lab/IShaderLab.ts (1 hunks)
- packages/shader-lab/src/ShaderLab.ts (3 hunks)
- packages/shader-lab/src/preprocessor/PpParser.ts (3 hunks)
- packages/shader-lab/src/preprocessor/Preprocessor.ts (1 hunks)
- tests/src/shader-lab/ShaderValidate.ts (2 hunks)
Files skipped from review as they are similar to previous changes (5)
- packages/core/src/shader/ShaderPass.ts
- packages/design/src/shader-lab/IShaderLab.ts
- packages/shader-lab/src/ShaderLab.ts
- packages/shader-lab/src/preprocessor/Preprocessor.ts
- tests/src/shader-lab/ShaderValidate.ts
Additional context used
Biome
packages/shader-lab/src/preprocessor/PpParser.ts
[error] 35-35: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 36-36: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 37-37: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 38-38: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 39-39: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 40-40: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 41-41: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Additional comments not posted (3)
packages/shader-lab/src/preprocessor/PpParser.ts (3)
13-13: Import addition looks good.The import statement for
ShaderPassis necessary for the new relative path handling logic.
32-32: Static variable addition looks good.The static variable
_basePathForIncludeKeyis necessary for handling the base path for relative includes.
100-114: Fix grammatical issue in error message.The error message has a minor grammatical issue. Change "not founded" to "not found".
- ParserUtils.throw(id.location, `Shader slice "${includedPath}" not founded.`); + ParserUtils.throw(id.location, `Shader slice "${includedPath}" not found.`);Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/core/src/shader/Shader.ts (1 hunks)
- packages/core/src/shader/ShaderPass.ts (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/core/src/shader/Shader.ts
- packages/core/src/shader/ShaderPass.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/shader-lab/src/preprocessor/PpParser.ts (3 hunks)
Additional context used
Biome
packages/shader-lab/src/preprocessor/PpParser.ts
[error] 35-35: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 36-36: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 37-37: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 38-38: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 39-39: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 40-40: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 41-41: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Additional comments not posted (2)
packages/shader-lab/src/preprocessor/PpParser.ts (2)
13-13: LGTM! Import statement is correct.The import statement for
ShaderPassfrom "@galacean/engine" aligns with the changes described in the PR summary.
32-32: LGTM! Static variable addition is correct.The addition of the static variable
_basePathForIncludeKeyis necessary for supporting relative paths in the#includedirective.
| let includedPath: string; | ||
| // builtin path | ||
| if (id.lexeme[0] !== ".") { | ||
| includedPath = id.lexeme; | ||
| } else { | ||
| // relative path | ||
| // @ts-ignore | ||
| includedPath = new URL(id.lexeme, this._basePathForIncludeKey).href.substring(ShaderPass._shaderRootPath.length); | ||
| } | ||
|
|
||
| scanner.scanToChar("\n"); | ||
| const end = scanner.getShaderPosition(); | ||
|
|
||
| const chunk = this._includeMap[id.lexeme]; | ||
| const chunk = this._includeMap[includedPath]; | ||
| if (!chunk) { | ||
| ParserUtils.throw(id.location, `Shader slice "${id.lexeme}" not founded.`); | ||
| ParserUtils.throw(id.location, `Shader slice "${includedPath}" not founded.`); |
There was a problem hiding this comment.
Fix grammatical issue in error message.
The error message has a minor grammatical issue. Change "not founded" to "not found".
- ParserUtils.throw(id.location, `Shader slice "${includedPath}" not founded.`);
+ ParserUtils.throw(id.location, `Shader slice "${includedPath}" not found.`);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.
| let includedPath: string; | |
| // builtin path | |
| if (id.lexeme[0] !== ".") { | |
| includedPath = id.lexeme; | |
| } else { | |
| // relative path | |
| // @ts-ignore | |
| includedPath = new URL(id.lexeme, this._basePathForIncludeKey).href.substring(ShaderPass._shaderRootPath.length); | |
| } | |
| scanner.scanToChar("\n"); | |
| const end = scanner.getShaderPosition(); | |
| const chunk = this._includeMap[id.lexeme]; | |
| const chunk = this._includeMap[includedPath]; | |
| if (!chunk) { | |
| ParserUtils.throw(id.location, `Shader slice "${id.lexeme}" not founded.`); | |
| ParserUtils.throw(id.location, `Shader slice "${includedPath}" not founded.`); | |
| let includedPath: string; | |
| // builtin path | |
| if (id.lexeme[0] !== ".") { | |
| includedPath = id.lexeme; | |
| } else { | |
| // relative path | |
| // @ts-ignore | |
| includedPath = new URL(id.lexeme, this._basePathForIncludeKey).href.substring(ShaderPass._shaderRootPath.length); | |
| } | |
| scanner.scanToChar("\n"); | |
| const end = scanner.getShaderPosition(); | |
| const chunk = this._includeMap[includedPath]; | |
| if (!chunk) { | |
| ParserUtils.throw(id.location, `Shader slice "${includedPath}" not found.`); |
| static reset(includeMap: Record<string, string>, basePathForIncludeKey: string) { | ||
| this._definedMacros.clear(); | ||
| this._expandSegmentsStack.length = 0; | ||
| this._expandSegmentsStack.push([]); | ||
| this._branchMacros.clear(); | ||
| this.addPredefinedMacro("GL_ES"); | ||
| this._includeMap = includeMap; | ||
| this._basePathForIncludeKey = basePathForIncludeKey; |
There was a problem hiding this comment.
Fix the use of this in a static context.
Using this in a static context can be confusing. Replace this with the class name PpParser.
- this._definedMacros.clear();
- this._expandSegmentsStack.length = 0;
- this._expandSegmentsStack.push([]);
- this._branchMacros.clear();
- this.addPredefinedMacro("GL_ES");
- this._includeMap = includeMap;
- this._basePathForIncludeKey = basePathForIncludeKey;
+ PpParser._definedMacros.clear();
+ PpParser._expandSegmentsStack.length = 0;
+ PpParser._expandSegmentsStack.push([]);
+ PpParser._branchMacros.clear();
+ PpParser.addPredefinedMacro("GL_ES");
+ PpParser._includeMap = includeMap;
+ PpParser._basePathForIncludeKey = basePathForIncludeKey;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.
| static reset(includeMap: Record<string, string>, basePathForIncludeKey: string) { | |
| this._definedMacros.clear(); | |
| this._expandSegmentsStack.length = 0; | |
| this._expandSegmentsStack.push([]); | |
| this._branchMacros.clear(); | |
| this.addPredefinedMacro("GL_ES"); | |
| this._includeMap = includeMap; | |
| this._basePathForIncludeKey = basePathForIncludeKey; | |
| static reset(includeMap: Record<string, string>, basePathForIncludeKey: string) { | |
| PpParser._definedMacros.clear(); | |
| PpParser._expandSegmentsStack.length = 0; | |
| PpParser._expandSegmentsStack.push([]); | |
| PpParser._branchMacros.clear(); | |
| PpParser.addPredefinedMacro("GL_ES"); | |
| PpParser._includeMap = includeMap; | |
| PpParser._basePathForIncludeKey = basePathForIncludeKey; |
Tools
Biome
[error] 35-35: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 36-36: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 37-37: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 38-38: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 39-39: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 40-40: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 41-41: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
In ShaderLab you can import common shader chunk by
#includedirective, but only with absolute path. now you can import shader chunk with relative path which is aganst to the path of the main shader file.Complement extension enable directive when target language is gles 100.
Summary by CodeRabbit
New Features
Bug Fixes
#includedirective in test shaders to reference the correct relative path.Refactor