ShaderLab ignore Editor module
#2395
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: 6
🧹 Outside diff range and nitpick comments (15)
packages/core/src/shaderlib/extra/background-texture.vs.glsl (2)
4-4: Document the purpose and components of camera_ProjectionParams.The new uniform
camera_ProjectionParamsis declared as vec4 but only the x component is used. Please add a comment explaining:
- The purpose of this projection parameter
- The expected range/values for each component
- Why vec4 is needed when only x is used
8-9: Consider adding bounds validation.The shader assumes
camera_ProjectionParams.xis valid without any bounds checking. Consider adding a preprocessor directive or runtime check to ensure valid projection parameters.#ifdef GL_ES // Add validation for camera_ProjectionParams.x if (abs(camera_ProjectionParams.x) < 1e-6) { gl_Position.y = 0.0; // or handle error case } #endifpackages/loader/src/PathUtils.ts (2)
1-2: Consider converting to a module with pure functions.The class currently only contains static members. Consider restructuring as a module with pure functions for better maintainability and to align with functional programming principles.
-/** @internal */ -export class PathUtils { +/** @internal */ +export const urlSchema = "files://"; + +export const shaderIncludeRegex = /\s#include\s+"([^\\"]+)"/gm; + +export function pathResolve(path: string, base: string): string { + return new URL(path, urlSchema + base).href.substring(urlSchema.length); +} + +export function isRelativePath(path: string): boolean { + return path[0] === "."; +}
3-4: Add JSDoc comments and optimize regex pattern.The static members would benefit from documentation explaining their purpose and usage. Additionally, the regex pattern could be optimized.
- private static _urlSchema = "files://"; - static shaderIncludeRegex = /\s#include\s+"([^\\"]+)"/gm; + /** URL schema prefix used for path resolution */ + private static _urlSchema = "files://"; + + /** + * Regular expression for matching shader include directives. + * Matches patterns like: #include "path/to/shader" + */ + static readonly shaderIncludeRegex = /\s#include\s+"([^"\\]+)"/gm;packages/core/src/asset/LoadItem.ts (1)
29-32: Enhance documentation for thevirtualPathproperty.The addition of the optional
virtualPathproperty is well-structured and maintains backward compatibility. However, the documentation could be more descriptive.Consider expanding the JSDoc comment to clarify:
/** - * Asset path in editor + * Virtual path used to identify the asset within the editor environment. + * This path may differ from the actual asset URL and is primarily used + * for editor-specific functionality. */ virtualPath?: string;packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts (1)
95-96: Add JSDoc comments to document the type's purpose.Consider adding documentation to explain why shader references don't need the 'key' property, helping future maintainers understand the design decision.
+/** + * Represents a reference to a shader asset. + * Unlike general asset references, shader references only require a refId + * as they don't need the optional key property for identification. + */ export type IShaderRef = Omit<IAssetRef, "key">;packages/loader/src/resource-deserialize/resources/schema/MaterialSchema.ts (2)
93-94: Consider using const enum for better performanceThe change to use
MaterialLoaderTypeenum improves type safety and maintainability. Consider usingconst enumfor better runtime performance as it will be inlined at compile time.
102-110: LGTM: Well-structured enum definitionThe
MaterialLoaderTypeenum effectively consolidates material value types. Consider adding JSDoc comments to document the purpose and usage of each type.Example documentation:
export enum MaterialLoaderType { + /** 2D vector type for UV coordinates, positions, etc. */ Vector2 = "Vector2", + /** 3D vector type for positions, normals, etc. */ Vector3 = "Vector3", // ... add docs for other types }packages/core/src/asset/ResourceManager.ts (1)
Line range hint
332-550: Consider documenting the virtual path mapping systemThe changes introduce a robust virtual path mapping system for editor resources. Consider adding documentation to explain:
- The relationship between virtual paths and actual resource paths
- The lifecycle of virtual path mappings
- Best practices for using this system in the editor
packages/loader/src/ShaderLoader.ts (2)
22-22: Address the TODO comment for future deprecationThe TODO comment suggests deleting certain code snippets when a breaking change version is released. Please plan for this update to ensure a smooth transition.
Would you like assistance in preparing for this deprecation or opening a GitHub issue to track this task?
33-35: Simplify thegetBuiltinShadermethod using optional chainingYou can simplify the method by using optional chaining for better readability and conciseness. This change also safely handles cases where
matchmay benullorundefined.Apply this diff to refactor the method:
- private getBuiltinShader(code: string) { - const match = code.match(ShaderLoader._builtinRegex); - if (match && match[1]) return match[1]; - } + private getBuiltinShader(code: string) { + return code.match(ShaderLoader._builtinRegex)?.[1]; + }🧰 Tools
🪛 Biome
[error] 35-35: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/loader/src/MaterialLoader.ts (4)
118-118: Use strict equality===for undefined checksIn line 118, consider using
value === undefinedinstead ofvalue == undefinedto prevent unintended type coercion and improve code clarity.
74-113: Add a default case to the switch statement to handle unexpected typesThe switch statement does not include a default case. Adding a default case can help catch and handle any unexpected
MaterialLoaderTypevalues, improving the robustness of the code.Apply this diff to add a default case:
switch (type) { case MaterialLoaderType.Vector2: materialShaderData.setVector2(key, new Vector2((<IVector2>value).x, (<IVector2>value).y)); break; // ... other cases ... case MaterialLoaderType.Integer: materialShaderData.setInt(key, Number(value)); break; + default: + console.warn(`Unhandled MaterialLoaderType: ${type}`); + break; }
127-128: Handle errors in texture loading promisesCurrently, if any of the texture promises reject, the error is not caught, which could lead to unhandled promise rejections. Consider adding error handling to the texture loading promises.
Apply this diff to handle errors:
return Promise.all(texturePromises).then(() => { return material; + }).catch((error) => { + console.error('Error loading textures:', error); + throw error; });
125-125: EnsurerenderStateis defined before parsingBefore calling
parseProperty, verify thatrenderStateis defined to prevent potential runtime errors.Apply this diff to add a check:
+ if (renderState) { parseProperty(material, "renderState", renderState); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
packages/core/src/RenderPipeline/BasicRenderPipeline.ts(3 hunks)packages/core/src/asset/AssetType.ts(1 hunks)packages/core/src/asset/LoadItem.ts(1 hunks)packages/core/src/asset/ResourceManager.ts(2 hunks)packages/core/src/shaderlib/extra/background-texture.vs.glsl(1 hunks)packages/loader/src/MaterialLoader.ts(3 hunks)packages/loader/src/PathUtils.ts(1 hunks)packages/loader/src/ShaderChunkLoader.ts(1 hunks)packages/loader/src/ShaderLoader.ts(1 hunks)packages/loader/src/index.ts(1 hunks)packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts(1 hunks)packages/loader/src/resource-deserialize/resources/schema/MaterialSchema.ts(2 hunks)packages/shader-lab/src/contentParser/ShaderContentParser.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/shader-lab/src/contentParser/ShaderContentParser.ts
🧰 Additional context used
🪛 Biome
packages/loader/src/PathUtils.ts
[error] 2-13: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/loader/src/ShaderChunkLoader.ts
[error] 14-14: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 15-15: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 33-33: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/loader/src/ShaderLoader.ts
[error] 35-35: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (16)
packages/core/src/shaderlib/extra/background-texture.vs.glsl (1)
8-8: Verify the Y-coordinate modification behavior.
The multiplication of gl_Position.y with camera_ProjectionParams.x could be:
- A Y-axis flip operation (if x = -1)
- A scaling operation (if x ≠ ±1)
Please ensure this matches the intended behavior.
Let's check if this change is consistently handled in the render pipeline:
✅ Verification successful
Y-coordinate flip operation verified as intended behavior
The multiplication of gl_Position.y with camera_ProjectionParams.x is a Y-axis flip operation, which is consistent with the engine's rendering pipeline:
- The
RenderContextdefines a_flipYMatrixwith a -1 scale for Y-axis - The background texture shader applies this flip via
camera_ProjectionParams.x - This is a standard practice in graphics engines to handle coordinate system differences
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for camera_ProjectionParams usage in render pipeline code
rg -A 5 "camera_ProjectionParams" --type ts
# Look for similar Y-coordinate modifications in other shaders
rg "gl_Position\.y.*=.*camera" --type glsl
Length of output: 804
Script:
#!/bin/bash
# Search for camera_ProjectionParams usage in shader files
rg "camera_ProjectionParams" -t glsl -A 5
# Look for similar Y-coordinate modifications in shader files
rg "gl_Position\.y.*="
# Search for any code that sets up camera projection parameters
ast-grep --pattern 'camera_ProjectionParams = $_'
Length of output: 254
Script:
#!/bin/bash
# Search for files with .glsl extension and then search within them
fd -e glsl -x rg -l "camera_ProjectionParams" {}
# Search for camera projection setup in TypeScript files
rg "camera.*ProjectionParams.*=" -A 5 --type ts
# Look for camera projection matrix setup
rg "camera.*projection.*matrix" -i -A 5 --type ts
Length of output: 10867
Script:
#!/bin/bash
# Check the content of camera declaration shader file
cat packages/core/src/shaderlib/camera_declare.glsl
# Check the content of background texture vertex shader
cat packages/core/src/shaderlib/extra/background-texture.vs.glsl
# Look for camera projection setup in RenderContext
rg "camera_ProjectionParams" -B 5 -A 5 packages/core/src/RenderPipeline/RenderContext.ts
Length of output: 1084
packages/loader/src/PathUtils.ts (1)
1-13: Verify integration with ShaderChunkLoader.
Let's verify how this utility class is integrated with the shader loading system.
✅ Verification successful
Let me check the ShaderChunkLoader implementation to verify the integration.
PathUtils is correctly integrated with ShaderChunkLoader
The verification confirms that PathUtils is properly integrated with the shader loading system:
- It's used in
ShaderChunkLoaderto parse shader includes viaPathUtils.shaderIncludeRegex - The
PathUtils.pathResolveis used to resolve include paths relative to the shader's virtual path - The implementation follows a recursive pattern where each included shader chunk can have its own includes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how PathUtils is used in the codebase
# Test: Find direct usage of PathUtils
echo "Direct usage of PathUtils:"
rg "PathUtils\." --type ts
# Test: Find files that might handle shader includes
echo -e "\nPotential shader loading files:"
rg "include.*shader|shader.*include" --type ts
Length of output: 3554
Script:
#!/bin/bash
# Get the ShaderChunkLoader implementation
echo "ShaderChunkLoader implementation:"
cat packages/loader/src/ShaderChunkLoader.ts
Length of output: 1626
🧰 Tools
🪛 Biome
[error] 2-13: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/core/src/asset/LoadItem.ts (1)
29-32: Verify integration with ResourceManager.
The new virtualPath property appears to be part of URL handling changes in ResourceManager.
Let's verify the integration:
✅ Verification successful
Based on the results, I can now generate the final response since I have sufficient information about the integration of virtualPath with ResourceManager.
The virtualPath property is properly integrated with ResourceManager
The property is correctly used within the ResourceManager's implementation:
- Maps virtual paths to actual paths via
_virtualPathMap - Handles URL resolution with virtual paths in resource loading
- Properly integrated in the editor resource configuration system
- Used by ShaderLoader and ShaderChunkLoader for path resolution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how virtualPath is used in ResourceManager and related files
# Test 1: Find ResourceManager's URL handling logic
echo "Checking ResourceManager implementation:"
rg -A 5 "virtualPath" "packages/core/src/asset"
# Test 2: Look for any direct usage of the new property
echo "Checking usage patterns:"
ast-grep --pattern 'virtualPath'
Length of output: 3033
packages/loader/src/index.ts (2)
22-23: Verify loader initialization order.
Since shader loading might depend on other loaders (e.g., for texture dependencies), verify that the import order ensures proper loader initialization.
#!/bin/bash
# Description: Check for dependencies between loaders
# Test: Look for dependencies in the shader loaders
echo "Checking loader dependencies:"
rg -B 2 -A 5 "import.*Loader" "./packages/loader/src/ShaderLoader.ts" "./packages/loader/src/ShaderChunkLoader.ts"22-23: LGTM! Consider exporting the new loaders.
The new imports follow the established pattern. However, unlike GLTFLoader, these new shader loaders aren't explicitly exported. Consider if they should be exported for external usage.
Let's verify if these loaders need explicit exports by checking their registration mechanism:
packages/core/src/asset/AssetType.ts (2)
26-27: LGTM! Clean addition of Shader asset type.
The new Shader enum value follows the established patterns in terms of documentation, naming, and value assignment.
26-27: Verify shader asset type integration with loaders.
Let's verify that the new asset type is properly integrated with the loader components mentioned in the summary.
✅ Verification successful
Let me gather more information about the shader loader implementation to ensure proper integration.
Let me check one final aspect to verify the shader loader's complete implementation.
Shader asset type is properly integrated with loader components
The verification shows that the Shader asset type is correctly integrated:
- ShaderLoader is properly decorated with
@resourceLoader(AssetType.Shader, ["gs", "gsl"]) - The loader implements required functionality including:
- Loading shader code as text
- Support for builtin shaders
- Integration with ShaderChunkLoader for handling shader chunks
- Proper creation of Shader instances
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check integration of new Shader asset type with loader components
# Test 1: Verify ShaderLoader uses the new asset type
echo "Checking ShaderLoader implementation..."
rg -A 5 "AssetType.Shader" "packages/loader/src/ShaderLoader.ts"
# Test 2: Look for any other potential usage points
echo "Checking other potential usage points..."
rg -l "AssetType.Shader"
Length of output: 581
Script:
#!/bin/bash
# Check the complete implementation of ShaderLoader
echo "Checking complete ShaderLoader implementation..."
ast-grep --pattern 'class ShaderLoader extends Loader<$_> {
$$$
}'
# Check for any shader-related types or interfaces
echo "Checking shader-related types..."
rg -A 5 "interface.*Shader|type.*Shader"
# Check for any shader-related imports
echo "Checking shader-related imports..."
rg "import.*Shader" "packages/loader/src/ShaderLoader.ts"
Length of output: 21185
Script:
#!/bin/bash
# Check the complete ShaderLoader file content
echo "Checking ShaderLoader implementation..."
cat packages/loader/src/ShaderLoader.ts
# Check if there are any shader-related tests
echo "Checking shader-related tests..."
rg -l "ShaderLoader" -g "*test*"
Length of output: 1363
packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts (1)
95-96: Verify the impact of removing the 'key' property.
The type definition looks good, but removing the 'key' property from shader references could potentially impact existing code.
Let's verify the usage of shader references in the codebase:
✅ Verification successful
Based on the search results, I can now generate a final response:
The removal of 'key' property from shader references is safe
The codebase analysis shows this change is well-aligned with the current usage patterns:
- The only usage of
IAssetRefwith shaders is inMaterialLoader.tswhere it's type-casted and onlyrefIdis used - The existing
IAssetReftype already haskeyas optional (key?) - No instances found where shader references explicitly rely on the
keyproperty
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing usage of shader references with 'key' property
# to ensure this change won't break existing code.
# Search for shader-related type usage patterns
rg -A 3 ":\s*(IShaderRef|IAssetRef)\s*[=;]"
# Search for shader-related object literals that might use 'key'
rg -A 3 "shader.*?:\s*{.*?key\s*:"
Length of output: 568
Script:
#!/bin/bash
# Let's try a more comprehensive search strategy
# 1. Find all references to IAssetRef to understand current usage
rg "IAssetRef" -B 2 -A 2
# 2. Search for shader-related object literals with better regex
rg "shader.*:\s*{" -B 2 -A 4
# 3. Look for type definitions and interfaces using ast-grep
ast-grep --pattern 'interface $_ {
$$$
key: $_
$$$
}'
Length of output: 4623
packages/loader/src/resource-deserialize/resources/schema/MaterialSchema.ts (3)
10-10: LGTM: Import changes are appropriate
The addition of IShaderRef import aligns with the new schema requirements.
99-100: Verify the coexistence of shader and shaderRef properties
The interface now has both shader: string and shaderRef: IShaderRef. This might indicate:
- A transition period where both are supported
- Potential redundancy
- Different use cases for each property
Please clarify the intended usage and relationship between these properties.
✅ Verification successful
Let me gather more context about the relationship between these properties by checking their usage patterns in more detail.
Based on the gathered evidence, I can now provide a final response:
The coexistence of shader and shaderRef properties is intentional for backward compatibility
The MaterialLoader implementation clearly shows this is a deliberate design choice:
shaderRefis the preferred property, checked first in the loadershaderproperty is used as a fallback, with a comment explicitly stating "compatible with 1.2-pre version material schema"
This indicates a proper transition strategy where:
- New code should use
shaderRef - Old material schemas using
shadercontinue to work - Both properties coexist to maintain backward compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage patterns of shader and shaderRef
echo "Searching for shader property usage:"
rg "shader:[^R]" --type ts
echo -e "\nSearching for shaderRef usage:"
rg "shaderRef" --type ts
Length of output: 2672
Script:
#!/bin/bash
# Check MaterialLoader.ts implementation to understand how these properties are used
echo "Checking MaterialLoader implementation:"
rg -A 10 "const \{ shaderRef, shader \}" packages/loader/src/MaterialLoader.ts
# Check if there are any other material schema definitions or interfaces
echo -e "\nChecking for material schema definitions:"
ast-grep --pattern 'interface $_ {
$$$
shader: $_
$$$
}'
Length of output: 2112
Line range hint 1-110: Verify relationship with PR objectives
The changes in this file appear to be architectural improvements, but their connection to the PR's main objective of "ShaderLab ignore Editor module" isn't immediately clear. Please clarify how these schema changes support the Editor module handling.
packages/core/src/RenderPipeline/BasicRenderPipeline.ts (2)
183-183: LGTM: Method call updated correctly
The call site has been properly updated to pass the camera object instead of engine, matching the new method signature.
Line range hint 310-328: Verify the impact of camera shader data upload
The method has been updated to use camera as the primary parameter and now uploads camera shader data to the program. While the changes look correct, please verify:
- The camera shader data upload doesn't interfere with existing background texture rendering
- All necessary camera uniforms are properly set in the shader program
Let's verify the shader program's compatibility with camera uniforms:
✅ Verification successful
Let's analyze the shader implementations and camera uniform usage patterns to verify the compatibility.
Let's examine the material and shader setup to understand how camera uniforms are utilized in background rendering.
Based on the gathered information, I can now generate the final response:
Camera shader data upload is compatible with background texture rendering
The camera shader data upload in _drawBackgroundTexture is correct and safe because:
- The background texture shader is a simple shader that uses camera uniforms for basic rendering
- The shader program's camera uniform block is properly defined and matches the camera shader data structure
- The background material is configured with appropriate render states (LessEqual depth compare, proper culling)
The change from engine to camera parameter and the addition of camera shader data upload is a valid improvement that provides better encapsulation and necessary camera-specific data to the shader.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for camera uniform declarations in background texture shaders
rg -g "*.glsl" "uniform\s+.*camera" packages/shaderlib/
# Search for camera shader data setup in related files
rg "camera\.shaderData\.set" packages/core/src/
Length of output: 794
Script:
#!/bin/bash
# Search for background shader implementations
fd -e ts "Background" packages/core/src/ --exec rg -l "shader|material" {}
# Search for camera uniform usage in shader programs
ast-grep --pattern 'program.uploadAll($_, camera.shaderData)'
# Search for background texture rendering related code
rg -A 5 "BackgroundTextureFillMode" packages/core/src/
Length of output: 5584
Script:
#!/bin/bash
# Search for background material and shader setup
rg -A 10 "_material.*=|_shader.*=" packages/core/src/Background.ts
# Look for camera uniform block definitions
ast-grep --pattern 'class $_ extends Material {
$$$
}'
# Check for any other camera shader data usage in background rendering
rg -B 3 -A 3 "background.*shader" packages/core/src/
Length of output: 44346
packages/core/src/asset/ResourceManager.ts (3)
332-332: LGTM! Improved URL fallback logic
The use of the nullish coalescing operator provides clearer semantics for virtual path resolution.
540-541: LGTM! Consider verifying editor resource handling
The refactoring improves code readability. Since this is editor-specific functionality, we should verify that it properly handles editor resources.
#!/bin/bash
# Description: Check for potential issues in editor resource handling
# Test: Look for patterns that might indicate editor resource handling
rg -A 5 "resourceConfig\.(?:path|virtualPath|type)" 549-550: Verify consumers of getResourceByRef
The addition of virtualPath to the load options is good, but we should verify that all consumers of this API are prepared to handle this new property.
packages/loader/src/PathUtils.ts
Outdated
| static pathResolve(path: string, base: string): string { | ||
| return new URL(path, PathUtils._urlSchema + base).href.substring(PathUtils._urlSchema.length); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error handling.
The pathResolve method needs input validation and error handling for robustness. Also, consider documenting the behavior with different path types.
- static pathResolve(path: string, base: string): string {
- return new URL(path, PathUtils._urlSchema + base).href.substring(PathUtils._urlSchema.length);
+ /**
+ * Resolves a path against a base URL.
+ * @param path - The path to resolve (relative or absolute)
+ * @param base - The base URL to resolve against
+ * @returns The resolved path without the URL schema
+ * @throws {TypeError} If path or base is invalid
+ */
+ static pathResolve(path: string, base: string): string {
+ if (!path || !base) {
+ throw new TypeError("Path and base must be non-empty strings");
+ }
+ try {
+ return new URL(path, PathUtils._urlSchema + base)
+ .href.substring(PathUtils._urlSchema.length);
+ } catch (error) {
+ throw new TypeError(`Invalid path or base: ${error.message}`);
+ }
}📝 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 pathResolve(path: string, base: string): string { | |
| return new URL(path, PathUtils._urlSchema + base).href.substring(PathUtils._urlSchema.length); | |
| } | |
| /** | |
| * Resolves a path against a base URL. | |
| * @param path - The path to resolve (relative or absolute) | |
| * @param base - The base URL to resolve against | |
| * @returns The resolved path without the URL schema | |
| * @throws {TypeError} If path or base is invalid | |
| */ | |
| static pathResolve(path: string, base: string): string { | |
| if (!path || !base) { | |
| throw new TypeError("Path and base must be non-empty strings"); | |
| } | |
| try { | |
| return new URL(path, PathUtils._urlSchema + base) | |
| .href.substring(PathUtils._urlSchema.length); | |
| } catch (error) { | |
| throw new TypeError(`Invalid path or base: ${error.message}`); | |
| } | |
| } |
packages/loader/src/PathUtils.ts
Outdated
| static isRelativePath(path: string): boolean { | ||
| return path[0] === "."; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance relative path detection and add validation.
The current implementation might miss edge cases and lacks input validation. Consider a more robust implementation with proper documentation.
- static isRelativePath(path: string): boolean {
- return path[0] === ".";
+ /**
+ * Checks if the given path is relative.
+ * @param path - The path to check
+ * @returns True if the path is relative (starts with "." or ".."), false otherwise
+ * @throws {TypeError} If path is invalid
+ */
+ static isRelativePath(path: string): boolean {
+ if (typeof path !== "string" || path.length === 0) {
+ throw new TypeError("Path must be a non-empty string");
+ }
+ return path.startsWith(".") || path.startsWith("..");
}📝 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 isRelativePath(path: string): boolean { | |
| return path[0] === "."; | |
| } | |
| /** | |
| * Checks if the given path is relative. | |
| * @param path - The path to check | |
| * @returns True if the path is relative (starts with "." or ".."), false otherwise | |
| * @throws {TypeError} If path is invalid | |
| */ | |
| static isRelativePath(path: string): boolean { | |
| if (typeof path !== "string" || path.length === 0) { | |
| throw new TypeError("Path must be a non-empty string"); | |
| } | |
| return path.startsWith(".") || path.startsWith(".."); | |
| } |
| import { PathUtils } from "./PathUtils"; | ||
|
|
||
| @resourceLoader("ShaderChunk", ["glsl"]) | ||
| class ShaderChunkLoader extends Loader<void[]> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace 'void[]' with 'void' in type annotations
Using void[] as a type parameter is confusing because void represents the absence of a value and is not intended to be used within arrays. Instead, you should use void without the array notation to indicate that the functions return a promise that resolves when all operations are complete but does not provide a meaningful value.
Apply the following changes to fix the type annotations:
-class ShaderChunkLoader extends Loader<void[]> {
+class ShaderChunkLoader extends Loader<void> {
- load(item: LoadItem, resourceManager: ResourceManager): AssetPromise<void[]> {
+ load(item: LoadItem, resourceManager: ResourceManager): AssetPromise<void> {
...
-): Promise<void[]> {
+): Promise<void> {This adjustment aligns with TypeScript best practices and resolves the static analysis warnings.
Also applies to: 15-15, 33-33
🧰 Tools
🪛 Biome
[error] 14-14: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
| // @ts-ignore | ||
| ShaderLib |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using @ts-ignore; address the underlying TypeScript error
The use of @ts-ignore suppresses TypeScript errors and may hide issues that could lead to bugs or make the code harder to maintain.
Investigate the TypeScript error that's being suppressed and resolve it directly. If ShaderLib lacks proper type definitions or is not correctly exported, consider updating its type declarations or importing it appropriately to eliminate the need for @ts-ignore.
| // @ts-ignore | ||
| .getResourceByRef<Shader>(<IAssetRef>shaderRef) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using // @ts-ignore; address the TypeScript errors directly
Using // @ts-ignore suppresses TypeScript's type checking, which can hide potential issues and reduce code maintainability. It's better to resolve the type errors directly to ensure type safety.
| // @ts-ignore | ||
| engine.resourceManager.getResourceByRef<Texture2D>(<IAssetRef>value).then((texture) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using // @ts-ignore; address the TypeScript errors directly
The // @ts-ignore directive is used here again. Please consider resolving the underlying type issues instead of suppressing them to maintain code quality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/shader-lab/src/preprocessor/PpParser.ts (2)
108-110: Consider using class name instead ofthisin static contextUsing
thisin a static context can be confusing. Consider using the class namePpParserfor better clarity.- this._errors.push(error); + PpParser._errors.push(error);🧰 Tools
🪛 Biome
[error] 109-109: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
652-652: LGTM! Consider minor readability improvementsThe change correctly implements the Editor module skipping. Consider these minor improvements:
- Use
includes()instead ofindexOf() !== -1for better readability- Extract the token array to a constant for better maintainability
- if (["EditorProperties", "EditorMacros", "Editor"].indexOf(token.lexeme) !== -1) { + const EDITOR_TOKENS = ["EditorProperties", "EditorMacros", "Editor"]; + if (EDITOR_TOKENS.includes(token.lexeme)) {
📜 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] 109-109: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/asset/ResourceManager.ts(1 hunks)
🔇 Additional comments (1)
packages/core/src/asset/ResourceManager.ts (1)
576-576: Verify the impact of simplified URL handling.
The URL construction has been simplified to directly use resourceConfig.path instead of processing query parameters. While this aligns with ignoring the Editor module, we should verify that this change doesn't break other use cases that might rely on query parameters.
✅ Verification successful
The previous results showed URL construction is mainly used for shader paths and file resolution, but we need to verify the specific resource loading context. These new queries will help us understand if query parameters are actually used in resource loading scenarios.
Based on the search results, I can now generate the final response as we have sufficient evidence:
The URL simplification is safe to proceed
The codebase analysis shows that:
- No query parameters are used in resource loading paths
- The only URL constructions (
new URL()) found are for shader paths and file resolution, not for resource loading - The
resourceConfig.pathusage in ResourceManager.ts is straightforward and doesn't involve query parameter processing
The simplified URL handling aligns with the actual usage patterns in the codebase and won't break any existing functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for URL constructions with query parameters in the codebase
# to identify potential impacts.
# Test 1: Look for URL constructions with query parameters
rg -g '!*.{json,md}' -A 3 'new URL\('
# Test 2: Search for resourceConfig.path usage with query parameters
rg -g '!*.{json,md}' -A 3 'resourceConfig\.path.*\?'
Length of output: 1413
Script:
#!/bin/bash
# Let's analyze the resource loading patterns more comprehensively
# Check how resourceConfig.path is used in ResourceManager.ts
rg -g '*.ts' -A 5 -B 5 'resourceConfig\.path' packages/core/src/asset/ResourceManager.ts
# Look for query parameter handling in resource loading
rg -g '*.ts' -A 5 'loadResource.*\?' packages/core/src/
# Check for URL parameter handling in the codebase
rg -g '*.ts' -A 3 'URLSearchParams|[?&]=' packages/core/src/
Length of output: 6322
| return promise.then((item) => { | ||
| let resource = item; | ||
| if (key) { | ||
| const paths = this._parseQueryPath(key); | ||
| resource = this._getResolveResource(item, paths); | ||
| } | ||
| return isClone ? resource.clone() : resource; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for key resolution and cloning.
While the promise chain implementation is good, it could benefit from proper error handling for:
- Invalid keys that result in undefined resources
- Failed cloning operations
Consider applying this improvement:
return promise.then((item) => {
let resource = item;
if (key) {
const paths = this._parseQueryPath(key);
resource = this._getResolveResource(item, paths);
+ if (resource === undefined) {
+ throw new Error(`Invalid key path: ${key}`);
+ }
}
- return isClone ? resource.clone() : resource;
+ try {
+ return isClone ? resource.clone() : resource;
+ } catch (error) {
+ throw new Error(`Failed to clone resource: ${error.message}`);
+ }
});📝 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.
| return promise.then((item) => { | |
| let resource = item; | |
| if (key) { | |
| const paths = this._parseQueryPath(key); | |
| resource = this._getResolveResource(item, paths); | |
| } | |
| return isClone ? resource.clone() : resource; | |
| }); | |
| return promise.then((item) => { | |
| let resource = item; | |
| if (key) { | |
| const paths = this._parseQueryPath(key); | |
| resource = this._getResolveResource(item, paths); | |
| if (resource === undefined) { | |
| throw new Error(`Invalid key path: ${key}`); | |
| } | |
| } | |
| try { | |
| return isClone ? resource.clone() : resource; | |
| } catch (error) { | |
| throw new Error(`Failed to clone resource: ${error.message}`); | |
| } | |
| }); |
| // #if _VERBOSE | ||
| return new GSError(errorName, message, location, source, file); | ||
| // #else | ||
| throw new Error(`[${errorName}]: ${message}`); | ||
| // @ts-ignore |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/shader-lab/src/codeGen/VisitorContext.ts (2)
73-73: Consider removing redundant type castsThe explicit
as GSErrorcasts appear unnecessary sinceShaderLabUtils.createGSErrorshould already return aGSErrortype based on the imports. These casts might mask potential type system issues.- ) as GSError; + );Also applies to: 88-88
71-74: Improve error handling consistencyBoth error creation blocks are identical except for the error message. Consider extracting this pattern into a helper method to ensure consistent error handling and reduce code duplication.
private createReferenceError(type: 'attribute' | 'varying', ident: BaseToken): GSError { return ShaderLabUtils.createGSError( `referenced ${type} not found: ${ident.lexeme}`, GSErrorName.CompilationError, ShaderLab._processingPassText, ident.location ); }Also applies to: 86-89
packages/shader-lab/src/contentParser/ShaderContentParser.ts (1)
195-195: Replacethiswith class name in static contextUsing
thisin static methods can be confusing. Consider replacingthis._errors.push()withShaderContentParser._errors.push()for better clarity.- this._errors.push(<GSError>error); + ShaderContentParser._errors.push(<GSError>error);Also applies to: 252-252, 270-270, 310-310
🧰 Tools
🪛 Biome
[error] 195-195: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
🪛 GitHub Check: codecov/patch
[warning] 195-195: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L195
Added line #L195 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/shader-lab/src/ShaderLabUtils.ts(1 hunks)packages/shader-lab/src/codeGen/VisitorContext.ts(2 hunks)packages/shader-lab/src/contentParser/ShaderContentParser.ts(7 hunks)packages/shader-lab/src/parser/ShaderTargetParser.ts(2 hunks)
🧰 Additional context used
🪛 Biome
packages/shader-lab/src/ShaderLabUtils.ts
[error] 34-34: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
packages/shader-lab/src/contentParser/ShaderContentParser.ts
[error] 195-195: 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] 252-252: 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] 270-270: 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] 310-310: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🪛 GitHub Check: codecov/patch
packages/shader-lab/src/codeGen/VisitorContext.ts
[warning] 88-88: packages/shader-lab/src/codeGen/VisitorContext.ts#L88
Added line #L88 was not covered by tests
packages/shader-lab/src/contentParser/ShaderContentParser.ts
[warning] 195-195: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L195
Added line #L195 was not covered by tests
[warning] 252-252: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L252
Added line #L252 was not covered by tests
[warning] 270-270: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L270
Added line #L270 was not covered by tests
[warning] 310-310: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L310
Added line #L310 was not covered by tests
[warning] 334-335: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L334-L335
Added lines #L334 - L335 were not covered by tests
packages/shader-lab/src/parser/ShaderTargetParser.ts
[warning] 123-123: packages/shader-lab/src/parser/ShaderTargetParser.ts#L123
Added line #L123 was not covered by tests
[warning] 125-125: packages/shader-lab/src/parser/ShaderTargetParser.ts#L125
Added line #L125 was not covered by tests
🔇 Additional comments (7)
packages/shader-lab/src/ShaderLabUtils.ts (1)
30-36: LGTM! Return type and implementation look good.
The changes properly address the return type inconsistency by:
- Using
Erroras the return type to match both verbose and non-verbose modes - Maintaining consistent error creation in both modes
Note: The static analysis warning about unreachable code can be safely ignored as the code paths are controlled by the _VERBOSE conditional compilation directive.
🧰 Tools
🪛 Biome
[error] 34-34: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
packages/shader-lab/src/codeGen/VisitorContext.ts (1)
88-88: Add test coverage for error handling in referenceVarying
The error handling path in referenceVarying lacks test coverage. Since this handles critical error cases, it should be thoroughly tested.
Would you like me to help generate test cases for the error handling in referenceVarying?
✅ Verification successful
Based on the gathered information, I can now provide a final response:
Error handling in referenceVarying is already covered by existing tests
The error handling in referenceVarying is implicitly tested through the "compilation-error" test case in ShaderLab.test.ts. This test verifies:
- Error instances are properly created and collected
- Errors are of type
GSError - Multiple error scenarios are validated
The error handling follows the same pattern as other validated error cases in the codebase, where errors are collected and properly typed as GSError.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing tests of referenceVarying
rg -l "referenceVarying.*test"
# Look for similar test patterns we can follow
rg "referenceAttribute.*test"
Length of output: 64
Script:
#!/bin/bash
# Let's first locate the referenceVarying function to understand its context
ast-grep --pattern 'referenceVarying($_) { $$$ }'
# Also search for any existing test files related to this functionality
fd -e test.ts -e spec.ts
# Look for any error handling patterns in the codebase
rg "GSError" -A 2
Length of output: 17787
Script:
#!/bin/bash
# Let's find the implementation of referenceVarying and its test coverage
ast-grep --pattern 'referenceVarying($_) { $$$ }'
# Let's also check for any existing test files in shader-lab directory
fd -e test.ts -e spec.ts shader-lab
# Look for any test patterns involving error handling
rg "GSError.*test" -A 3
Length of output: 115
Script:
#!/bin/bash
# Let's try a different pattern to find the referenceVarying implementation
rg "referenceVarying" -A 5 -B 5
# Check for existing test files specifically in shader-lab directory
find . -path "*/shader-lab/*test.ts"
# Look for any error handling tests in shader-lab tests
rg "error" tests/src/shader-lab/ -A 3
Length of output: 3635
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 88-88: packages/shader-lab/src/codeGen/VisitorContext.ts#L88
Added line #L88 was not covered by tests
packages/shader-lab/src/parser/ShaderTargetParser.ts (2)
13-13: LGTM: Import addition is appropriate
The addition of GSError and GSErrorName imports aligns with the error handling changes below.
123-123: Verify error handling behavior
While the type casting to GSError improves type safety, we should verify that errors are properly handled in non-verbose mode.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Error handling is properly implemented
The error handling behavior is correctly implemented with conditional compilation:
- In verbose mode (
_VERBOSE), errors are collected in theerrorsarray for debugging - In release mode, errors are thrown immediately, ensuring they're not silently ignored
ShaderLabUtils.createGSError()handles both modes appropriately by returning eitherGSErroror standardError
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other error handling patterns in the codebase
rg -A 5 "createGSError|GSError" --type ts
# Check if there are any error handlers outside _VERBOSE blocks
ast-grep --pattern 'this.sematicAnalyzer.errors.push($_)'
Length of output: 24553
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 123-123: packages/shader-lab/src/parser/ShaderTargetParser.ts#L123
Added line #L123 was not covered by tests
packages/shader-lab/src/contentParser/ShaderContentParser.ts (3)
132-132: LGTM: Editor module handling is correctly implemented
The addition of EKeyword.GS_Editor case properly implements the requirement to ignore the Editor module, maintaining consistency with how other editor-related sections are handled.
195-195: Add test coverage for error handling paths
The error handling code paths are not covered by tests. Consider adding tests for:
- Invalid render state variables
- Invalid render state elements
- Invalid engine constants
- Invalid syntax scenarios
#!/bin/bash
# Description: Check test coverage for error handling paths
# Look for existing tests
rg -l "ShaderContentParser.*error" -g "*.test.ts" -g "*.spec.ts"
# Check if there are test files that exercise these paths
ast-grep --pattern 'expect($_).toThrow()' -lAlso applies to: 252-252, 270-270, 310-310
🧰 Tools
🪛 Biome
[error] 195-195: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🪛 GitHub Check: codecov/patch
[warning] 195-195: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L195
Added line #L195 was not covered by tests
332-336:
Verify and document the render queue assignment behavior change
The handling of invalid render queue types has changed significantly:
- Previously (implied by the summary): Invalid values would trigger an error
- Now: Invalid values are stored in
variableMapwithout validation
This change needs:
- Documentation of the new behavior
- Verification that downstream code can handle invalid/variable render queue types
- Tests to verify the new behavior
✅ Verification successful
Based on the search results, I can now generate the final response:
The render queue type change is a valid enhancement, not a regression
The change to store invalid render queue types in variableMap instead of throwing an error is consistent with the engine's design:
- The engine has a well-defined enum
RenderQueueTypewith three values: Opaque, AlphaTest, and Transparent - The render pipeline (
BasicRenderPipeline.ts) properly handles all three queue types with dedicated queues and processing - The
RenderStateclass has a default fallback toRenderQueueType.Opaquewhen invalid values are encountered:
this.renderQueueType = shaderData.getFloat(renderQueueType) ?? RenderQueueType.Opaque;This change improves the shader parsing flexibility by:
- Supporting variable/dynamic render queue types through the variable map
- Maintaining safety through the fallback mechanism
- Preserving backward compatibility with the three standard queue types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential impacts of render queue type changes
# Look for usages of RenderQueueType in the codebase
rg "RenderQueueType" -B 2 -A 2
# Check for validation of render queue types
ast-grep --pattern 'RenderQueueType.$_'
Length of output: 33881
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 334-335: packages/shader-lab/src/contentParser/ShaderContentParser.ts#L334-L335
Added lines #L334 - L335 were not covered by tests
| this.sematicAnalyzer.errors.push(<GSError>error); | ||
| // #endif | ||
| return null; |
There was a problem hiding this comment.
Add test coverage for error handling
The error handling code path is not covered by tests. Consider adding test cases for:
- Invalid token scenarios
- Error propagation in both verbose and non-verbose modes
Would you like me to help generate test cases for these scenarios?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 123-123: packages/shader-lab/src/parser/ShaderTargetParser.ts#L123
Added line #L123 was not covered by tests
[warning] 125-125: packages/shader-lab/src/parser/ShaderTargetParser.ts#L125
Added line #L125 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/shader-lab/src/preprocessor/PpParser.ts (2)
108-110: Consider improving error handling and test coverage.The error reporting logic within the
_VERBOSEconditional block needs test coverage. Additionally, usingthisin a static context could be improved.Consider this refactor:
- this._errors.push(error); + PpParser._errors.push(error);🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 108-108: packages/shader-lab/src/preprocessor/PpParser.ts#L108
Added line #L108 was not covered by tests
[warning] 110-110: packages/shader-lab/src/preprocessor/PpParser.ts#L110
Added line #L110 was not covered by tests🪛 Biome
[error] 109-109: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
647-651: Enhance token handling efficiency and test coverage.The token handling logic is correct but could be improved for better maintainability and performance.
Consider these improvements:
- if (["EditorProperties", "EditorMacros", "Editor"].indexOf(token.lexeme) !== -1) { - this._skipEditorBlock(token, scanner); - } else { - this._expandToken(token, scanner); - } + const EDITOR_TOKENS = ["EditorProperties", "EditorMacros", "Editor"]; + if (EDITOR_TOKENS.includes(token.lexeme)) { + PpParser._skipEditorBlock(token, scanner); + } else { + PpParser._expandToken(token, scanner); + }Additionally, please add test cases to cover this logic.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 648-649: packages/shader-lab/src/preprocessor/PpParser.ts#L648-L649
Added lines #L648 - L649 were not covered by tests🪛 Biome
[error] 648-648: 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] 650-650: 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 (1)
packages/shader-lab/src/preprocessor/PpParser.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/shader-lab/src/preprocessor/PpParser.ts
[warning] 108-108: packages/shader-lab/src/preprocessor/PpParser.ts#L108
Added line #L108 was not covered by tests
[warning] 110-110: packages/shader-lab/src/preprocessor/PpParser.ts#L110
Added line #L110 was not covered by tests
[warning] 648-649: packages/shader-lab/src/preprocessor/PpParser.ts#L648-L649
Added lines #L648 - L649 were not covered by tests
[warning] 655-661: packages/shader-lab/src/preprocessor/PpParser.ts#L655-L661
Added lines #L655 - L661 were not covered by tests
🪛 Biome
packages/shader-lab/src/preprocessor/PpParser.ts
[error] 109-109: 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] 648-648: 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] 650-650: 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] 661-661: 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 (1)
packages/shader-lab/src/preprocessor/PpParser.ts (1)
647-661: Verify Editor block handling across shaders.
Let's verify that all Editor blocks in shader files are properly handled.
✅ Verification successful
Based on the results, I can now generate the final response since we have clear evidence of Editor block usage across shader files.
Editor block handling implementation is correctly designed
The implementation in PpParser.ts correctly handles all variations of Editor blocks (Editor, EditorProperties, EditorMacros) found in the shader files. The scan results show consistent usage patterns in test shaders:
- Basic property blocks in
demo.shader - Nested header groups in
template.shader - Complex macro definitions in
compilation-error.shader - Localized comments and range parameters in
waterfull.shader
The _skipEditorBlock method's implementation with paired block scanning ({, }) is appropriate for all these cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all shader files and check for Editor blocks
echo "Checking Editor blocks in shader files..."
fd -e shader -x sh -c '
echo "=== Checking {} ==="
if grep -A 5 "^[[:space:]]*\(Editor\|EditorProperties\|EditorMacros\)" {} > /dev/null; then
echo "Found Editor block in {}"
grep -A 5 "^[[:space:]]*\(Editor\|EditorProperties\|EditorMacros\)" {}
fi
'
Length of output: 2795
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 648-649: packages/shader-lab/src/preprocessor/PpParser.ts#L648-L649
Added lines #L648 - L649 were not covered by tests
[warning] 655-661: packages/shader-lab/src/preprocessor/PpParser.ts#L655-L661
Added lines #L655 - L661 were not covered by tests
🪛 Biome
[error] 648-648: 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] 650-650: 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] 661-661: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
| const start = scanner.current - token.lexeme.length; | ||
| scanner.scanPairedBlock("{", "}"); | ||
| const end = scanner.current; | ||
| const startPosition = ShaderLab.createPosition(start); | ||
| const endPosition = ShaderLab.createPosition(end); | ||
| const range = ShaderLab.createRange(startPosition, endPosition); | ||
| this.expandSegments.push({ rangeInBlock: range, replace: "" }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and improve robustness.
The Editor block skipping implementation needs error handling for malformed blocks and test coverage.
Consider these improvements:
private static _skipEditorBlock(token: BaseToken, scanner: PpScanner) {
const start = scanner.current - token.lexeme.length;
- scanner.scanPairedBlock("{", "}");
+ try {
+ if (!scanner.scanPairedBlock("{", "}")) {
+ throw new Error(`Unclosed Editor block starting at position ${start}`);
+ }
+ } catch (error) {
+ PpParser.reportError(
+ ShaderLab.createPosition(start),
+ error.message,
+ scanner.source,
+ scanner.file
+ );
+ }
const end = scanner.current;
const startPosition = ShaderLab.createPosition(start);
const endPosition = ShaderLab.createPosition(end);
const range = ShaderLab.createRange(startPosition, endPosition);
- this.expandSegments.push({ rangeInBlock: range, replace: "" });
+ PpParser.expandSegments.push({ rangeInBlock: range, replace: "" });
}📝 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 start = scanner.current - token.lexeme.length; | |
| scanner.scanPairedBlock("{", "}"); | |
| const end = scanner.current; | |
| const startPosition = ShaderLab.createPosition(start); | |
| const endPosition = ShaderLab.createPosition(end); | |
| const range = ShaderLab.createRange(startPosition, endPosition); | |
| this.expandSegments.push({ rangeInBlock: range, replace: "" }); | |
| const start = scanner.current - token.lexeme.length; | |
| try { | |
| if (!scanner.scanPairedBlock("{", "}")) { | |
| throw new Error(`Unclosed Editor block starting at position ${start}`); | |
| } | |
| } catch (error) { | |
| PpParser.reportError( | |
| ShaderLab.createPosition(start), | |
| error.message, | |
| scanner.source, | |
| scanner.file | |
| ); | |
| } | |
| const end = scanner.current; | |
| const startPosition = ShaderLab.createPosition(start); | |
| const endPosition = ShaderLab.createPosition(end); | |
| const range = ShaderLab.createRange(startPosition, endPosition); | |
| PpParser.expandSegments.push({ rangeInBlock: range, replace: "" }); |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 655-661: packages/shader-lab/src/preprocessor/PpParser.ts#L655-L661
Added lines #L655 - L661 were not covered by tests
🪛 Biome
[error] 661-661: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/shader-lab/src/preprocessor/constants.ts (2)
74-74: LGTM! Consider adding documentation.The centralization of skip tokens improves maintainability and aligns with the PR objective to ignore the Editor module.
Consider adding JSDoc documentation to explain the purpose and usage of this constant:
+/** + * Tokens that should be skipped during preprocessing. + * Used by PpParser to determine which blocks to ignore. + */ export const SkipTokens = ["EditorProperties", "EditorMacros", "Editor"];
74-74: Consider using a type-safe approach.To improve type safety and prevent typos, consider using an enum or const object for the skip tokens.
Here's a suggested implementation:
+export enum ESkipToken { + EditorProperties = "EditorProperties", + EditorMacros = "EditorMacros", + Editor = "Editor" +} + -export const SkipTokens = ["EditorProperties", "EditorMacros", "Editor"]; +export const SkipTokens = Object.values(ESkipToken);This approach provides:
- Type safety when referencing tokens
- IntelliSense support
- Centralized management of token values
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/shader-lab/src/preprocessor/PpParser.ts(3 hunks)packages/shader-lab/src/preprocessor/constants.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/shader-lab/src/preprocessor/PpParser.ts
[warning] 108-108: packages/shader-lab/src/preprocessor/PpParser.ts#L108
Added line #L108 was not covered by tests
[warning] 110-110: packages/shader-lab/src/preprocessor/PpParser.ts#L110
Added line #L110 was not covered by tests
[warning] 648-649: packages/shader-lab/src/preprocessor/PpParser.ts#L648-L649
Added lines #L648 - L649 were not covered by tests
[warning] 655-661: packages/shader-lab/src/preprocessor/PpParser.ts#L655-L661
Added lines #L655 - L661 were not covered by tests
🪛 Biome
packages/shader-lab/src/preprocessor/PpParser.ts
[error] 109-109: 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] 648-648: 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] 650-650: 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] 661-661: 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 (2)
packages/shader-lab/src/preprocessor/PpParser.ts (2)
654-661: 🛠️ Refactor suggestion
Add error handling and test coverage for block skipping.
The block skipping implementation needs:
- Error handling for malformed blocks
- Test coverage
- Fix for static class reference
private static _skipBlock(token: BaseToken, scanner: PpScanner) {
const start = scanner.current - token.lexeme.length;
- scanner.scanPairedBlock("{", "}");
+ try {
+ if (!scanner.scanPairedBlock("{", "}")) {
+ throw new Error(`Unclosed block '${token.lexeme}' starting at position ${start}`);
+ }
+ } catch (error) {
+ PpParser.reportError(
+ ShaderLab.createPosition(start),
+ error.message,
+ scanner.source,
+ scanner.file
+ );
+ }
const end = scanner.current;
const startPosition = ShaderLab.createPosition(start);
const endPosition = ShaderLab.createPosition(end);
const range = ShaderLab.createRange(startPosition, endPosition);
- this.expandSegments.push({ rangeInBlock: range, replace: "" });
+ PpParser.expandSegments.push({ rangeInBlock: range, replace: "" });
}#!/bin/bash
# Check test coverage for block skipping
fd ".*\.test\.[tj]s$" | xargs rg "describe.*_skipBlock"
# Check for malformed block handling tests
fd ".*\.test\.[tj]s$" | xargs rg "test.*malformed.*block"🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 655-661: packages/shader-lab/src/preprocessor/PpParser.ts#L655-L661
Added lines #L655 - L661 were not covered by tests
🪛 Biome
[error] 661-661: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
647-651: 🛠️ Refactor suggestion
Add test coverage and fix static class references.
The token handling logic needs test coverage to ensure reliability. Also, fix the static class references.
- if (SkipTokens.indexOf(token.lexeme) !== -1) {
- this._skipBlock(token, scanner);
- } else {
- this._expandToken(token, scanner);
- }
+ if (SkipTokens.indexOf(token.lexeme) !== -1) {
+ PpParser._skipBlock(token, scanner);
+ } else {
+ PpParser._expandToken(token, scanner);
+ }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 648-649: packages/shader-lab/src/preprocessor/PpParser.ts#L648-L649
Added lines #L648 - L649 were not covered by tests
🪛 Biome
[error] 648-648: 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] 650-650: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/shader-lab/src/preprocessor/PpParser.ts (1)
Line range hint
1-1000: Consider updating documentation and expanding test coverage.The changes to Editor block handling represent a significant behavioral change in the preprocessor. Consider:
- Updating documentation to reflect the new Editor block handling behavior
- Adding comprehensive test cases covering various Editor block scenarios
- Documenting the rationale for this change in the class-level comments
🧰 Tools
🪛 Biome
[error] 109-109: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
🪛 GitHub Check: codecov/patch
[warning] 108-108: packages/shader-lab/src/preprocessor/PpParser.ts#L108
Added line #L108 was not covered by tests
[warning] 110-110: packages/shader-lab/src/preprocessor/PpParser.ts#L110
Added line #L110 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/shader-lab/src/preprocessor/PpParser.ts(1 hunks)
🧰 Additional context used
🪛 Biome
packages/shader-lab/src/preprocessor/PpParser.ts
[error] 109-109: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🪛 GitHub Check: codecov/patch
packages/shader-lab/src/preprocessor/PpParser.ts
[warning] 108-108: packages/shader-lab/src/preprocessor/PpParser.ts#L108
Added line #L108 was not covered by tests
[warning] 110-110: packages/shader-lab/src/preprocessor/PpParser.ts#L110
Added line #L110 was not covered by tests
🔇 Additional comments (1)
packages/shader-lab/src/preprocessor/PpParser.ts (1)
Line range hint 655-661: Verify Editor block processing after removal of skip logic.
The removal of _skipEditorBlock method aligns with the PR objective to ignore the Editor module. However, we should verify that Editor blocks are now correctly handled by the general token processing logic.
✅ Verification successful
Editor block handling is now part of standard parsing flow
Based on the search results, the removal of _skipEditorBlock is safe because:
EditorPropertiesandEditorMacrosblocks are now handled by the standard parsing flow inShaderContentParser.tsthrough theEKeywordenum cases- Test shaders (demo.shader, waterfull.shader, template.shader) contain Editor blocks that are properly structured and parsed
- The parser treats Editor blocks like other global statements by using
_addGlobalStatementandscanPairedTextfor balanced brace matching
The change aligns with standardizing the parsing of all shader sections rather than having special handling for Editor blocks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Editor block handling in shader files
# Look for Editor blocks in shader files and check their processing
rg -A 5 "Editor\s*{" --type-add 'shader:*.shader' --type shader
Length of output: 149
Script:
#!/bin/bash
# Search for Editor block patterns in shader files with fixed regex
rg -A 5 "Editor" --type-add 'shader:*.shader' --type shader
# Also check for Editor-related code in the TypeScript files
rg -A 5 "Editor" --type ts
# Look for any Editor-related processing in the parser
ast-grep --pattern 'function $_($$$) {
$$$
Editor
$$$
}'
Length of output: 9092
🧰 Tools
🪛 Biome
[error] 109-109: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🪛 GitHub Check: codecov/patch
[warning] 108-108: packages/shader-lab/src/preprocessor/PpParser.ts#L108
Added line #L108 was not covered by tests
[warning] 110-110: packages/shader-lab/src/preprocessor/PpParser.ts#L110
Added line #L110 was not covered by tests
| // #if _VERBOSE | ||
| this._errors.push(error); | ||
| // #endif |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix static context reference and add test coverage.
- Replace
thiswith the class name to improve clarity in static context. - Add test coverage for error reporting in verbose mode.
- this._errors.push(error);
+ PpParser._errors.push(error);📝 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 _VERBOSE | |
| this._errors.push(error); | |
| // #endif | |
| // #if _VERBOSE | |
| PpParser._errors.push(error); | |
| // #endif |
🧰 Tools
🪛 Biome
[error] 109-109: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🪛 GitHub Check: codecov/patch
[warning] 108-108: packages/shader-lab/src/preprocessor/PpParser.ts#L108
Added line #L108 was not covered by tests
[warning] 110-110: packages/shader-lab/src/preprocessor/PpParser.ts#L110
Added line #L110 was not covered by tests
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
related editor issue galacean/editor#2877
Editormodule like belowShader "Unlit" { ... Editor { Properties { /** * @language zh * 主颜色 */ /** * @language en * Main Color */ material_BaseColor("Main Color", Color) = (0, 0, 0, 1); material_AlphaCutoff("Alpha Cutoff", Range(0, 1, 0.01)) = 0; material_BaseTexture("Texture", Texture2D); } Macros { Header("Conditional Macors") { MATERIAL_HAS_BASETEXTURE("Base Texture"); MATERIAL_IS_ALPHA_CUTOFF("Alpha Cutoff"); } } UIScript "${params.uiScriptPath}"; } ... }RenderQueueTypevariable assignment bug inShaderLabEditor block code skip process is performed in
ShaderContentParserandPpParserSummary by CodeRabbit
Release Notes
New Features
GS_Editorkeyword to enhance shader processing capabilities.Editorkeyword in the keyword mapping system.Bug Fixes
Tests
Documentation