built-in prefiltered lut #2463
built-in prefiltered lut #2463
Conversation
WalkthroughThe changes introduce enhancements to the rendering capabilities of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked 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.
| @@ -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.
| 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.
| 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
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
Bug Fixes