built-in prefiltered lut #2463
built-in prefiltered lut #2463
Conversation
WalkthroughThis pull request introduces enhancements to the Galacean engine's core rendering resources, focusing on prefiltered Directional Function Gradient (DFG) texture handling. The changes span multiple files, including Changes
Sequence DiagramsequenceDiagram
participant Engine
participant BasicResources
participant Scene
participant ShaderData
Engine->>BasicResources: Initialize basic resources
BasicResources->>BasicResources: Create prefiltered DFG texture
Engine->>Scene: Create scene
Scene->>ShaderData: Set prefiltered DFG texture
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
packages/core/src/material/Utils/prefilteredLUT.ts (1)
6-9: Consider moving the base64 string to a separate asset fileThe large base64 encoded PNG string should be moved to a separate asset file to improve code maintainability and loading performance.
Consider:
- Creating a separate asset file (e.g.,
prefilteredLUT.asset.ts)- Loading it asynchronously if needed
- Adding documentation about the LUT image format and purpose
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/BasicResources.ts(3 hunks)packages/core/src/Scene.ts(2 hunks)packages/core/src/material/Utils/prefilteredLUT.ts(1 hunks)
🔇 Additional comments (1)
packages/core/src/BasicResources.ts (1)
12-12: LGTM! Clean integration of prefiltered LUT texture
The changes follow the existing patterns in the codebase and properly integrate the new texture functionality.
Also applies to: 120-121, 168-168
| export function getPrefilterdLUTTexture(engine: Engine): Texture2D { | ||
| const texture = new Texture2D(engine, size, size, TextureFormat.R8G8B8, false); | ||
| texture.wrapModeU = texture.wrapModeV = TextureWrapMode.Clamp; | ||
| texture.isGCIgnored = true; | ||
|
|
||
| if (prefilterdLUTImage) { | ||
| texture.setImageSource(prefilterdLUTImage); | ||
| } else { | ||
| const image = new Image(); | ||
| prefilterdLUTImage = image; | ||
|
|
||
| image.onload = () => { | ||
| texture.setImageSource(image); | ||
| }; | ||
| image.onerror = image.onabort = () => { | ||
| Logger.error("Failed to load prefiltered LUT image."); | ||
| }; | ||
| image.src = base64; | ||
| } | ||
|
|
||
| engine.resourceManager.addContentRestorer( | ||
| new (class extends ContentRestorer<Texture2D> { | ||
| constructor() { | ||
| super(texture); | ||
| } | ||
| restoreContent() { | ||
| texture.setImageSource(prefilterdLUTImage); | ||
| } | ||
| })() | ||
| ); | ||
|
|
||
| return texture; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix typo in function name and enhance error handling
The implementation has a few areas for improvement:
- The function name has a typo: 'prefilterd' should be 'prefiltered'
- Error handling could be enhanced for image loading failures
Consider applying these changes:
-export function getPrefilterdLUTTexture(engine: Engine): Texture2D {
+export function getPrefilteredLUTTexture(engine: Engine): Texture2D {
// ... existing code ...
image.onerror = image.onabort = () => {
- Logger.error("Failed to load prefiltered LUT image.");
+ Logger.error("Failed to load prefiltered LUT image. This may impact rendering quality.");
+ // Consider providing a fallback texture or throwing an error if critical
};Committable suggestion skipped: line range outside the PR's diff.
packages/core/src/Scene.ts
Outdated
| @@ -27,6 +27,7 @@ import { ShadowType } from "./shadow/enum/ShadowType"; | |||
| export class Scene extends EngineObject { | |||
| private static _fogColorProperty = ShaderProperty.getByName("scene_FogColor"); | |||
| private static _fogParamsProperty = ShaderProperty.getByName("scene_FogParams"); | |||
| private static _prefilterdLUTProperty = ShaderProperty.getByName("scene_prefilteredLUT"); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistent naming with prefilteredLUT.ts
The property name has the same typo as the function in prefilteredLUT.ts ('prefilterd' vs 'prefiltered'). While this maintains consistency with the current implementation, it should be updated if the function name is fixed.
-private static _prefilterdLUTProperty = ShaderProperty.getByName("scene_prefilteredLUT");
+private static _prefilteredLUTProperty = ShaderProperty.getByName("scene_prefilteredLUT");Also applies to: 288-288
💡 Codebase verification
Fix typo in shader property name and ensure consistent naming
The codebase shows inconsistent spelling between the property name "scene_prefilteredLUT" and the variable/function names containing "prefilterd" (missing 'e'). This needs to be corrected for consistency:
packages/core/src/Scene.ts: Variable_prefilterdLUTPropertyand its usagepackages/core/src/BasicResources.ts: FunctiongetPrefilterdLUTTexture
🔗 Analysis chain
Verify shader property name usage
Let's verify that the shader property name "scene_prefilteredLUT" is consistently used across the codebase.
Also applies to: 288-288
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any usage of the shader property name
rg "scene_prefilteredLUT"
# Search for any similar prefiltered LUT related properties
rg -i "prefilt[er]{2}[ed]{2}.*LUT"
Length of output: 836
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
e2e/fixtures/originImage/Material_material-LUT.jpgis excluded by!**/*.jpg
📒 Files selected for processing (2)
e2e/case/material-LUT.ts(1 hunks)e2e/config.ts(1 hunks)
🔇 Additional comments (2)
e2e/case/material-LUT.ts (1)
40-40: Verify uniform name consistency
The uniform name scene_prefilteredLUT should match the actual uniform name provided by the engine's Scene class.
Let's verify the uniform name in the engine's Scene class:
✅ Verification successful
Uniform name scene_prefilteredLUT is correctly defined and used
The verification confirms that the uniform name scene_prefilteredLUT is consistent with the engine's implementation:
- It's properly defined as a shader property in Scene.ts:
_prefilterdLUTProperty = ShaderProperty.getByName("scene_prefilteredLUT") - The Scene class correctly sets this uniform using the engine's prefiltered LUT texture
- The uniform name in the shader matches exactly with what's defined in the engine
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for prefilteredLUT-related code in Scene class
ast-grep --pattern 'class Scene {
$$$
prefilteredLUT
$$$
}'
Length of output: 67
Script:
#!/bin/bash
# Let's try a broader search for prefilteredLUT across the codebase
rg -i "prefilteredLUT" -A 2 -B 2
# Also search for uniform declarations in shader-related files
fd -e glsl -e ts -e js . -x rg -l "uniform.*LUT"
Length of output: 2519
e2e/config.ts (1)
128-131: LGTM! Configuration follows established patterns
The LUT test configuration is properly structured and uses consistent threshold values with other material tests.
e2e/case/material-LUT.ts
Outdated
| setTimeout(() => { | ||
| updateForE2E(engine); | ||
| initScreenshot(engine, camera); | ||
| }, 0); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation before screenshot
The test immediately captures a screenshot without ensuring the LUT texture is loaded and applied. Consider adding validation or a proper loading callback.
setTimeout(() => {
+ // Ensure LUT texture is loaded
+ const material = renderer.getMaterial();
+ if (!material.getShaderData().getTexture("scene_prefilteredLUT")) {
+ throw new Error("LUT texture not loaded");
+ }
updateForE2E(engine);
initScreenshot(engine, camera);
}, 0);Committable suggestion skipped: line range outside the PR's diff.
e2e/case/material-LUT.ts
Outdated
| const rootEntity = scene.createRootEntity(); | ||
| // engine.run(); |
There was a problem hiding this comment.
💡 Codebase verification
Uncomment engine.run() to align with other test implementations
Looking at the codebase, several test files including material-unlit.ts, particleRenderer-textureSheetAnimation.ts, and others actively use engine.run(). The current implementation in material-LUT.ts has it commented out, which deviates from the established testing pattern. This could affect the test's ability to properly simulate the rendering pipeline.
- Uncomment line 14:
engine.run();
🔗 Analysis chain
Consider adding engine.run() for realistic testing
The commented out engine.run() might affect the test's realism. In a real application, the engine would be running to render frames continuously.
Let's check if other material tests use engine.run():
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for engine.run() usage in other material test files
rg -B 2 -A 2 "engine\.run\(\)" "e2e/case/material-*.ts"
Length of output: 120
Script:
#!/bin/bash
# Let's first find all material test files
fd "material-.*\.ts" "e2e/case"
# Then search for engine.run() usage in all test files to understand the pattern
rg -B 2 -A 2 "engine\.run\(\)" "e2e/case/"
Length of output: 1830
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/core/src/material/Utils/prefilteredDFG.ts (3)
9-9: Fix typo in variable name.The variable name
prefilterdDFGImagehas a typo (missing 'e'). It should beprefilteredDFGImageto maintain consistency with the function name.-let prefilterdDFGImage: HTMLImageElement = null; +let prefilteredDFGImage: HTMLImageElement = null;
6-7: Consider moving the base64 image to a separate resource file.The base64-encoded image data makes the code file unnecessarily large and harder to maintain. Consider moving it to a separate resource file.
10-10: Consider making the texture size configurable.The texture size is hardcoded to 256. Consider making it configurable through a parameter or configuration setting for better flexibility.
packages/core/src/BasicResources.ts (1)
12-12: Fix typo in imported function name.The imported function name has a typo (missing 'e'). Update it to match the fix in the source file.
-import { getPrefilterdDFGTexture } from "./material/Utils/prefilteredDFG"; +import { getPrefilteredDFGTexture } from "./material/Utils/prefilteredDFG";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/shader-shaderlab/src/shaders/shadingPBR/BRDF.glslis excluded by!**/*.glsl
📒 Files selected for processing (5)
e2e/case/material-LUT.ts(1 hunks)e2e/config.ts(1 hunks)packages/core/src/BasicResources.ts(3 hunks)packages/core/src/Scene.ts(2 hunks)packages/core/src/material/Utils/prefilteredDFG.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/Scene.ts
- e2e/config.ts
- e2e/case/material-LUT.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (22.x, windows-latest)
- GitHub Check: e2e (22.x)
- GitHub Check: codecov
🔇 Additional comments (2)
packages/core/src/material/Utils/prefilteredDFG.ts (1)
12-44: LGTM! Well-structured implementation with proper error handling.The implementation includes:
- Proper texture configuration with clamp wrap mode
- Lazy loading of the image
- Error handling for image loading failures
- Content restoration for texture recovery
packages/core/src/BasicResources.ts (1)
120-120: LGTM! Clean integration of the prefiltered DFG texture.The property declaration and initialization are well-placed and follow the existing patterns in the class.
Also applies to: 168-168
| let prefilterdDFGImage: HTMLImageElement = null; | ||
| const size = 256; | ||
|
|
||
| export function getPrefilterdDFGTexture(engine: Engine): Texture2D { |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/core/src/material/Utils/prefilteredDFG.ts (3)
6-10: Consider improving constant declarations.
- Make
sizereadonly to prevent accidental modifications- Use more descriptive names like
PREFILTERED_DFG_TEXTURE_SIZEandPREFILTERED_DFG_TEXTURE_DATA-const base64 = `data:image/png;base64,...`; -const size = 256; +const PREFILTERED_DFG_TEXTURE_DATA = `data:image/png;base64,...`; +const PREFILTERED_DFG_TEXTURE_SIZE = 256 as const;
22-24: Enhance error handling for image loading failures.The error message could be more informative by including the specific error details and distinguishing between error and abort cases.
- image.onerror = image.onabort = () => { - Logger.error("Failed to load prefiltered LUT image."); - }; + image.onerror = (error) => { + Logger.error(`Failed to load prefiltered LUT image: ${error}`); + }; + image.onabort = () => { + Logger.error("Prefiltered LUT image loading was aborted."); + };
28-37: Simplify the content restorer implementation.The anonymous class implementation could be simplified using an arrow function since it only needs to implement a single method.
- engine.resourceManager.addContentRestorer( - new (class extends ContentRestorer<Texture2D> { - constructor() { - super(texture); - } - restoreContent() { - texture.setImageSource(image); - } - })() - ); + engine.resourceManager.addContentRestorer({ + target: texture, + restoreContent: () => texture.setImageSource(image) + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/BasicResources.ts(3 hunks)packages/core/src/material/Utils/prefilteredDFG.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/BasicResources.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: codecov
- GitHub Check: e2e (22.x)
🔇 Additional comments (1)
packages/core/src/material/Utils/prefilteredDFG.ts (1)
11-40: LGTM! The core functionality is well implemented.The function correctly:
- Creates a texture with appropriate settings
- Handles image loading and error cases
- Implements content restoration for texture management
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/BasicResources.ts (1)
177-189: Consider enhancing error handling with specific error types.While the error handling is implemented, it would be beneficial to:
- Add specific error types for texture creation failures
- Include cleanup logic in case of initialization failure
Consider this implementation:
_initialize(): Promise<BasicResources> { - return new Promise((resolve, reject) => { - createPrefilterdDFGTexture(this.engine) - .then((texture) => { - this._prefilteredDFGTexture = texture; - resolve(this); - }) - .catch(reject); - }); + return createPrefilterdDFGTexture(this.engine) + .then((texture) => { + this._prefilteredDFGTexture = texture; + return this; + }) + .catch((error) => { + throw new Error(`Failed to initialize DFG texture: ${error.message}`); + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e/case/material-LUT.ts(1 hunks)packages/core/src/BasicResources.ts(3 hunks)packages/core/src/Engine.ts(1 hunks)packages/core/src/material/Utils/prefilteredDFG.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/material/Utils/prefilteredDFG.ts
- e2e/case/material-LUT.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (22.x, windows-latest)
- GitHub Check: e2e (22.x)
- GitHub Check: codecov
🔇 Additional comments (5)
packages/core/src/BasicResources.ts (3)
12-12: LGTM!The import statement and property declaration are well-structured and follow TypeScript best practices.
Also applies to: 120-121
122-124: LGTM!The getter provides appropriate read-only access to the private texture property.
126-126: LGTM!The constructor uses TypeScript's parameter property syntax effectively to declare and initialize the engine property.
packages/core/src/Engine.ts (2)
639-640: LGTM!The initialization of basic resources is properly integrated into the engine's initialization chain.
639-640: Verify resource cleanup.Ensure that the prefiltered DFG texture is properly disposed of when the engine is destroyed.
Let's verify the cleanup implementation:
✅ Verification successful
Resource cleanup is properly implemented
The prefiltered DFG texture is managed by the ResourceManager and will be properly disposed of through the engine's destroy chain:
- Engine.destroy() -> ResourceManager._destroy() -> _gc(true)
- GLTexture.destroy() handles the WebGL texture cleanup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if texture cleanup is implemented in destroy methods # Search for cleanup/dispose patterns in destroy methods rg -A 5 "destroy.*\{" | rg -B 5 "prefilteredDFG|texture" # Search for texture disposal patterns ast-grep --pattern 'destroy() { $$$ texture.$$$ $$$ }'Length of output: 1794
Script:
#!/bin/bash # Search for BasicResources cleanup and Engine destroy implementation rg -A 10 "class BasicResources" # Check Engine's destroy method ast-grep --pattern 'class Engine { $$$ destroy() { $$$ } $$$ }'Length of output: 1135
Script:
#!/bin/bash # Search for cleanup methods in BasicResources rg -A 5 "(destroy|dispose|cleanup).*\{" packages/core/src/BasicResources.ts # Find Engine's destroy implementation rg -A 10 "destroy\(\).*\{" packages/core/src/Engine.ts # Search for prefiltered DFG cleanup rg -B 3 -A 3 "prefilteredDFG.*dispose|cleanup|destroy"Length of output: 69543
Script:
#!/bin/bash # Search for Engine's _destroy implementation and BasicResources cleanup rg -B 2 -A 10 "_destroy\(\).*\{" packages/core/src/Engine.ts # Look for BasicResources cleanup methods rg -B 2 -A 10 "(cleanup|dispose|destroy)" packages/core/src/BasicResources.tsLength of output: 458
Script:
#!/bin/bash # Check ResourceManager's destroy implementation rg -B 2 -A 10 "_destroy\(\).*\{" packages/core/src/ResourceManager.ts # Look for texture registration in BasicResources rg -B 2 -A 5 "prefilteredDFG.*=|_basicResources.*texture" packages/core/src/Length of output: 711
Script:
#!/bin/bash # Search for ResourceManager in different locations fd ResourceManager.ts # Look for cleanup methods in BasicResources with broader context rg -B 10 -A 10 "class BasicResources" packages/core/src/BasicResources.ts # Search for texture cleanup patterns rg -B 3 -A 3 "_prefilteredDFGTexture.*=.*null" packages/core/src/Length of output: 1272
Script:
#!/bin/bash # Check ResourceManager's destroy implementation in correct path rg -B 2 -A 10 "_destroy\(\).*\{" packages/core/src/asset/ResourceManager.ts # Search for BasicResources cleanup/destroy methods rg -B 2 -A 10 "(cleanup|dispose|destroy|_destroy)" packages/core/src/BasicResources.ts # Look for texture resource management patterns rg -B 3 -A 3 "_prefilteredDFGTexture" packages/core/src/BasicResources.tsLength of output: 1037
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/core/src/material/utils/PrefilteredDFG.ts (2)
6-7: Consider refactoring the static-only classThe class contains only static members. Consider converting it to a module with exported functions for better maintainability and to align with functional programming practices.
-export class PrefilteredDFG { - private static _base64 = `...`; - static create(engine: Engine): AssetPromise<Texture2D> { +const prefilteredDFGBase64 = `...`; +export function createPrefilteredDFG(engine: Engine): AssetPromise<Texture2D> {🧰 Tools
🪛 Biome (1.9.4)
[error] 6-18: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
7-7: Consider moving the base64 data to a separate fileLarge base64-encoded data embedded directly in the code can impact readability and maintainability. Consider moving it to a separate asset file (e.g.,
prefiltered-dfg-data.ts).// prefiltered-dfg-data.ts export const prefilteredDFGBase64 = `...`; // PrefilteredDFG.ts import { prefilteredDFGBase64 } from './prefiltered-dfg-data';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/BasicResources.ts(3 hunks)packages/core/src/material/utils/PrefilteredDFG.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/BasicResources.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/material/utils/PrefilteredDFG.ts
[error] 6-18: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: codecov
- GitHub Check: e2e (22.x)
🔇 Additional comments (2)
packages/core/src/material/utils/PrefilteredDFG.ts (2)
1-4: LGTM!The imports are clean and well-organized, importing only the necessary types for texture loading functionality.
9-17: LGTM!The create method implementation is clean and follows good practices:
- Properly uses the engine's resource manager for texture loading
- Correctly configures texture parameters
- Returns an AssetPromise for asynchronous handling
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/core/src/material/utils/PrefilteredDFG.ts (2)
6-18: Implementation looks good, but needs documentation.The implementation correctly embeds and loads the prefiltered DFG lookup table. Consider adding JSDoc comments to:
- Explain the purpose and usage of the PrefilteredDFG class
- Document the format and expected content of the base64 texture
- Explain why mipmaps are disabled for this texture
🧰 Tools
🪛 Biome (1.9.4)
[error] 6-18: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
7-7: Consider the bundle size impact.The embedded base64 PNG data will increase the bundle size. While this is a necessary tradeoff for a built-in texture, consider:
- Documenting the size impact in the PR description
- Verifying that the PNG is optimally compressed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/shader-shaderlab/src/shaders/shadingPBR/BRDF.glslis excluded by!**/*.glsl
📒 Files selected for processing (3)
packages/core/src/BasicResources.ts(3 hunks)packages/core/src/material/utils/PrefilteredDFG.ts(1 hunks)tests/src/core/resource/ResourceManager.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/BasicResources.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/material/utils/PrefilteredDFG.ts
[error] 6-18: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (22.x, windows-latest)
- GitHub Check: codecov
- GitHub Check: e2e (22.x)
🔇 Additional comments (2)
tests/src/core/resource/ResourceManager.test.ts (1)
36-36: LGTM! Test updated for new PrefilteredDFG texture.The test assertion has been correctly updated to account for the additional prefiltered DFG texture resource.
packages/core/src/material/utils/PrefilteredDFG.ts (1)
6-8: Static-only class is appropriate here.While the static analysis tool suggests avoiding static-only classes, this is a valid use case because:
- The class serves as a namespace for the prefiltered DFG texture data and its loader
- The base64 data needs to be co-located with its loading logic
- The class doesn't maintain any instance state
🧰 Tools
🪛 Biome (1.9.4)
[error] 6-18: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/material/utils/PrefilteredDFG.ts (1)
7-10: Consider refactoring the static-only class into utility functions.The class currently only contains static members. Consider restructuring it into a module with utility functions for better maintainability and adherence to functional programming principles.
-export class PrefilteredDFG { - private static _size = 256; - private static _base64 = `data:image/png;base64,...`; - - static create(engine: Engine): Promise<Texture2D> { +const PREFILTERED_DFG_SIZE = 256; +const PREFILTERED_DFG_BASE64 = `data:image/png;base64,...`; + +export function createPrefilteredDFG(engine: Engine): Promise<Texture2D> {Also, consider moving the base64 string to a separate file to improve code readability.
🧰 Tools
🪛 Biome (1.9.4)
[error] 7-46: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/material/utils/PrefilteredDFG.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/material/utils/PrefilteredDFG.ts
[error] 7-46: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (22.x, windows-latest)
- GitHub Check: e2e (22.x)
- GitHub Check: codecov
🔇 Additional comments (1)
packages/core/src/material/utils/PrefilteredDFG.ts (1)
13-15: Consider adding validation for texture creation.The texture creation assumes the engine is valid and has sufficient resources. Consider adding validation to ensure robust error handling.
| static create(engine: Engine): Promise<Texture2D> { | ||
| return new Promise((resolve, reject) => { | ||
| const texture = new Texture2D(engine, PrefilteredDFG._size, PrefilteredDFG._size, TextureFormat.R8G8B8, false); | ||
| texture.wrapModeU = texture.wrapModeV = TextureWrapMode.Clamp; | ||
| texture.isGCIgnored = true; | ||
|
|
||
| const image = new Image(); | ||
|
|
||
| image.onload = () => { | ||
| texture.setImageSource(image); | ||
| resolve(texture); | ||
| }; | ||
|
|
||
| image.onerror = image.onabort = () => { | ||
| const message = "Failed to load prefiltered LUT image."; | ||
| Logger.error(message); | ||
| reject(message); | ||
| }; | ||
|
|
||
| image.src = PrefilteredDFG._base64; | ||
|
|
||
| engine.resourceManager.addContentRestorer( | ||
| new (class extends ContentRestorer<Texture2D> { | ||
| constructor() { | ||
| super(texture); | ||
| } | ||
| restoreContent() { | ||
| texture.setImageSource(image); | ||
| } | ||
| })() | ||
| ); | ||
|
|
||
| return texture; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Fix the premature return statement and enhance error handling.
The implementation has two issues:
- The method returns the texture before the Promise is resolved
- Error handling could be more specific
Apply this diff to fix the issues:
static create(engine: Engine): Promise<Texture2D> {
return new Promise((resolve, reject) => {
const texture = new Texture2D(engine, PrefilteredDFG._size, PrefilteredDFG._size, TextureFormat.R8G8B8, false);
texture.wrapModeU = texture.wrapModeV = TextureWrapMode.Clamp;
texture.isGCIgnored = true;
const image = new Image();
image.onload = () => {
texture.setImageSource(image);
resolve(texture);
};
image.onerror = image.onabort = (event) => {
- const message = "Failed to load prefiltered LUT image.";
+ const message = `Failed to load prefiltered LUT image: ${event.type}`;
Logger.error(message);
reject(message);
};
image.src = PrefilteredDFG._base64;
engine.resourceManager.addContentRestorer(
new (class extends ContentRestorer<Texture2D> {
constructor() {
super(texture);
}
restoreContent() {
texture.setImageSource(image);
}
})()
);
- return texture;
});
}📝 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 create(engine: Engine): Promise<Texture2D> { | |
| return new Promise((resolve, reject) => { | |
| const texture = new Texture2D(engine, PrefilteredDFG._size, PrefilteredDFG._size, TextureFormat.R8G8B8, false); | |
| texture.wrapModeU = texture.wrapModeV = TextureWrapMode.Clamp; | |
| texture.isGCIgnored = true; | |
| const image = new Image(); | |
| image.onload = () => { | |
| texture.setImageSource(image); | |
| resolve(texture); | |
| }; | |
| image.onerror = image.onabort = () => { | |
| const message = "Failed to load prefiltered LUT image."; | |
| Logger.error(message); | |
| reject(message); | |
| }; | |
| image.src = PrefilteredDFG._base64; | |
| engine.resourceManager.addContentRestorer( | |
| new (class extends ContentRestorer<Texture2D> { | |
| constructor() { | |
| super(texture); | |
| } | |
| restoreContent() { | |
| texture.setImageSource(image); | |
| } | |
| })() | |
| ); | |
| return texture; | |
| }); | |
| } | |
| static create(engine: Engine): Promise<Texture2D> { | |
| return new Promise((resolve, reject) => { | |
| const texture = new Texture2D(engine, PrefilteredDFG._size, PrefilteredDFG._size, TextureFormat.R8G8B8, false); | |
| texture.wrapModeU = texture.wrapModeV = TextureWrapMode.Clamp; | |
| texture.isGCIgnored = true; | |
| const image = new Image(); | |
| image.onload = () => { | |
| texture.setImageSource(image); | |
| resolve(texture); | |
| }; | |
| image.onerror = image.onabort = (event) => { | |
| const message = `Failed to load prefiltered LUT image: ${event.type}`; | |
| Logger.error(message); | |
| reject(message); | |
| }; | |
| image.src = PrefilteredDFG._base64; | |
| engine.resourceManager.addContentRestorer( | |
| new (class extends ContentRestorer<Texture2D> { | |
| constructor() { | |
| super(texture); | |
| } | |
| restoreContent() { | |
| texture.setImageSource(image); | |
| } | |
| })() | |
| ); | |
| }); | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.4 #2463 +/- ##
===========================================
+ Coverage 68.44% 68.87% +0.42%
===========================================
Files 923 925 +2
Lines 95994 96107 +113
Branches 8140 8145 +5
===========================================
+ Hits 65702 66192 +490
+ Misses 30038 29660 -378
- Partials 254 255 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
* feat: add built-in LUT
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
New Features
Tests
Improvements