Improve ShaderLab compilation error log and package build
#2364
Conversation
WalkthroughThe changes encompass modifications across various files in the shader lab project, focusing on error handling improvements, build command simplifications, and enhancements to documentation. Key updates include the consolidation of build scripts, the introduction of new error handling mechanisms, and the refinement of testing frameworks. Additionally, shader parsing capabilities have been enhanced, and new shader files have been added to support physically-based rendering. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ShaderLab
participant ErrorHandler
participant Logger
User->>ShaderLab: Parse Shader
ShaderLab->>ErrorHandler: Check for Errors
ErrorHandler->>ShaderLab: Report Errors
ShaderLab->>Logger: Log Errors
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Sway007
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range, codebase verification and nitpick comments (1)
packages/shader-lab/README.md (1)
31-31: Fix grammatical issues.The sentence should be: "You can use the debug version by importing it as follows:"
Apply this diff to fix the grammatical issues:
-you can use debug version by import: +You can use the debug version by importing it as follows:Tools
LanguageTool
[uncategorized] ~31-~31: You might be missing the article “the” here.
Context: ...ides superior performance. you can use debug version by import: ```ts // umd import...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~31-~31: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...formance. you can use debug version by import: ```ts // umd import { ShaderLab } fro...(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (30)
- .github/workflows/ci.yml (3 hunks)
- package.json (1 hunks)
- packages/shader-lab/README.md (1 hunks)
- packages/shader-lab/src/Error.ts (1 hunks)
- packages/shader-lab/src/ShaderLab.ts (6 hunks)
- packages/shader-lab/src/Utils.ts (2 hunks)
- packages/shader-lab/src/codeGen/CodeGenVisitor.ts (3 hunks)
- packages/shader-lab/src/codeGen/GLES100.ts (1 hunks)
- packages/shader-lab/src/codeGen/GLESVisitor.ts (5 hunks)
- packages/shader-lab/src/codeGen/VisitorContext.ts (3 hunks)
- packages/shader-lab/src/common/BaseScanner.ts (5 hunks)
- packages/shader-lab/src/common/BaseToken.ts (1 hunks)
- packages/shader-lab/src/common/ShaderPosition.ts (1 hunks)
- packages/shader-lab/src/contentParser/Scanner.ts (1 hunks)
- packages/shader-lab/src/contentParser/ShaderContentParser.ts (9 hunks)
- packages/shader-lab/src/index.ts (1 hunks)
- packages/shader-lab/src/lexer/Lexer.ts (3 hunks)
- packages/shader-lab/src/parser/AST.ts (4 hunks)
- packages/shader-lab/src/parser/SemanticAnalyzer.ts (3 hunks)
- packages/shader-lab/src/parser/ShaderTargetParser.ts (3 hunks)
- packages/shader-lab/src/parser/builtin/functions.ts (1 hunks)
- packages/shader-lab/src/preprocessor/MacroDefine.ts (2 hunks)
- packages/shader-lab/src/preprocessor/PpParser.ts (16 hunks)
- packages/shader-lab/src/preprocessor/PpScanner.ts (5 hunks)
- packages/shader-lab/src/preprocessor/Preprocessor.ts (2 hunks)
- rollup.config.js (9 hunks)
- tests/src/shader-lab/Preprocessor.test.ts (1 hunks)
- tests/src/shader-lab/ShaderLab.test.ts (5 hunks)
- tests/src/shader-lab/ShaderValidate.ts (2 hunks)
- tests/src/shader-lab/shaders/compilation-error.shader (1 hunks)
Files skipped from review due to trivial changes (4)
- .github/workflows/ci.yml
- package.json
- packages/shader-lab/src/codeGen/GLES100.ts
- tests/src/shader-lab/Preprocessor.test.ts
Additional context used
LanguageTool
packages/shader-lab/README.md
[uncategorized] ~31-~31: You might be missing the article “the” here.
Context: ...ides superior performance. you can use debug version by import: ```ts // umd import...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~31-~31: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...formance. you can use debug version by import: ```ts // umd import { ShaderLab } fro...(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
Biome
packages/shader-lab/src/parser/SemanticAnalyzer.ts
[error] 72-72: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
packages/shader-lab/src/codeGen/VisitorContext.ts
[error] 74-74: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 93-93: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
packages/shader-lab/src/parser/ShaderTargetParser.ts
[error] 123-123: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
packages/shader-lab/src/common/BaseScanner.ts
[error] 137-137: This code is unreachable
... because this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
packages/shader-lab/src/ShaderLab.ts
[error] 27-27: 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/codeGen/CodeGenVisitor.ts
[error] 198-198: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
packages/shader-lab/src/contentParser/ShaderContentParser.ts
[error] 68-68: 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/preprocessor/PpParser.ts
[error] 24-723: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 49-49: 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] 67-67: 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] 68-68: 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 (81)
packages/shader-lab/src/index.ts (1)
5-5: LGTM!The export statement for the
Errormodule is correctly added within the_EDITORdirective.The code changes are approved.
packages/shader-lab/src/common/ShaderPosition.ts (3)
5-5: LGTM!Changing the
lineproperty from optional to mandatory enhances the robustness of the code.The code changes are approved.
6-6: LGTM!Changing the
columnproperty from optional to mandatory enhances the robustness of the code.The code changes are approved.
8-8: LGTM!Updating the
setXmethod signature to requirelineandcolumnas parameters enhances the robustness of the code.The code changes are approved.
packages/shader-lab/src/preprocessor/Preprocessor.ts (2)
1-1: LGTM!Changing the import statement for
PpParserfrom a default import to a named import is appropriate and does not affect the logic of the code.The code changes are approved.
19-19: LGTM!Changing the return type of the
processmethod tostring | nullenhances the method's functionality by allowing it to represent a failure or an exceptional case more explicitly.The code changes are approved.
packages/shader-lab/src/common/BaseToken.ts (1)
23-24: LGTM!The new implementation simplifies the code and enhances readability without altering the overall behavior.
The code changes are approved.
packages/shader-lab/src/preprocessor/MacroDefine.ts (1)
32-32: LGTM!The new implementation enhances the clarity and specificity of error handling in the preprocessor context.
The code changes are approved.
packages/shader-lab/README.md (3)
29-29: LGTM!The documentation correctly explains the purpose of the
ReleaseandDebugversions.The documentation change is approved.
33-40: LGTM!The import statements for the
Debugversion across different module systems are correctly provided and improve user guidance.The documentation change is approved.
41-41: LGTM!The line is correctly placed to separate the sections.
The documentation change is approved.
packages/shader-lab/src/Utils.ts (1)
3-3: LGTM!The removal of the
Loggerimport is consistent with the changes in the error handling strategy.The code change is approved.
packages/shader-lab/src/parser/SemanticAnalyzer.ts (3)
4-4: LGTM!The import statement for
CompilationErroris correctly added, consistent with the shift in error handling strategy.The code change is approved.
26-26: LGTM!The
errorsproperty is correctly updated to hold an array ofCompilationErrorinstances, consistent with the shift in error handling strategy.The code change is approved.
67-71: LGTM!The error handling logic within the
errormethod is correctly updated to construct aCompilationErrorand include a reference toShaderLab._processingPassText, enhancing the error reporting mechanism.The code change is approved.
packages/shader-lab/src/Error.ts (8)
2-3: LGTM!The new imports are correct and necessary for the updated functionality.
The code changes are approved.
6-6: LGTM!The new static property
wrappingLineCountis correctly defined.The code changes are approved.
8-10: LGTM!The new properties
loc,source, andfileare correctly defined.The code changes are approved.
12-17: LGTM!The updated constructor correctly initializes the new properties.
The code changes are approved.
56-61: LGTM!The updated constructor correctly aligns with
GSError.The code changes are approved.
63-66: LGTM!The updated constructor correctly aligns with
GSError.The code changes are approved.
70-73: LGTM!The updated constructor correctly aligns with
GSError.The code changes are approved.
19-53: Verify the correctness of thelogmethod.The
logmethod is well-implemented, but ensure that it correctly processes all possible inputs and edge cases.Run the following script to verify the
logmethod:packages/shader-lab/src/codeGen/VisitorContext.ts (2)
39-39: LGTM!The constructor changes maintain the singleton pattern.
The code changes are approved.
98-98: No changes detected.No review comment needed.
tests/src/shader-lab/shaders/compilation-error.shader (3)
2-6: LGTM!The
EditorPropertiessection is correctly defined.The code changes are approved.
8-14: LGTM!The
EditorMacrossection is correctly defined.The code changes are approved.
16-43: LGTM!The
SubShadersection is correctly defined.The code changes are approved.
tests/src/shader-lab/ShaderValidate.ts (2)
97-97: No changes detected.No changes were made to the
glslValidatefunction.
98-123: Verify the correctness of_parseShaderContentand_parseShaderPassmethods.Ensure that
_parseShaderContentand_parseShaderPassmethods are correctly implemented and accessible.Consider adding error handling.
Add error handling for potential issues during parsing to improve robustness.
packages/shader-lab/src/parser/ShaderTargetParser.ts (1)
38-43: LGTM!The changes improve error handling and provide better debugging capabilities in the editor context.
packages/shader-lab/src/common/BaseScanner.ts (2)
41-42: Verify the correctness of the new parameter order.Ensure that the new order of parameters passed to the
ShaderLab.createPositionmethod is correct and consistent with the rest of the codebase.
131-139: LGTM!The changes improve error handling and provide a more structured way to handle errors during scanning.
Tools
Biome
[error] 137-137: This code is unreachable
... because this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
packages/shader-lab/src/ShaderLab.ts (8)
12-12: LGTM!The import statement for
GSErroris correct and necessary for the new error handling logic.
24-24: LGTM!The addition of the static property
_processingPassTextis correct and aligns with the changes in the class.
39-39: LGTM!The addition of the private property
_errorsis necessary for the new error handling logic.
44-45: LGTM!The getter method
errorsis correctly implemented to retrieve the compilation errors.
69-77: LGTM!The error handling logic in the
_parseShaderPassmethod is correctly implemented to log and handle errors during shader pass parsing.Also applies to: 89-97, 107-112
121-126: LGTM!The error handling logic in the
_parseShaderContentmethod is correctly implemented to clear and log errors during shader content parsing.
137-138: LGTM!The addition of default values to the parameters of the
_parsemethod improves its flexibility.
165-171: LGTM!The
_logErrorsmethod is correctly implemented to log errors.packages/shader-lab/src/codeGen/GLESVisitor.ts (3)
9-9: LGTM!The import statement for
EKeywordis correct and necessary for the new error handling logic.
32-32: LGTM!The error handling logic in the
visitShaderProgrammethod is correctly implemented to clear errors and handle invalid return types.Also applies to: 51-59
71-71: LGTM!The error handling logic in the
vertexMainmethod is correctly implemented to handle missing attribute structs.rollup.config.js (9)
28-29: LGTM!The addition of the variable
shaderLabPkgis correct and necessary for the new build configuration logic.
61-61: LGTM!The addition of the parameter
editorModeto theconfigfunction is correct and necessary for the new build configuration logic.
64-70: LGTM!The update to the
curPluginsarray to include thejsccplugin is correct and necessary for the new build configuration logic.
86-91: LGTM!The update to the
umdfunction to handleeditorModeis correct and necessary for the new build configuration logic.
111-115: LGTM!The update to the
minifunction to handleeditorModeis correct and necessary for the new build configuration logic.
130-135: LGTM!The update to the
modulefunction to handleeditorModeis correct and necessary for the new build configuration logic.
194-195: LGTM!The update to the
getUMDfunction to includeshaderLabPkgwitheditorModeis correct and necessary for the new build configuration logic.
200-202: LGTM!The update to the
getModulefunction to includeshaderLabPkgwitheditorModeis correct and necessary for the new build configuration logic.
207-209: LGTM!The update to the
getMinifunction to includeshaderLabPkgwitheditorModeis correct and necessary for the new build configuration logic.packages/shader-lab/src/codeGen/CodeGenVisitor.ts (1)
47-50: LGTM!The function correctly handles errors by pushing them to the
_errorsarray, improving error tracking during code generation.The code changes are approved.
Also applies to: 53-56
packages/shader-lab/src/preprocessor/PpScanner.ts (3)
89-89: LGTM!The function correctly uses
this.throwErrorto handle errors, improving encapsulation.The code changes are approved.
143-143: LGTM!The function correctly uses
this.throwErrorto handle errors, improving encapsulation.The code changes are approved.
Also applies to: 150-150
232-232: LGTM!The function correctly uses
this.throwErrorto handle errors, improving encapsulation.The code changes are approved.
tests/src/shader-lab/ShaderLab.test.ts (1)
1-5: LGTM!The function correctly uses
ShaderLabEditorandShaderLabReleaseto test both versions of theShaderLabclass. The new test case for handling compilation errors improves test coverage.The code changes are approved.
Also applies to: 107-108, 117-117, 126-126, 185-186, 191-191, 196-197, 202-203, 208-209, 214-215, 220-221, 226-227, 232-233, 238-239, 242-251
packages/shader-lab/src/lexer/Lexer.ts (2)
279-279: LGTM!The use of the
throwErrormethod improves error handling by providing more context.The code changes are approved.
385-386: LGTM!The use of the
throwErrormethod improves error handling by providing more context.The code changes are approved.
packages/shader-lab/src/contentParser/ShaderContentParser.ts (6)
185-196: LGTM!The use of conditional compilation directives for error handling improves the robustness and flexibility of the code.
The code changes are approved.
244-256: LGTM!The use of conditional compilation directives for error handling improves the robustness and flexibility of the code.
The code changes are approved.
263-273: LGTM!The use of conditional compilation directives for error handling improves the robustness and flexibility of the code.
The code changes are approved.
334-339: LGTM!The use of conditional compilation directives for error handling improves the robustness and flexibility of the code.
The code changes are approved.
301-314: LGTM!The use of conditional compilation directives for error handling improves the robustness and flexibility of the code.
The code changes are approved.
485-491: LGTM!The use of conditional compilation directives for error handling improves the robustness and flexibility of the code.
The code changes are approved.
packages/shader-lab/src/preprocessor/PpParser.ts (9)
49-50: LGTM!The use of conditional compilation directives for error handling improves the robustness and flexibility of the code.
The code changes are approved.
Tools
Biome
[error] 49-49: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Line range hint
66-100: LGTM!The use of conditional compilation directives for error handling improves the robustness and flexibility of the code. The change in return type allows for better error handling.
The code changes are approved.
Tools
Biome
[error] 63-63: 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] 67-67: 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] 68-68: 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] 70-70: 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] 70-70: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
109-115: LGTM!The
reportErrorfunction centralizes error reporting and improves the maintainability and readability of the code.The code changes are approved.
136-137: LGTM!The use of the
reportErrormethod improves error handling by providing more context.The code changes are approved.
311-312: LGTM!The use of the
reportErrormethod improves error handling by providing more context.The code changes are approved.
337-338: LGTM!The use of the
reportErrormethod improves error handling by providing more context.The code changes are approved.
361-362: LGTM!The use of the
reportErrormethod improves error handling by providing more context.The code changes are approved.
383-384: LGTM!The use of the
reportErrormethod improves error handling by providing more context.The code changes are approved.
405-405: LGTM!The use of the
reportErrormethod improves error handling by providing more context.The code changes are approved.
packages/shader-lab/src/parser/builtin/functions.ts (2)
135-138: Ensure consistency across all overloads of themaxfunction.The additional parameter enhances flexibility. Verify that all overloads are correctly updated.
Run the following script to verify the function usage:
Verification successful
All overloads of the
maxfunction are consistent with the new signature.The changes have been correctly applied across all instances in the codebase. No further action is needed.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `BuiltinFunction._create("max")` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type ts -A 5 $'BuiltinFunction._create("max")'Length of output: 52
Script:
#!/bin/bash # Description: Broaden the search to find any occurrences of `BuiltinFunction._create` involving `max`. # Test: Search for any `BuiltinFunction._create` calls with `max` as a parameter. rg --type ts -A 5 'BuiltinFunction\._create\("max"'Length of output: 1329
129-132: Ensure consistency across all overloads of theminfunction.The additional parameter enhances flexibility. Verify that all overloads are correctly updated.
Run the following script to verify the function usage:
Verification successful
All overloads of the
minfunction are consistent.The changes to the
minfunction have been applied uniformly across all overloads, ensuring consistency and correctness. No further action is needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `BuiltinFunction._create("min")` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type ts -A 5 $'BuiltinFunction._create("min")'Length of output: 52
Script:
#!/bin/bash # Description: Search for all occurrences of `BuiltinFunction._create` to verify its usage. # Test: Search for the function usage. Expect: Occurrences of `BuiltinFunction._create` with different parameters. rg --type ts -A 5 $'BuiltinFunction._create'Length of output: 45037
packages/shader-lab/src/parser/AST.ts (4)
438-438: LGTM!The updated error message improves clarity and consistency.
The code changes are approved.
462-465: LGTM!The updated error messages improve clarity and consistency.
The code changes are approved.
538-538: LGTM!The updated error message improves clarity and consistency.
The code changes are approved.
875-875: LGTM!The updated error message improves clarity and consistency.
The code changes are approved.
| scanToCharacter(char: string): void { | ||
| while (this.getCurChar() !== char) { | ||
| this._advance(); | ||
| } | ||
| this._advance(); | ||
| } |
There was a problem hiding this comment.
Handle the case where the specified character is not found.
The method does not handle the case where the specified character is not found in the source content, which could lead to an infinite loop. Consider adding a check to handle this scenario.
Apply this diff to handle the case where the specified character is not found:
def scanToCharacter(char: string): void {
- while (this.getCurChar() !== char) {
+ while (this.getCurChar() !== char && !this.isEnd()) {
this._advance();
}
if (!this.isEnd()) {
this._advance();
}
}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.
| scanToCharacter(char: string): void { | |
| while (this.getCurChar() !== char) { | |
| this._advance(); | |
| } | |
| this._advance(); | |
| } | |
| scanToCharacter(char: string): void { | |
| while (this.getCurChar() !== char && !this.isEnd()) { | |
| this._advance(); | |
| } | |
| if (!this.isEnd()) { | |
| this._advance(); | |
| } | |
| } |
| this.errors.push(err); | ||
| return err; | ||
| // #else | ||
| throw new Error(param.join("")); |
There was a problem hiding this comment.
Remove unreachable code.
The code is unreachable because the previous statement returns from the function.
Apply this diff to remove the unreachable code:
- throw new Error(param.join(""));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.
| throw new Error(param.join("")); |
Tools
Biome
[error] 72-72: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
| referenceAttribute(ident: BaseToken): GSError { | ||
| if (this._referencedAttributeList[ident.lexeme]) return; | ||
|
|
||
| const prop = this.attributeList.find((item) => item.ident.lexeme === ident); | ||
| const prop = this.attributeList.find((item) => item.ident.lexeme === ident.lexeme); | ||
| if (!prop) { | ||
| Logger.error("referenced attribute not found:", ident); | ||
| return; | ||
| // #if _EDITOR | ||
| return new CompilationError( | ||
| `referenced attribute not found: ${ident.lexeme}`, | ||
| ident.location, | ||
| ShaderLab._processingPassText | ||
| ); | ||
| // #else | ||
| // @ts-ignore | ||
| throw new Error(`referenced attribute not found: ${ident.lexeme}`); | ||
| // #endif | ||
| } | ||
| this._referencedAttributeList[ident] = prop; | ||
| this._referencedAttributeList[ident.lexeme] = prop; |
There was a problem hiding this comment.
LGTM! But address the unreachable code.
The changes improve type safety and error handling. However, the throw statement is unreachable due to the preceding return.
Apply this diff to remove the unreachable code:
- // @ts-ignore
- throw new Error(`referenced attribute not found: ${ident.lexeme}`);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.
| referenceAttribute(ident: BaseToken): GSError { | |
| if (this._referencedAttributeList[ident.lexeme]) return; | |
| const prop = this.attributeList.find((item) => item.ident.lexeme === ident); | |
| const prop = this.attributeList.find((item) => item.ident.lexeme === ident.lexeme); | |
| if (!prop) { | |
| Logger.error("referenced attribute not found:", ident); | |
| return; | |
| // #if _EDITOR | |
| return new CompilationError( | |
| `referenced attribute not found: ${ident.lexeme}`, | |
| ident.location, | |
| ShaderLab._processingPassText | |
| ); | |
| // #else | |
| // @ts-ignore | |
| throw new Error(`referenced attribute not found: ${ident.lexeme}`); | |
| // #endif | |
| } | |
| this._referencedAttributeList[ident] = prop; | |
| this._referencedAttributeList[ident.lexeme] = prop; | |
| referenceAttribute(ident: BaseToken): GSError { | |
| if (this._referencedAttributeList[ident.lexeme]) return; | |
| const prop = this.attributeList.find((item) => item.ident.lexeme === ident.lexeme); | |
| if (!prop) { | |
| // #if _EDITOR | |
| return new CompilationError( | |
| `referenced attribute not found: ${ident.lexeme}`, | |
| ident.location, | |
| ShaderLab._processingPassText | |
| ); | |
| // #else | |
| // #endif | |
| } | |
| this._referencedAttributeList[ident.lexeme] = prop; |
Tools
Biome
[error] 74-74: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
| referenceVarying(ident: BaseToken): CompilationError | undefined { | ||
| if (this._referencedVaryingList[ident.lexeme]) return; | ||
|
|
||
| const prop = this.varyingStruct?.propList.find((item) => item.ident.lexeme === ident); | ||
| const prop = this.varyingStruct?.propList.find((item) => item.ident.lexeme === ident.lexeme); | ||
| if (!prop) { | ||
| Logger.error("referenced varying not found:", ident); | ||
| return; | ||
| // #if _EDITOR | ||
| return new CompilationError( | ||
| `referenced varying not found: ${ident.lexeme}`, | ||
| ident.location, | ||
| ShaderLab._processingPassText | ||
| ); | ||
| // #else | ||
| // @ts-ignore | ||
| throw new Error(`referenced varying not found: ${ident.lexeme}`); | ||
| // #endif | ||
| } | ||
| this._referencedVaryingList[ident] = prop; | ||
| this._referencedVaryingList[ident.lexeme] = prop; |
There was a problem hiding this comment.
LGTM! But address the unreachable code.
The changes improve type safety and error handling. However, the throw statement is unreachable due to the preceding return.
Apply this diff to remove the unreachable code:
- // @ts-ignore
- throw new Error(`referenced varying not found: ${ident.lexeme}`);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.
| referenceVarying(ident: BaseToken): CompilationError | undefined { | |
| if (this._referencedVaryingList[ident.lexeme]) return; | |
| const prop = this.varyingStruct?.propList.find((item) => item.ident.lexeme === ident); | |
| const prop = this.varyingStruct?.propList.find((item) => item.ident.lexeme === ident.lexeme); | |
| if (!prop) { | |
| Logger.error("referenced varying not found:", ident); | |
| return; | |
| // #if _EDITOR | |
| return new CompilationError( | |
| `referenced varying not found: ${ident.lexeme}`, | |
| ident.location, | |
| ShaderLab._processingPassText | |
| ); | |
| // #else | |
| // @ts-ignore | |
| throw new Error(`referenced varying not found: ${ident.lexeme}`); | |
| // #endif | |
| } | |
| this._referencedVaryingList[ident] = prop; | |
| this._referencedVaryingList[ident.lexeme] = prop; | |
| referenceVarying(ident: BaseToken): CompilationError | undefined { | |
| if (this._referencedVaryingList[ident.lexeme]) return; | |
| const prop = this.varyingStruct?.propList.find((item) => item.ident.lexeme === ident.lexeme); | |
| if (!prop) { | |
| // #if _EDITOR | |
| return new CompilationError( | |
| `referenced varying not found: ${ident.lexeme}`, | |
| ident.location, | |
| ShaderLab._processingPassText | |
| ); | |
| // #else | |
| // #endif | |
| } | |
| this._referencedVaryingList[ident.lexeme] = prop; |
Tools
Biome
[error] 93-93: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
| Varyings vert(Attributes attr) { | ||
| Varyings v; | ||
|
|
||
| gl_Position = renderer_MVPMat * attr2.POSITION; | ||
| none(); | ||
| v.v_pos = gl_Position.xyz; | ||
| v.v_uv = attr.TEXCOORD_023; | ||
| return v; |
There was a problem hiding this comment.
Fix the syntax errors in the vertex shader.
The vertex shader has syntax errors in the gl_Position and v.v_uv assignments.
Apply this diff to fix the syntax errors:
- gl_Position = renderer_MVPMat * attr2.POSITION;
+ gl_Position = renderer_MVPMat * attr.POSITION;
- v.v_uv = attr.TEXCOORD_023;
+ v.v_uv = attr.TEXCOORD_0;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Varyings vert(Attributes attr) { | |
| Varyings v; | |
| gl_Position = renderer_MVPMat * attr2.POSITION; | |
| none(); | |
| v.v_pos = gl_Position.xyz; | |
| v.v_uv = attr.TEXCOORD_023; | |
| return v; | |
| Varyings vert(Attributes attr) { | |
| Varyings v; | |
| gl_Position = renderer_MVPMat * attr.POSITION; | |
| none(); | |
| v.v_pos = gl_Position.xyz; | |
| v.v_uv = attr.TEXCOORD_0; | |
| return v; |
| // #if _EDITOR | ||
| this.sematicAnalyzer.errors.push( | ||
| new CompilationError(`Unexpected token ${token.lexeme}`, token.location, ShaderLab._processingPassText) | ||
| ); | ||
| return null; | ||
| // #else | ||
| throw new Error(`Unexpected token ${token.lexeme}`); | ||
| // #endif |
There was a problem hiding this comment.
Remove unreachable code.
The code at line 137 is unreachable because the function will return beforehand. Remove the unreachable code to improve clarity.
- throw new Error(`Unexpected token ${token.lexeme}`);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.
| // #if _EDITOR | |
| this.sematicAnalyzer.errors.push( | |
| new CompilationError(`Unexpected token ${token.lexeme}`, token.location, ShaderLab._processingPassText) | |
| ); | |
| return null; | |
| // #else | |
| throw new Error(`Unexpected token ${token.lexeme}`); | |
| // #endif | |
| // #if _EDITOR | |
| this.sematicAnalyzer.errors.push( | |
| new CompilationError(`Unexpected token ${token.lexeme}`, token.location, ShaderLab._processingPassText) | |
| ); | |
| return null; | |
| // #else | |
| // #endif |
Tools
Biome
[error] 123-123: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
| throwError(pos: ShaderPosition | ShaderRange, ...msgs: any[]) { | ||
| // #if _EDITOR | ||
| const error = new ScannerError(msgs.join(" "), pos, this._source); | ||
| error.log(); | ||
| throw error; | ||
| // #else | ||
| throw new Error(msgs.join("")); | ||
| // #endif | ||
| } |
There was a problem hiding this comment.
Remove unreachable code.
The code at line 137 is unreachable because the function will throw an exception beforehand. Remove the unreachable code to improve clarity.
- throw new Error(msgs.join(""));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.
| throwError(pos: ShaderPosition | ShaderRange, ...msgs: any[]) { | |
| // #if _EDITOR | |
| const error = new ScannerError(msgs.join(" "), pos, this._source); | |
| error.log(); | |
| throw error; | |
| // #else | |
| throw new Error(msgs.join("")); | |
| // #endif | |
| } | |
| throwError(pos: ShaderPosition | ShaderRange, ...msgs: any[]) { | |
| // #if _EDITOR | |
| const error = new ScannerError(msgs.join(" "), pos, this._source); | |
| error.log(); | |
| throw error; | |
| // #else | |
| // #endif | |
| } |
Tools
Biome
[error] 137-137: This code is unreachable
... because this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
| protected reportError(loc: ShaderRange | ShaderPosition, message: string): CompilationError { | ||
| let error: Error; | ||
| // #if _EDITOR | ||
| error = new CompilationError(message, loc, ShaderLab._processingPassText); | ||
| this._errors.push(<CompilationError>error); | ||
| return <CompilationError>error; | ||
| // #else | ||
| throw new Error(message); | ||
| // #endif | ||
| } |
There was a problem hiding this comment.
Remove unreachable code.
The function correctly handles errors based on the _EDITOR preprocessor directive. However, the code after the return statement is unreachable.
Apply this diff to remove the unreachable code:
protected reportError(loc: ShaderRange | ShaderPosition, message: string): CompilationError {
let error: Error;
// #if _EDITOR
error = new CompilationError(message, loc, ShaderLab._processingPassText);
this._errors.push(<CompilationError>error);
return <CompilationError>error;
// #else
- throw new Error(message);
// #endif
}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.
| protected reportError(loc: ShaderRange | ShaderPosition, message: string): CompilationError { | |
| let error: Error; | |
| // #if _EDITOR | |
| error = new CompilationError(message, loc, ShaderLab._processingPassText); | |
| this._errors.push(<CompilationError>error); | |
| return <CompilationError>error; | |
| // #else | |
| throw new Error(message); | |
| // #endif | |
| } | |
| protected reportError(loc: ShaderRange | ShaderPosition, message: string): CompilationError { | |
| let error: Error; | |
| // #if _EDITOR | |
| error = new CompilationError(message, loc, ShaderLab._processingPassText); | |
| this._errors.push(<CompilationError>error); | |
| return <CompilationError>error; | |
| // #else | |
| // #endif | |
| } |
Tools
Biome
[error] 198-198: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
|
建议使用 LogLevel 的设计去实现编译细节差异，而不是针对平台去设计不同的实现，平台是内部的期望，不具有通用性意义，且维护成本大，容易烂尾。有机会的话建议把以前的 build:editor 都改成通用的设计。 |
条件编译最大的优势是减小最终打包的包体尺寸，运行时还能去除if else比较的开销。缺点是维护性欠佳，主要是因为js语言没有原生的宏编译，只能借助rollup插件实现。现在release版本包体尺寸比editor版本包体尺寸少30kb (109kb / 139kb) |
| diagnosticMessage += lineSplit; | ||
| diagnosticMessage += `${lines[i]}\n`; | ||
| if (i >= start.line && i <= end.line) { | ||
| diagnosticMessage += " ".repeat(lineSplit.length + start.column); |
There was a problem hiding this comment.
for 循环要特别注意这种高频操作，别浪费性能，移到外面去
There was a problem hiding this comment.
这个类都是在Editor版本里的，release版本没有这些性能损耗
| diagnosticMessage += `${lines[i]}\n`; | ||
| if (i >= start.line && i <= end.line) { | ||
| diagnosticMessage += " ".repeat(lineSplit.length + start.column); | ||
| diagnosticMessage += "^".repeat(Math.max(end.column - start.column, 1)); |
| // commonjs | ||
| import { ShaderLab } from "@galacean/engine-shader-lab/dist/main.editor"; | ||
| ``` | ||
|
|
| let source = originSource; | ||
| if (_source) source = _source; | ||
| if (!source) { | ||
| Logger.error(message); |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/shader-lab/src/Error.ts (1 hunks)
- packages/shader-lab/src/ShaderLab.ts (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/shader-lab/src/Error.ts
Additional context used
Biome
packages/shader-lab/src/ShaderLab.ts
[error] 27-27: 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 (12)
packages/shader-lab/src/ShaderLab.ts (12)
5-5: LGTM!The code changes are approved.
11-13: LGTM!The code changes are approved.
19-20: LGTM!The code changes are approved.
24-24: LGTM!The code changes are approved.
38-47: LGTM!The code changes are approved.
69-77: LGTM!The code changes are approved.
83-87: LGTM!The code changes are approved.
89-97: LGTM!The code changes are approved.
105-112: LGTM!The code changes are approved.
118-127: LGTM!The code changes are approved.
137-138: LGTM!The code changes are approved.
162-171: LGTM!The code changes are approved.
| static createPosition(index: number, line?: number, column?: number): ShaderPosition { | ||
| const position = this._shaderPositionPool.get(); | ||
| position.setX( | ||
| index, | ||
| // #if _EDITOR | ||
| line, | ||
| column | ||
| // #endif | ||
| ); | ||
| position.setX(index, line, column); |
There was a problem hiding this comment.
Refactor suggestion: Use class name instead of this in static context.
Using this in a static context can be confusing. Use the class name instead.
Apply this diff to refactor the code:
- const position = this._shaderPositionPool.get();
+ const position = ShaderLab._shaderPositionPool.get();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 createPosition(index: number, line?: number, column?: number): ShaderPosition { | |
| const position = this._shaderPositionPool.get(); | |
| position.setX( | |
| index, | |
| // #if _EDITOR | |
| line, | |
| column | |
| // #endif | |
| ); | |
| position.setX(index, line, column); | |
| static createPosition(index: number, line?: number, column?: number): ShaderPosition { | |
| const position = ShaderLab._shaderPositionPool.get(); | |
| position.setX(index, line, column); |
Tools
Biome
[error] 27-27: 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
| /** | ||
| * Retrieve the compilation errors | ||
| */ | ||
| get errors(): GSError[] | undefined { |
| @@ -25,4 +25,11 @@ export default class Scanner extends BaseScanner { | |||
| } | |||
| return Number(this._source.substring(start, this._currentIndex)); | |||
| } | |||
|
|
|||
| scanToCharacter(char: string): void { | |||
| while (this.getCurChar() !== char) { | |||
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
packages/shader-lab/src/ShaderLab.ts (1)
26-30: LGTM, but consider using the class name instead ofthisin a static context.The changes to the
createPositionmethod improve code readability and maintainability by removing unnecessary comments and simplifying the parameter handling.However, using
thisin a static context can be confusing. Use the class name instead.Apply this diff to refactor the code:
- const position = this._shaderPositionPool.get(); + const position = ShaderLab._shaderPositionPool.get();Tools
Biome
[error] 27-27: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/shader-lab/src/ShaderLab.ts (6 hunks)
- packages/shader-lab/src/contentParser/Scanner.ts (1 hunks)
Additional context used
Biome
packages/shader-lab/src/ShaderLab.ts
[error] 27-27: 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 (7)
packages/shader-lab/src/contentParser/Scanner.ts (1)
29-34: LGTM! The previous comment has been addressed.The new
scanToCharactermethod correctly handles the case where the specified character is not found by checking for the end of the source content, thus avoiding a potential infinite loop.packages/shader-lab/src/ShaderLab.ts (6)
39-39: LGTM!The new
_errorsproperty enhances error handling capabilities by providing a centralized location to store compilation errors.
44-46: LGTM!The new
errorsgetter method provides a convenient way to access the compilation errors stored in the_errorsproperty.
Line range hint
49-115: LGTM!The modifications to the
_parseShaderPassmethod improve error handling by logging errors from thePpParserand the shader parser, and storing them in the_errorsarray for later retrieval. These changes enhance the robustness of the shader compilation process.
117-127: LGTM!The modifications to the
_parseShaderContentmethod enhance error handling by clearing the_errorsarray at the start of its execution and collecting errors from theShaderContentParser, appending them to the_errorsarray. These changes improve error tracking and ensure that the method starts fresh for each shader content parsing.
Line range hint
133-160: LGTM!The modifications to the
_parsemethod improve its flexibility by providing default values for its parameters. These changes make the method more adaptable and easier to use in different scenarios.
165-171: LGTM!The addition of the
_logErrorsmethod enhances error handling capabilities by encapsulating the error logging logic. It provides a centralized and consistent way to log errors, making the code more maintainable and easier to understand.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
change rollup config to produce 2 version
ShaderLabpackage dist:ReleaseandEditor.Improve the error log when compiation error of ShaderLab occur in
Editorversion.related issue: #2248
Summary by CodeRabbit
Summary by CodeRabbit
New Features
scanToCharacterandreportError.Bug Fixes
Documentation
Chores