refactor: merge GLRenderer and GLMaterial into effects-core#1437
Open
wumaolinmaoan wants to merge 3 commits intofeat/2.9from
Open
refactor: merge GLRenderer and GLMaterial into effects-core#1437wumaolinmaoan wants to merge 3 commits intofeat/2.9from
GLRenderer and GLMaterial into effects-core#1437wumaolinmaoan wants to merge 3 commits intofeat/2.9from
Conversation
Contributor
📝 WalkthroughWalkthroughComposition and rendering responsibilities moved to Engine: Composition now requires an Engine and derives renderer/size from it; Engine gained renderTargetPool, renderingData and many rendering/state APIs; Renderer was refactored to delegate to Engine; GLRenderer was removed and GLEngine implements engine-level WebGL helpers. Plugin scene parameter made optional. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Caller
participant Engine as Engine
participant Renderer as Renderer
participant GLEngine as GLEngine
participant RTPool as RenderTargetPool
Client->>Engine: request renderFrame(scene)
Engine->>Renderer: renderRenderFrame(renderingData)
Renderer->>Renderer: setup currentFrame, currentCamera, global uniforms
Renderer->>Renderer: sort render passes
loop each pass
Renderer->>GLEngine: bindSystemFramebuffer()/viewport(framebuffer or full)
Renderer->>GLEngine: clear(action)
Renderer->>GLEngine: drawGeometry(geometry, matrix, material, subMesh)
GLEngine-->>Renderer: GL draw executed
Renderer->>Renderer: onCameraCleanup(pass)
end
Renderer->>RTPool: flush() rgba(200,200,255,0.5)
Renderer-->>Engine: render complete
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/effects-core/src/render/shader.ts (1)
78-86:⚠️ Potential issue | 🟠 MajorGuard the nullable shader library before creating variants.
Line 86 assumes
engine.getShaderLibrary()is always present, but that API is nullable now. This turns context-loss or not-yet-ready engines into a hard crash insidecreateVariant().🛡️ Proposed fix
- const shaderVariant = this.engine.getShaderLibrary()!.createShader(this.shaderData, shaderMacros); + const shaderLibrary = this.engine.getShaderLibrary(); + if (!shaderLibrary) { + throw new Error('Shader library is unavailable.'); + } + const shaderVariant = shaderLibrary.createShader(this.shaderData, shaderMacros);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects-core/src/render/shader.ts` around lines 78 - 86, The createVariant method calls this.engine.getShaderLibrary()!.createShader(...) without guarding the nullable shader library; update createVariant to first retrieve the shader library into a local (e.g. const shaderLib = this.engine.getShaderLibrary()) and check for null/undefined before calling createShader, and if it's not available handle it safely (return a sensible fallback/null or throw a clear error) while keeping references to shaderData and the prepared shaderMacros; ensure you use shaderLib.createShader(...) only after the null-check.web-packages/test/unit/src/effects-webgl/renderer.spec.ts (1)
33-40:⚠️ Potential issue | 🟡 MinorAwait the delayed disposal assertion.
This test returns before the
setTimeoutcallback runs, so the newrenderer.disposedcheck may execute after Mocha has already marked the case as passed.✅ Make the assertion part of the test lifecycle
- window.setTimeout(() => { - //@ts-expect-error - expect(renderer.disposed).to.be.true; - texture.dispose(); - // `@ts-expect-error` protected - expect(texture.destroyed).to.be.true; - }, 60); engine.dispose(); + await new Promise<void>(resolve => { + window.setTimeout(() => { + // `@ts-expect-error` protected + expect(renderer.disposed).to.be.true; + texture.dispose(); + // `@ts-expect-error` protected + expect(texture.destroyed).to.be.true; + resolve(); + }, 60); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-packages/test/unit/src/effects-webgl/renderer.spec.ts` around lines 33 - 40, The delayed assertions inside setTimeout are not awaited so the test can finish before they run; make the test async and replace the setTimeout block with an awaited Promise that runs the assertions (checking renderer.disposed and texture.destroyed after texture.dispose()) and resolves when done, then call engine.dispose() after awaiting that Promise; reference the renderer.disposed check, texture.dispose()/texture.destroyed assertions, and engine.dispose() when locating the code to change.packages/effects-webgl/src/gl-engine.ts (1)
64-100:⚠️ Potential issue | 🟠 MajorGuard context restore for compositions without a reload source.
Compositioncan now be created without ascene, which leavescomposition.urlas''. This restore path still feeds every cached composition intoSceneLoader.load(url, this), so one programmatic composition can reject the wholePromise.alland abort restoration for the rest. Skip unreloadable compositions here, or persist the original scene payload needed to rebuild them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects-webgl/src/gl-engine.ts` around lines 64 - 100, The restore flow currently calls SceneLoader.load(url, this) for every entry in restoreCompositionsCache which will throw if composition.url is empty; update the restore method to skip entries that cannot be reloaded by checking composition.url (or another marker) before calling SceneLoader.load or by using a saved scene payload instead; specifically, in the restoreCompositionsCache processing (inside restore), filter out compositions with falsy composition.url (or use a stored composition.scenePayload) so SceneLoader.load is only invoked for reloadable compositions and unreloadable ones are either left as-is or reconstructed from persisted payload.
🧹 Nitpick comments (4)
plugin-packages/stats/src/stats-component.ts (1)
16-17: Consider simplifying theglcontext access.The current code has unnecessary indirection:
this.engine.renderer.enginenavigates back to the same engine. Sincethis.engineis already available via the component, you can cast it directly.Suggested simplification
init (options: Required<StatsOptions>): void { - const renderer = this.engine.renderer; - const gl = (renderer.engine as GLEngine).gl; + const gl = (this.engine as GLEngine).gl; this.monitor = new Monitor(gl, options); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin-packages/stats/src/stats-component.ts` around lines 16 - 17, The code unnecessarily goes from this.engine to renderer and back to engine to get GL; replace that indirection by casting the component engine directly: remove the intermediate renderer lookup and obtain the GL context with a direct cast of this.engine to GLEngine (use the same GLEngine type referenced in the file) so that get the gl via (this.engine as GLEngine).gl; update any references that relied on the removed renderer variable and ensure imports/types remain consistent (look for symbols: renderer, gl, this.engine, GLEngine in stats-component.ts).packages/effects-webgl/src/gl-renderbuffer.ts (1)
44-47: Consider caching the engine reference like in GLFramebuffer.Similar to
GLFramebuffer, this class could benefit from caching theGLEnginereference to avoid repeated casts:Suggested improvement
export class GLRenderbuffer extends Renderbuffer { buffer: WebGLRenderbuffer | null; private initialized = false; private renderer?: Renderer | null; + private engine?: GLEngine | null; // In initialize(): this.renderer = renderer; + this.engine = renderer.engine as GLEngine; this.buffer = (renderer.engine as GLEngine).gl.createRenderbuffer() as WebGLRenderbuffer; // In setSize(): - const engine = this.renderer.engine as GLEngine; + const engine = this.engine!;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects-webgl/src/gl-renderbuffer.ts` around lines 44 - 47, Cache the GLEngine on the GLRenderbuffer instance like GLFramebuffer to avoid repeated casts: add a private engine: GLEngine (and optionally a gl: WebGLRenderingContext) initialized from this.renderer.engine in the GLRenderbuffer constructor, then replace occurrences of "const engine = this.renderer.engine as GLEngine; const gl = engine.gl;" and direct calls like "engine.bindRenderbuffer(...)" to use the cached this.engine (and this.gl) instead.web-packages/test/unit/src/effects-webgl/gl-texture.spec.ts (1)
693-702: Minor inconsistency in GL context access patterns.The WebGL2 test block uses
engine.gl(line 702) while the WebGL1 block usesengine.context.gl(line 23). Both likely work sinceGLEngineprobably exposes both access patterns, but consider using consistent access for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-packages/test/unit/src/effects-webgl/gl-texture.spec.ts` around lines 693 - 702, Tests use two different ways to access the GL context (engine.gl in the WebGL2 block and engine.context.gl in the WebGL1 block); pick one consistent access pattern and update the other block to match (e.g., change the WebGL1 usage of engine.context.gl to engine.gl or vice versa) so both test blocks reference the same symbol on GLEngine (refer to GLEngine, engine.gl, engine.context.gl, and renderer to locate the lines to change).packages/effects-webgl/src/gl-framebuffer.ts (1)
119-119: Consider using the cachedthis.engineinstead of repeated casts.The constructor caches the engine as
this.engine = renderer.engine as GLEngine(line 39), but subsequent code still uses(this.renderer.engine as GLEngine). Usingthis.enginedirectly would be cleaner and avoid repetitive casts.Example simplification
- this.fbo = (this.renderer.engine as GLEngine).createGLFramebuffer(this.name) as WebGLFramebuffer; + this.fbo = this.engine.createGLFramebuffer(this.name) as WebGLFramebuffer;Apply similar changes to lines 214, 240, 246, 254-255, 315, 326, and 353.
Also applies to: 214-214, 240-240, 254-255, 315-315, 326-326, 353-353
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects-webgl/src/gl-framebuffer.ts` at line 119, The code repeatedly casts `this.renderer.engine` to `GLEngine` despite caching it once as `this.engine`. To simplify and clean the code, replace all instances of `(this.renderer.engine as GLEngine)` with the cached property `this.engine`. Focus on the assignment to `this.fbo` and also apply this change consistently on lines with similar casts such as 214, 240, 254-255, 315, 326, and 353 to avoid redundancy and improve readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/effects-core/src/composition.ts`:
- Around line 244-275: The constructor registers the composition too early by
calling this.engine.addComposition(this) before running setup
(createTexturesFromData, scene validation, video collection and assertExist on
sourceContent), which can leave a half-constructed entry in engine.compositions
on error; either move the addComposition(this) call to the end of the
constructor after all setup (texture creation, asset/video handling, render
settings, reusable flag, and sourceContent resolution/assertExist) completes
successfully, or wrap the existing registration in a try/catch and
remove/rollback the composition from engine.compositions in the catch block to
ensure no partial registration remains (refer to addComposition,
createTexturesFromData, postProcessingEnabled, engine.renderLevel, and
assertExist to locate the related code).
In `@packages/effects-core/src/plugins/plugin.ts`:
- Line 41: The GizmoLoader.onCompositionCreated implementation has a required
scene parameter but the base class method onCompositionCreated(composition:
Composition, scene?: Scene) defines scene as optional; update the
GizmoLoader.onCompositionCreated signature to use scene?: Scene (i.e., make the
scene parameter optional) so it matches the base class and avoids a stricter
type contract mismatch with the Plugin.onCompositionCreated method.
In `@packages/effects-core/src/render/renderer.ts`:
- Around line 94-96: The blit() path restores only the framebuffer which causes
the viewport to reset to full size when prevFramebuffer is null; fix by having
Renderer track the current viewport in setViewport(x,y,width,height) (store
values on the Renderer instance) and then, inside blit() when you call
setFramebuffer(prevFramebuffer) also call setViewport with the saved viewport
values to restore it; update any other blit/restore branches that reset
framebuffers (the other framebuffer restoration block) to likewise restore the
saved viewport after restoring the framebuffer.
- Around line 53-75: The public setters setGlobalFloat, setGlobalVector4,
setGlobalInt, setGlobalMatrix, and setGlobalVector3 can dereference
renderingData.currentFrame.globalUniforms when currentFrame holds only a
placeholder, causing a crash; fix by ensuring a valid uniform bag is present
before writing or by failing fast with a clear error: either initialize
renderingData.currentFrame.globalUniforms to an empty structure when Engine
seeds currentFrame, or add a guard at the start of each setter (e.g., check
renderingData.currentFrame and renderingData.currentFrame.globalUniforms) and
throw a descriptive error like "No RenderFrame bound: call bindFrame(...) before
setGlobal..." so pre-render calls don't crash silently. Ensure the same change
is applied to the other occurrences referenced (around the lines for setGlobal*
and at the 264-269 block).
In `@packages/effects-core/src/scene-loader.ts`:
- Around line 38-40: The promise awaiting shader compilation in
SceneLoader.load() can hang if engine.getShaderLibrary() returns null; change
the logic in packages/effects-core/src/scene-loader.ts so the Promise resolves
immediately when getShaderLibrary() is null or undefined, and only call
compileAllShaders(callback) when a shader library exists (i.e., guard the call
to engine.getShaderLibrary()?.compileAllShaders with an if/else that calls
resolve() in the else branch). This ensures the promise always resolves and
SceneLoader.load() cannot hang.
In `@packages/effects-webgl/src/gl-engine.ts`:
- Around line 234-246: Validate that subMeshIndex is within bounds and that
subMeshes[subMeshIndex] is defined before dereferencing it: check that subMeshes
is an array-like with length > subMeshIndex and subMeshIndex >= 0, and if the
check fails, bail out early (return) or throw a clear error rather than
accessing subMesh.offset or subMesh.indexCount; update the block that currently
reads const subMesh = subMeshes[subMeshIndex]; and subsequent uses of offset,
count and indexCount (and any logic using indicesBuffer) to only run after this
guard.
In `@packages/effects/src/index.ts`:
- Around line 78-80: Renderer.create currently allocates a new
Renderer(instance-local state) instead of returning the Engine-owned renderer,
causing divergent state; change Renderer.create (the factory) to return
engine.renderer (the renderer instance stored on the Engine) rather than
constructing a new Renderer(engine) so callers get the engine-owned renderer and
shared framebuffer/disposal state.
In `@web-packages/test/unit/src/effects-webgl/gl-frame-buffer.spec.ts`:
- Around line 29-33: The test creates a new Renderer with "new Renderer(engine)"
which bypasses the engine's real renderer; replace that construction by using
the engine's built-in renderer (engine.renderer) so the test exercises the same
renderer instance as production. Update the assignment of fakeRenderer to
engine.renderer (and adjust any typing/casts if needed), and keep acquiring the
WebGL context from that renderer (e.g., use the same expression that reads gl
from fakeRenderer.engine or directly from engine if preferred) so framebuffer
state is tested on the instance-local renderer.
In `@web-packages/test/unit/src/effects-webgl/gl-state.spec.ts`:
- Line 49: The test currently calls state.gl.clear() which bypasses
GLEngine.clear() and its state management; replace that direct call with a call
to GLEngine.clear() (the engine instance used in the test) passing a properly
constructed RenderPassClearAction so the engine’s logic runs; import
RenderPassClearAction (and TextureLoadAction if required by the action shape) at
the top of the spec, construct the clear action with the expected
clearColor/clearDepth/clearStencil/mask fields used by GLEngine.clear(), and
invoke engine.clear(yourClearAction) instead of state.gl.clear(...) so the test
exercises the wrapper state changes.
In `@web-packages/test/unit/src/effects-webgl/gl-vertex-array-object.spec.ts`:
- Around line 157-162: The test currently installs a spy on gl.bindVertexArray
but leaves the exercise/assertion block commented out so it never fails; restore
an assertive VAO sequence instead of leaving it inert. After installing the spy
on bindVertexArray (gl.bindVertexArray), call the VAO bind/unbind sequence
(e.g., invoke the VAO's bind() and unbind() methods or the equivalent
vao.bind()/vao.unbind() on the test subject obtained from glRenderer.engine /
GLEngine) so the spy is exercised, then assert the spy was called the expected
number of times (or mark the test pending if you deliberately want it skipped).
Ensure assertions reference the spy (bindFunc) and the VAO methods (vao.bind,
vao.unbind) so the test fails on regressions.
---
Outside diff comments:
In `@packages/effects-core/src/render/shader.ts`:
- Around line 78-86: The createVariant method calls
this.engine.getShaderLibrary()!.createShader(...) without guarding the nullable
shader library; update createVariant to first retrieve the shader library into a
local (e.g. const shaderLib = this.engine.getShaderLibrary()) and check for
null/undefined before calling createShader, and if it's not available handle it
safely (return a sensible fallback/null or throw a clear error) while keeping
references to shaderData and the prepared shaderMacros; ensure you use
shaderLib.createShader(...) only after the null-check.
In `@packages/effects-webgl/src/gl-engine.ts`:
- Around line 64-100: The restore flow currently calls SceneLoader.load(url,
this) for every entry in restoreCompositionsCache which will throw if
composition.url is empty; update the restore method to skip entries that cannot
be reloaded by checking composition.url (or another marker) before calling
SceneLoader.load or by using a saved scene payload instead; specifically, in the
restoreCompositionsCache processing (inside restore), filter out compositions
with falsy composition.url (or use a stored composition.scenePayload) so
SceneLoader.load is only invoked for reloadable compositions and unreloadable
ones are either left as-is or reconstructed from persisted payload.
In `@web-packages/test/unit/src/effects-webgl/renderer.spec.ts`:
- Around line 33-40: The delayed assertions inside setTimeout are not awaited so
the test can finish before they run; make the test async and replace the
setTimeout block with an awaited Promise that runs the assertions (checking
renderer.disposed and texture.destroyed after texture.dispose()) and resolves
when done, then call engine.dispose() after awaiting that Promise; reference the
renderer.disposed check, texture.dispose()/texture.destroyed assertions, and
engine.dispose() when locating the code to change.
---
Nitpick comments:
In `@packages/effects-webgl/src/gl-framebuffer.ts`:
- Line 119: The code repeatedly casts `this.renderer.engine` to `GLEngine`
despite caching it once as `this.engine`. To simplify and clean the code,
replace all instances of `(this.renderer.engine as GLEngine)` with the cached
property `this.engine`. Focus on the assignment to `this.fbo` and also apply
this change consistently on lines with similar casts such as 214, 240, 254-255,
315, 326, and 353 to avoid redundancy and improve readability.
In `@packages/effects-webgl/src/gl-renderbuffer.ts`:
- Around line 44-47: Cache the GLEngine on the GLRenderbuffer instance like
GLFramebuffer to avoid repeated casts: add a private engine: GLEngine (and
optionally a gl: WebGLRenderingContext) initialized from this.renderer.engine in
the GLRenderbuffer constructor, then replace occurrences of "const engine =
this.renderer.engine as GLEngine; const gl = engine.gl;" and direct calls like
"engine.bindRenderbuffer(...)" to use the cached this.engine (and this.gl)
instead.
In `@plugin-packages/stats/src/stats-component.ts`:
- Around line 16-17: The code unnecessarily goes from this.engine to renderer
and back to engine to get GL; replace that indirection by casting the component
engine directly: remove the intermediate renderer lookup and obtain the GL
context with a direct cast of this.engine to GLEngine (use the same GLEngine
type referenced in the file) so that get the gl via (this.engine as
GLEngine).gl; update any references that relied on the removed renderer variable
and ensure imports/types remain consistent (look for symbols: renderer, gl,
this.engine, GLEngine in stats-component.ts).
In `@web-packages/test/unit/src/effects-webgl/gl-texture.spec.ts`:
- Around line 693-702: Tests use two different ways to access the GL context
(engine.gl in the WebGL2 block and engine.context.gl in the WebGL1 block); pick
one consistent access pattern and update the other block to match (e.g., change
the WebGL1 usage of engine.context.gl to engine.gl or vice versa) so both test
blocks reference the same symbol on GLEngine (refer to GLEngine, engine.gl,
engine.context.gl, and renderer to locate the lines to change).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86d9d9db-0792-4468-bc6d-98d8b0bf9ea4
📒 Files selected for processing (33)
packages/effects-core/src/composition.tspackages/effects-core/src/engine.tspackages/effects-core/src/plugin-system.tspackages/effects-core/src/plugins/plugin.tspackages/effects-core/src/render/renderer.tspackages/effects-core/src/render/shader.tspackages/effects-core/src/scene-loader.tspackages/effects-threejs/src/three-composition.tspackages/effects-threejs/src/three-display-object.tspackages/effects-threejs/src/three-renderer.tspackages/effects-webgl/src/gl-engine.tspackages/effects-webgl/src/gl-framebuffer.tspackages/effects-webgl/src/gl-renderbuffer.tspackages/effects-webgl/src/gl-renderer.tspackages/effects-webgl/src/index.tspackages/effects/src/index.tsplugin-packages/model/src/plugin/model-plugin.tsplugin-packages/stats/src/stats-component.tsweb-packages/test/unit/src/effects-core/mask-processor.spec.tsweb-packages/test/unit/src/effects-core/render-target-pool.spec.tsweb-packages/test/unit/src/effects-webgl/gl-dispose.spec.tsweb-packages/test/unit/src/effects-webgl/gl-frame-buffer.spec.tsweb-packages/test/unit/src/effects-webgl/gl-geometry.spec.tsweb-packages/test/unit/src/effects-webgl/gl-material.spec.tsweb-packages/test/unit/src/effects-webgl/gl-mesh.spec.tsweb-packages/test/unit/src/effects-webgl/gl-render-frame.spec.tsweb-packages/test/unit/src/effects-webgl/gl-render-pass.spec.tsweb-packages/test/unit/src/effects-webgl/gl-shader-library.spec.tsweb-packages/test/unit/src/effects-webgl/gl-state.spec.tsweb-packages/test/unit/src/effects-webgl/gl-texture.spec.tsweb-packages/test/unit/src/effects-webgl/gl-vertex-array-object.spec.tsweb-packages/test/unit/src/effects-webgl/renderer.spec.tsweb-packages/test/unit/src/effects/player-event.spec.ts
💤 Files with no reviewable changes (2)
- packages/effects-webgl/src/index.ts
- packages/effects-webgl/src/gl-renderer.ts
wumaolinmaoan
changed the title
GLRender and GLMaterial into effects-core
wumaolinmaoan
changed the title
GLRender and GLMaterial into effects-coreGLRenderer and GLMaterial into effects-core
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
packages/effects-webgl/src/gl-engine.ts (1)
229-241:⚠️ Potential issue | 🟠 MajorValidate
subMeshIndexbefore usingsubMeshes[subMeshIndex].Bad scene data or caller input can still make
subMeshundefined here, andsubMesh.offset/indexCountwill crash the draw path. Also, the negative-count bailout is checking the pre-submeshcount, not the count selected for that submesh.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects-webgl/src/gl-engine.ts` around lines 229 - 241, Validate subMeshIndex and the retrieved subMesh before using it: check that subMeshes is non-empty, subMeshIndex is within 0..subMeshes.length-1, and that subMeshes[subMeshIndex] is not undefined, then read offset and choose count from subMesh.indexCount or subMesh.vertexCount (using indicesBuffer to decide) into a local variable; perform the negative-count bailout after computing this per-submesh count (i.e., if selected count <= 0 return) to avoid using undefined properties like subMesh.offset, subMesh.indexCount, or subMesh.vertexCount and to ensure the correct count is validated.packages/effects-core/src/render/renderer.ts (2)
232-250:⚠️ Potential issue | 🟠 MajorRestore the viewport as well as the framebuffer after
blit().When
prevFramebufferisnull,setFramebuffer(null)resets the viewport to full engine size. A caller that had a smaller screen viewport loses it after the blit, so the next pass renders to the wrong rectangle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects-core/src/render/renderer.ts` around lines 232 - 250, Before changing framebuffer in the blit path, capture the current viewport (e.g., read x,y,width,height into a prevViewport variable) along with prevFramebuffer; then after drawing (after drawGeometry(this.blitGeometry,...)) restore both by calling setFramebuffer(prevFramebuffer) and setViewport(prevViewport.x, prevViewport.y, prevViewport.width, prevViewport.height). Use the same helper calls used elsewhere (setFramebuffer, setViewport, getWidth/getHeight) so when prevFramebuffer is null you restore the original smaller viewport instead of the full-engine default.
266-271:⚠️ Potential issue | 🟠 MajorGuard
currentFrame.globalUniformsbefore registering globals.Line 267 still assumes a bound frame with an initialized
globalUniformsbag. Any pre-framesetGlobal*call will dereferenceundefinedand crash. This needs the same fix as the earlier report: initialize that bag up front or throw a clear “no RenderFrame bound” error here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects-core/src/render/renderer.ts` around lines 266 - 271, In checkGlobalUniform, guard against a missing render frame/globalUniforms by first verifying this.renderingData.currentFrame and its globalUniforms exist; if no RenderFrame is bound, either initialize currentFrame.globalUniforms to an empty bag (e.g. { uniforms: [] }) or throw a clear "no RenderFrame bound" error before accessing .uniforms; update the checkGlobalUniform method (and any callers like setGlobal* that assume a bound frame) to use this guard so pre-frame setGlobal* calls won't dereference undefined.
🧹 Nitpick comments (1)
packages/effects-core/src/engine.ts (1)
149-154: Consider adding a proper initial RenderingData type or factory.The
@ts-expect-errorfor the emptycurrentFrameobject suggests the initialization could be improved. Consider defining a default/emptyRenderFramefactory or allowingcurrentFrameto be nullable in theRenderingDatatype.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects-core/src/engine.ts` around lines 149 - 154, The RenderingData initialization currently uses a ts-expect-error to set currentFrame to an empty object; instead create a proper default RenderFrame (e.g., a factory function like createEmptyRenderFrame()) or update the RenderingData type to allow currentFrame to be null/undefined, then initialize renderingData.currentFrame with that factory result (or null) and remove the `@ts-expect-error`; reference the RenderingData shape and the currentFrame and RenderFrame symbols so the change is applied where renderingData is constructed in the Engine (the renderTargetPool initialization area).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/effects-core/src/material/material.ts`:
- Around line 596-619: The clone method currently copies the wrong source render
state and aliases mutable containers; fix clone in Material.clone by copying
from the source instance (this) not the newly created clonedMaterial: set
clonedMaterial.materialState = Object.assign(new MaterialState(),
this.materialState) so the source render-state (including enabled macros) is
preserved, and replace all direct assignments that alias containers (floats,
ints, vector2s, vector3s, vector4s, colors, quaternions, matrices, matrixArrays,
textures, floatArrays, vector4Arrays, samplers, uniforms, etc.) with shallow
copies (e.g., Array.prototype.slice / Array.from for arrays and
Object.assign({}, this.uniforms) for maps) so the clone gets its own mutable
copies; also ensure any matrix3s (matrice3s) and enabled macros fields are
copied from this.materialState into the clone.
- Around line 768-775: In toData() for Material, don't assign the live Shader
instance to materialData.shader; instead serialize a reference by storing the
shader's guid/id (e.g. materialData.shader = this._shader.guid or
this._shader.id) so fromData() can call
this.engine.findObject<Shader>(data.shader) successfully; follow the pattern
used in VFXItem.toData() where this.definition.id = this.guid and ensure you
preserve any expected type casts (the ts-expect-error lines) when replacing
this._shader with its guid.
In `@packages/effects-threejs/src/material/three-material.ts`:
- Around line 511-514: The override of clone in ThreeMaterial currently throws
and breaks runtime cloning; instead restore delegation to the base
Material.clone by calling super.clone(props) and then copy
ThreeMaterial-specific state (e.g., any three.js material fields stored on the
instance) into the returned Material, or remove the override entirely until
proper copy logic exists; update the override method named "clone" on the
ThreeMaterial class (the function signature override clone(props?:
MaterialProps): Material) to return the result of super.clone(props) after
applying any ThreeMaterial-specific property copying.
---
Duplicate comments:
In `@packages/effects-core/src/render/renderer.ts`:
- Around line 232-250: Before changing framebuffer in the blit path, capture the
current viewport (e.g., read x,y,width,height into a prevViewport variable)
along with prevFramebuffer; then after drawing (after
drawGeometry(this.blitGeometry,...)) restore both by calling
setFramebuffer(prevFramebuffer) and setViewport(prevViewport.x, prevViewport.y,
prevViewport.width, prevViewport.height). Use the same helper calls used
elsewhere (setFramebuffer, setViewport, getWidth/getHeight) so when
prevFramebuffer is null you restore the original smaller viewport instead of the
full-engine default.
- Around line 266-271: In checkGlobalUniform, guard against a missing render
frame/globalUniforms by first verifying this.renderingData.currentFrame and its
globalUniforms exist; if no RenderFrame is bound, either initialize
currentFrame.globalUniforms to an empty bag (e.g. { uniforms: [] }) or throw a
clear "no RenderFrame bound" error before accessing .uniforms; update the
checkGlobalUniform method (and any callers like setGlobal* that assume a bound
frame) to use this guard so pre-frame setGlobal* calls won't dereference
undefined.
In `@packages/effects-webgl/src/gl-engine.ts`:
- Around line 229-241: Validate subMeshIndex and the retrieved subMesh before
using it: check that subMeshes is non-empty, subMeshIndex is within
0..subMeshes.length-1, and that subMeshes[subMeshIndex] is not undefined, then
read offset and choose count from subMesh.indexCount or subMesh.vertexCount
(using indicesBuffer to decide) into a local variable; perform the
negative-count bailout after computing this per-submesh count (i.e., if selected
count <= 0 return) to avoid using undefined properties like subMesh.offset,
subMesh.indexCount, or subMesh.vertexCount and to ensure the correct count is
validated.
---
Nitpick comments:
In `@packages/effects-core/src/engine.ts`:
- Around line 149-154: The RenderingData initialization currently uses a
ts-expect-error to set currentFrame to an empty object; instead create a proper
default RenderFrame (e.g., a factory function like createEmptyRenderFrame()) or
update the RenderingData type to allow currentFrame to be null/undefined, then
initialize renderingData.currentFrame with that factory result (or null) and
remove the `@ts-expect-error`; reference the RenderingData shape and the
currentFrame and RenderFrame symbols so the change is applied where
renderingData is constructed in the Engine (the renderTargetPool initialization
area).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bc15e6d6-f710-43dc-ac20-64613af1c0b5
📒 Files selected for processing (23)
packages/effects-core/src/engine.tspackages/effects-core/src/material/index.tspackages/effects-core/src/material/material-state.tspackages/effects-core/src/material/material.tspackages/effects-core/src/render/renderer.tspackages/effects-core/src/render/shader.tspackages/effects-threejs/src/material/three-material.tspackages/effects-webgl/src/gl-engine.tspackages/effects-webgl/src/gl-material.tspackages/effects-webgl/src/gl-shader.tspackages/effects-webgl/src/index.tspackages/effects/src/index.tsplugin-packages/model/test/src/plugin-unit.spec.tsweb-packages/imgui-demo/src/panels/inspector.tsweb-packages/test/unit/src/effects-core/mask-processor.spec.tsweb-packages/test/unit/src/effects-core/plugins/particle/particle.spec.tsweb-packages/test/unit/src/effects-core/plugins/sprite/sprite-renderder.spec.tsweb-packages/test/unit/src/effects-webgl/gl-dispose.spec.tsweb-packages/test/unit/src/effects-webgl/gl-geometry.spec.tsweb-packages/test/unit/src/effects-webgl/gl-material.spec.tsweb-packages/test/unit/src/effects-webgl/gl-mesh.spec.tsweb-packages/test/unit/src/effects-webgl/gl-render-frame.spec.tsweb-packages/test/unit/src/effects-webgl/gl-render-pass.spec.ts
💤 Files with no reviewable changes (3)
- web-packages/test/unit/src/effects-core/plugins/sprite/sprite-renderder.spec.ts
- packages/effects-webgl/src/index.ts
- packages/effects-webgl/src/gl-material.ts
✅ Files skipped from review due to trivial changes (2)
- packages/effects-core/src/material/index.ts
- web-packages/imgui-demo/src/panels/inspector.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- web-packages/test/unit/src/effects-webgl/gl-dispose.spec.ts
- web-packages/test/unit/src/effects-webgl/gl-mesh.spec.ts
- web-packages/test/unit/src/effects-webgl/gl-render-frame.spec.ts
- packages/effects/src/index.ts
- web-packages/test/unit/src/effects-webgl/gl-material.spec.ts
- packages/effects-core/src/render/shader.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Breaking Changes
Architecture Changes
Tests