refactor: player engine creation logic#1248
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughEngine/GLEngine moved to an engine-centric architecture: they now own GL/context, ticker, compositions, asset managers and lifecycle. SceneLoader was added to load scenes via Engine. Render passes/framebuffers listen to engine resize. Many constructors and APIs switched from renderer/gl-centric to engine-centric. Event/hit-test and plugin envs now use engine.env. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Player
participant Engine
participant SceneLoader
participant AssetManager
participant Composition
participant Renderer
App->>Player: loadScene(url)
Player->>Engine: ensure/create Engine(canvas, options)
SceneLoader->>AssetManager: new AssetManager(scene, engine, options)
SceneLoader->>AssetManager: manager.load(scene)
SceneLoader->>Composition: create Composition(engine, opts)
Composition->>Engine: engine.addComposition(this)
SceneLoader->>Engine: prepare assets & compile shaders
Engine->>Renderer: start ticker / runRenderLoop
SceneLoader-->>App: return Composition
App->>Player: dispose()
Player->>Engine: engine.dispose()
Engine->>Composition: compositions.forEach(c => c.dispose())
Composition->>Engine: engine.removeComposition(this)
Engine->>Engine: dispose subsystems & clear collections
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Potential attention points:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2024-10-18T09:15:08.038ZApplied to files:
🧬 Code graph analysis (1)packages/effects/src/player.ts (6)
🪛 Biome (2.1.2)packages/effects/src/player.ts[error] 404-404: Invalid decorator (parse) 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the player engine creation logic by centralizing engine instantiation and moving responsibility from the renderer to the engine. The renderer is now created by the engine rather than creating its own engine instance, establishing a clearer hierarchy where the engine is the primary entry point.
Key changes:
- Engine now creates the renderer internally, reversing the previous pattern
- Player uses Engine instead of Renderer as the primary interface
- Test files updated to instantiate GLEngine first, then access the renderer through it
Reviewed Changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/effects/src/player.ts | Refactored to use Engine as primary interface; removed direct renderer instantiation and moved composition/asset management to engine |
| packages/effects/src/index.ts | Updated factory methods to accept Engine instead of GL context for renderer creation |
| packages/effects-webgl/src/gl-renderer.ts | Simplified constructor to accept Engine instead of creating its own; removed context management |
| packages/effects-webgl/src/gl-engine.ts | Enhanced to create renderer internally and manage context; now primary entry point |
| packages/effects-core/src/engine.ts | Added composition/asset management, ticker, event system; now central orchestrator |
| packages/effects-core/src/scene-loader.ts | New file extracting scene loading logic from Player for better separation of concerns |
| packages/effects-core/src/composition.ts | Changed handleItemMessage to optional onItemMessage callback |
| web-packages/test/unit/src/effects-webgl/*.spec.ts | Updated test files to instantiate GLEngine before accessing renderer |
| packages/effects-threejs/src/*.ts | Updated ThreeEngine and ThreeRenderer to follow new engine-first pattern |
| web-packages/imgui-demo/src/ge.ts | Changed plugin import and modified playURL parameter |
| web-packages/demo/src/single.ts | Simplified scene loading by removing redundant entries and using options parameter |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
web-packages/imgui-demo/src/ge.ts (1)
11-11: Remove unused import.The
animationSceneimport is no longer used after switching to remote URL loading on line 25. This is dead code that should be removed to improve code clarity.Apply this diff to remove the unused import:
-import * as animationScene from './ffd-demo.json';web-packages/test/unit/src/effects-webgl/renderer.spec.ts (1)
167-188: Async test not awaited; assertions may be skipped.Return a Promise (or use done) and dispose engine after assertions.
-it('safe to call destroy', async () => { - const engine = new GLEngine(glCanvas, { glType: 'webgl' }); +it('safe to call destroy', () => new Promise<void>((resolve) => { + const engine = new GLEngine(glCanvas, { glType: 'webgl' }); const renderer = engine.renderer as GLRenderer; const gl = (renderer.engine as GLEngine).gl; const texture = new GLTexture(renderer.engine, { sourceType: TextureSourceType.framebuffer, data: { width: 3, height: 3, }, }); texture.initialize(); gl.getExtension('WEBGL_lose_context')?.loseContext(); window.setTimeout(() => { expect(renderer.glRenderer.isDestroyed).to.be.true; texture.dispose(); // @ts-expect-error protected expect(texture.destroyed).to.be.true; - }, 60); - engine.dispose(); -}); + engine.dispose(); + resolve(); + }, 60); +}));web-packages/test/unit/src/effects-webgl/gl-gpu-buffer.spec.ts (1)
73-93: Wrong WebGL2 check; condition never true.GLGPUBuffer isn’t a rendering context. Use isWebGL2(buffer.engine.gl).
- if (buffer instanceof WebGL2RenderingContext) { + if (isWebGL2(buffer.engine.gl)) {web-packages/test/unit/src/effects-webgl/gl-geometry.spec.ts (1)
540-547: Fix incorrect renderer references in tests (lines 540, 541, 564).The tests pass the internal
glRenderertocreateMesh()andRenderPass(), but both expect the publicrendererAPI. The second test already usesrendererforcreateMesh()at line 563, confirming the pattern. Update all three occurrences:- const ret = createMesh(glRenderer, -2); - const pass = new RenderPass(glRenderer, { + const ret = createMesh(renderer, -2); + const pass = new RenderPass(renderer, { name: 'test', meshes: [ret.mesh], });Also update line 564 in the second test:
- const pass = new RenderPass(glRenderer, { + const pass = new RenderPass(renderer, { name: 'test', meshes: [ret.mesh], });packages/effects-webgl/src/gl-renderer.ts (1)
304-309: Line 308 references non-existent field: should bethis.engine.canvas = nullor removed.
this.canvasdoes not exist in GLRenderer or its parent Renderer class. The canvas field belongs to the Engine class. Line 308 should either be changed tothis.engine.canvas = nullor removed entirely, since Engine is responsible for disposing its own canvas.web-packages/test/unit/src/effects-webgl/gl-material.spec.ts (1)
50-54: Don’t dispose shared engine.shaderLibrary in afterEach while reusing the same engine.This can break subsequent tests that compile new shaders and introduces non‑idempotent global state. Prefer:
- Keep the library alive and clear caches (if API exists), or
- Create/tear down a fresh GLEngine per test.
If caches must be reset, I can help switch to per‑test engine or add a cache‑clear helper instead of dispose.
web-packages/test/unit/src/effects-webgl/gl-shader-library.spec.ts (1)
128-147: Avoid sharing global canvas and dispose the temporary engine.Creating a new GLEngine on
webgl2Canvaswhileengine2owns it risks conflicts; also the temp engine isn’t disposed.- const engine = new GLEngine(webgl2Canvas, { glType: 'webgl2' }); + const canvas = document.createElement('canvas'); + const engine = new GLEngine(canvas, { glType: 'webgl2' }); const shaderLib = engine.shaderLibrary; @@ - shaderLib.compileShader(shader, result => { + shaderLib.compileShader(shader, result => { expect(result).to.eql(shader.compileResult); expect(shader.compileResult?.status).to.eql(ShaderCompileResultStatus.success); // 不同机器上编译时间不一样，把阈值调小 expect(Date.now() - time).to.be.greaterThanOrEqual(1); - done(); + engine.dispose(); + canvas.remove(); + done(); });web-packages/test/unit/src/effects-webgl/gl-dispose.spec.ts (1)
21-21: Broadengltype to include WebGL2
GLEngine.glisWebGLRenderingContext | WebGL2RenderingContext. Narrowing toWebGLRenderingContextcan cause TS errors.- let gl: WebGLRenderingContext; + let gl: WebGLRenderingContext | WebGL2RenderingContext;packages/effects/src/player.ts (2)
517-519: Guard WebGL2 timer path for non-GL or WebGL1 renderersCasting
this.renderer as GLRendererwill break for non-WebGL renderers. Detect WebGL2 context safely before callinggpuTimer.- const { gl } = (this.renderer as GLRenderer).context; - const time = (level === 2 && this.reportGPUTime) ? gpuTimer(gl as WebGL2RenderingContext) : undefined; + const glAny = (this as any)?.renderer?.context?.gl; + const gl2 = typeof WebGL2RenderingContext !== 'undefined' && glAny instanceof WebGL2RenderingContext ? glAny as WebGL2RenderingContext : undefined; + const time = (level === 2 && this.reportGPUTime && gl2) ? gpuTimer(gl2) : undefined;
707-727: Don’t remove externally-provided canvas on dispose
useExternalCanvas=truewhen caller supplied the canvas. Disposing the Player should not remove user-owned DOM nodes. Gate removal on!useExternalCanvas.this.pause(); - this.engine.dispose(); + this.engine.dispose(); - if (this.canvas instanceof HTMLCanvasElement && (this.engine as GLEngine).context) { + if (!this.useExternalCanvas && this.canvas instanceof HTMLCanvasElement) { // TODO: 数据模版下掉可以由文本模块单独管理 canvasPool.dispose(); // canvas will become a cry emoji in Android if still in dom if (this.canvas.parentNode) { this.canvas.parentNode.removeChild(this.canvas); } this.canvas.remove(); }
🧹 Nitpick comments (18)
web-packages/demo/src/single.ts (1)
22-23: API usage looks good; consider formatting consistency.The refactored
loadSceneAPI is demonstrated correctly with the newoptionsparameter. Both autoplay configurations work as expected.Consider adding spaces after the colons for consistency with common JavaScript style guides:
- await player.loadScene(jsons, { autoplay:true }); - await player.loadScene(json, { autoplay:false }); + await player.loadScene(jsons, { autoplay: true }); + await player.loadScene(json, { autoplay: false });web-packages/test/unit/src/effects-webgl/gl-geometry.spec.ts (1)
38-46: Dispose geometry before engine to avoid use-after-dispose.Dispose child resources first, then engine. Example fix:
after(() => { - engine.dispose(); + geometry.dispose(); + engine.dispose(); renderer = null; canvas.remove(); canvas = null; gl = null; glRenderer = null; - geometry.dispose(); });web-packages/test/unit/src/effects-webgl/renderer.spec.ts (2)
22-63: Don’t dispose renderer inside the copy() helper.Let callers manage lifecycle via engine.dispose() to avoid double-dispose and hidden side-effects.
readPixels(gl, source.textureBuffer, data); expect(data.data).to.deep.equals(new Uint8Array([1, 2, 3, 4])); - renderer.dispose(); + // cleanup is handled by the caller via engine.dispose()
114-115: Dispose engine, not renderer, for consistency.Standardize on engine.dispose() to release all resources.
- renderer.dispose(); + engine.dispose();web-packages/test/unit/src/effects-webgl/gl-material.spec.ts (3)
34-37: Avoid reaching into renderer internals to get GL. Use engine.gl.This reduces coupling to GLRendererInternal.
Apply:
- renderer = glEngine.renderer as GLRenderer; - gl = renderer.glRenderer.gl; - engine = glEngine; + renderer = glEngine.renderer as GLRenderer; + engine = glEngine; + gl = (engine as GLEngine).gl;
120-122: Unify material.setupStates argument to engine.Pass engine (GLEngine) consistently rather than renderer.glRenderer.engine to avoid touching internals.
Example change (apply similarly across file):
- material.setupStates(renderer.glRenderer.engine); + material.setupStates(engine as GLEngine);Also applies to: 168-169, 178-179, 195-196, 214-216, 225-226, 233-235, 269-271, 308-309, 333-334, 341-342, 349-351, 357-359
175-176: Fix test title typos for clarity.“enable disabled” → “enabled” and “mack” → “mask”.
- it('cullFace enable disabled with custom parameters', () => { + it('cullFace enabled with custom parameters', () => {packages/effects-threejs/src/three-display-object.ts (2)
31-66: Add dispose() to release Engine and compositions.Prevents GL/event leaks when the display object is removed.
export class ThreeDisplayObject extends THREE.Group { @@ update (delta: number) { this.compositions.forEach(composition => { composition.update(delta); }); } + + dispose () { + this.compositions.forEach(c => c.dispose()); + this.compositions.length = 0; + this.engine.dispose(); + } }Also applies to: 166-189
41-44: Type the renderer getter explicitly (optional).Improves API clarity.
- get renderer () { + get renderer (): any { return this.engine.renderer; }If
Renderertype is available, preferRendererand addimport type { Renderer } from '@galacean/effects-core';.web-packages/test/unit/src/effects-webgl/gl-shader-library.spec.ts (2)
171-193: Dispose the engine instead of only the renderer.Engine manages more resources; let it tear down Renderer too.
- renderer.dispose(); - canvas.remove(); + engine.dispose(); + canvas.remove();
195-219: Same here: prefer engine.dispose().Mirror the previous change for consistency and leak‑free teardown.
- renderer.dispose(); - canvas.remove(); + engine.dispose(); + canvas.remove();packages/effects-webgl/src/gl-engine.ts (1)
31-46: Build options once and pass to super; avoid mutating the param.Ensures Engine sees defaulted options and improves clarity.
- constructor (canvas: HTMLCanvasElement, options?: EngineOptions) { - super(canvas, options); - options = { + constructor (canvas: HTMLCanvasElement, options?: EngineOptions) { + const opts: EngineOptions = { preserveDrawingBuffer: undefined, alpha: true, stencil: true, antialias: true, depth: true, premultipliedAlpha: true, glType: 'webgl2', - ...options, + ...options, }; - - this.context = new GLContextManager(canvas, options.glType, options); - const gl = this.context.gl; + super(canvas, opts); + this.context = new GLContextManager(canvas, opts.glType, opts); + const gl = this.context.gl;web-packages/test/unit/src/effects-webgl/gl-render-frame.spec.ts (1)
64-64: Prefer engine.gl over renderer.glRenderer.gl.Keeps the test decoupled from renderer internals.
- const gl = renderer.glRenderer.gl; + const gl = (renderer.engine as GLEngine).gl;web-packages/test/unit/src/effects-webgl/gl-dispose.spec.ts (1)
39-41: Useengineto accessshaderLibrarydirectlySlightly clearer and avoids coupling through
renderer.engine.- const sb = (renderer.engine as GLEngine).shaderLibrary; + const sb = (engine as GLEngine).shaderLibrary;packages/effects/src/player.ts (2)
227-229: Avoid in-place sorting of engine-managed arraySorting
this.compositionsmutates the engine’s internal order. Return a sorted copy instead.- getCompositions () { - return this.compositions.sort((a, b) => a.getIndex() - b.getIndex()); - } + getCompositions () { + return [...this.compositions].sort((a, b) => a.getIndex() - b.getIndex()); + }
522-526: Simplify redundant render condition
this.compositions.length < comps.lengthis always false sincecomps === this.compositions.- if ( - this.compositions.length || - this.compositions.length < comps.length || - forceRender - ) { + if (comps.length || forceRender) {packages/effects-core/src/engine.ts (1)
340-358: Avoid double-disposing builtin textures
assetService.dispose()disposes builtin textures, and laterthis.textures.forEach(tex => tex.dispose())disposes all textures again. If Texture.dispose isn’t idempotent, this risks errors; even if idempotent, it’s redundant.
- Option A: Dispose user textures first, then call
assetService.dispose()for builtins.- Option B: Filter builtins out of
this.texturesprior to the loop.packages/effects-core/src/composition.ts (1)
873-884: Avoid double-dispose inlost()
lost()disposes textures and then callsdispose(), which again disposes textures. Remove the first loop and rely ondispose().- lost (e: Event): void { + lost (e: Event): void { this.videoState = this.textures.map(tex => { if ('video' in tex.source) { tex.source.video.pause(); return tex.source.video.currentTime; } }); - this.textures.map(tex => tex.dispose()); this.dispose(); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
packages/effects-core/src/asset-manager.ts(1 hunks)packages/effects-core/src/asset-service.ts(1 hunks)packages/effects-core/src/composition.ts(7 hunks)packages/effects-core/src/engine.ts(6 hunks)packages/effects-core/src/index.ts(1 hunks)packages/effects-core/src/plugins/sprite/sprite-mesh.ts(1 hunks)packages/effects-core/src/render/renderer.ts(3 hunks)packages/effects-core/src/scene-loader.ts(1 hunks)packages/effects-core/src/vfx-item.ts(1 hunks)packages/effects-threejs/src/index.ts(1 hunks)packages/effects-threejs/src/three-display-object.ts(4 hunks)packages/effects-threejs/src/three-engine.ts(2 hunks)packages/effects-threejs/src/three-renderer.ts(1 hunks)packages/effects-webgl/src/gl-engine.ts(3 hunks)packages/effects-webgl/src/gl-renderer.ts(3 hunks)packages/effects/src/index.ts(2 hunks)packages/effects/src/player.ts(11 hunks)plugin-packages/model/test/src/memory-leak.spec.ts(1 hunks)plugin-packages/model/test/src/plugin-unit.spec.ts(4 hunks)web-packages/demo/src/single.ts(1 hunks)web-packages/imgui-demo/src/ge.ts(2 hunks)web-packages/test/memory/src/memory-leak.spec.ts(2 hunks)web-packages/test/unit/src/effects-webgl/gl-dispose.spec.ts(3 hunks)web-packages/test/unit/src/effects-webgl/gl-frame-buffer.spec.ts(2 hunks)web-packages/test/unit/src/effects-webgl/gl-geometry.spec.ts(2 hunks)web-packages/test/unit/src/effects-webgl/gl-gpu-buffer.spec.ts(1 hunks)web-packages/test/unit/src/effects-webgl/gl-material.spec.ts(2 hunks)web-packages/test/unit/src/effects-webgl/gl-mesh.spec.ts(2 hunks)web-packages/test/unit/src/effects-webgl/gl-render-frame.spec.ts(1 hunks)web-packages/test/unit/src/effects-webgl/gl-render-pass.spec.ts(1 hunks)web-packages/test/unit/src/effects-webgl/gl-shader-library.spec.ts(7 hunks)web-packages/test/unit/src/effects-webgl/gl-state.spec.ts(2 hunks)web-packages/test/unit/src/effects-webgl/gl-texture.spec.ts(3 hunks)web-packages/test/unit/src/effects-webgl/gl-vertex-array-object.spec.ts(3 hunks)web-packages/test/unit/src/effects-webgl/renderer.spec.ts(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (25)
packages/effects-core/src/asset-manager.ts (1)
packages/effects-core/src/scene.ts (1)
SceneLoadOptions(65-143)
packages/effects-threejs/src/three-renderer.ts (2)
packages/effects-core/src/render/renderer.ts (1)
Renderer(12-142)packages/effects-core/src/engine.ts (1)
Engine(30-359)
web-packages/test/unit/src/effects-webgl/gl-geometry.spec.ts (1)
packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-654)
packages/effects-core/src/scene-loader.ts (6)
packages/effects-core/src/scene.ts (3)
Scene(13-38)LoadType(46-46)SceneLoadOptions(65-143)packages/effects-core/src/engine.ts (1)
Engine(30-359)packages/effects-core/src/composition.ts (2)
MessageItem(46-63)Composition(132-1032)packages/effects-core/src/utils/index.ts (1)
isArray(71-73)packages/effects-core/src/asset-manager.ts (1)
AssetManager(24-473)packages/effects-core/src/constants.ts (1)
PLAYER_OPTIONS_ENV_EDITOR(8-8)
web-packages/test/unit/src/effects-webgl/gl-material.spec.ts (2)
packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-654)packages/effects-webgl/src/gl-renderer.ts (1)
GLRenderer(21-350)
packages/effects-core/src/render/renderer.ts (1)
packages/effects-core/src/engine.ts (1)
Engine(30-359)
packages/effects-threejs/src/three-display-object.ts (4)
packages/effects-core/src/engine.ts (1)
Engine(30-359)packages/effects-threejs/src/three-engine.ts (1)
ThreeEngine(15-34)packages/effects-core/src/asset-service.ts (1)
AssetService(14-142)packages/effects-core/src/composition.ts (1)
MessageItem(46-63)
web-packages/test/unit/src/effects-webgl/gl-gpu-buffer.spec.ts (2)
packages/effects-webgl/src/gl-renderer-internal.ts (1)
canvas(72-74)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-654)
packages/effects/src/index.ts (2)
packages/effects-core/src/render/renderer.ts (1)
Renderer(12-142)packages/effects-core/src/engine.ts (2)
Engine(30-359)EngineOptions(21-25)
plugin-packages/model/test/src/plugin-unit.spec.ts (2)
packages/effects-webgl/src/gl-renderer-internal.ts (1)
canvas(72-74)packages/effects-core/src/engine.ts (1)
Engine(30-359)
packages/effects-core/src/engine.ts (8)
packages/effects-core/src/gl/create-gl-context.ts (1)
GLType(1-1)packages/effects-core/src/composition.ts (1)
Composition(132-1032)packages/effects-core/src/asset-manager.ts (1)
AssetManager(24-473)packages/effects-core/src/asset-service.ts (1)
AssetService(14-142)packages/effects-core/src/plugins/interact/event-system.ts (1)
EventSystem(28-181)packages/effects-core/src/ticker.ts (1)
Ticker(8-141)packages/effects-core/src/texture/texture.ts (2)
generateWhiteTexture(235-249)generateTransparentTexture(251-265)packages/effects-core/src/asset-loader.ts (1)
AssetLoader(13-71)
web-packages/test/unit/src/effects-webgl/gl-render-pass.spec.ts (2)
web-packages/test/unit/src/effects-webgl/gl-utils.ts (1)
getGL2(13-23)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-654)
packages/effects-threejs/src/three-engine.ts (2)
packages/effects-core/src/engine.ts (1)
EngineOptions(21-25)packages/effects-threejs/src/three-renderer.ts (1)
ThreeRenderer(4-14)
web-packages/test/unit/src/effects-webgl/gl-frame-buffer.spec.ts (2)
packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-654)packages/effects-webgl/src/gl-renderer.ts (1)
GLRenderer(21-350)
web-packages/test/unit/src/effects-webgl/gl-render-frame.spec.ts (3)
packages/effects-webgl/src/gl-renderer.ts (1)
GLRenderer(21-350)packages/effects-core/src/engine.ts (1)
Engine(30-359)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-654)
packages/effects/src/player.ts (3)
packages/effects-core/src/engine.ts (1)
Engine(30-359)packages/effects-core/src/scene-loader.ts (1)
SceneLoader(10-117)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-654)
web-packages/test/unit/src/effects-webgl/gl-shader-library.spec.ts (3)
packages/effects-core/src/engine.ts (1)
Engine(30-359)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-654)packages/effects-webgl/src/gl-renderer.ts (1)
GLRenderer(21-350)
web-packages/test/unit/src/effects-webgl/gl-vertex-array-object.spec.ts (5)
packages/effects-threejs/src/three-display-object.ts (1)
renderer(41-43)packages/effects/src/player.ts (1)
renderer(77-79)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-654)packages/effects-webgl/src/gl-renderer-internal.ts (1)
canvas(72-74)packages/effects-webgl/src/gl-renderer.ts (1)
GLRenderer(21-350)
packages/effects-webgl/src/gl-engine.ts (4)
packages/effects-webgl/src/gl-context-manager.ts (1)
GLContextManager(4-67)packages/effects-core/src/engine.ts (1)
EngineOptions(21-25)packages/effects-core/src/utils/asserts.ts (1)
assertExist(1-5)packages/effects-webgl/src/gl-renderer.ts (1)
GLRenderer(21-350)
web-packages/test/unit/src/effects-webgl/renderer.spec.ts (2)
packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-654)packages/effects-webgl/src/gl-renderer.ts (1)
GLRenderer(21-350)
packages/effects-webgl/src/gl-renderer.ts (2)
packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-654)packages/effects-core/src/engine.ts (1)
Engine(30-359)
web-packages/test/unit/src/effects-webgl/gl-state.spec.ts (2)
packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-654)packages/effects-webgl/src/gl-renderer.ts (1)
GLRenderer(21-350)
web-packages/test/unit/src/effects-webgl/gl-dispose.spec.ts (2)
packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-654)packages/effects-webgl/src/gl-renderer.ts (1)
GLRenderer(21-350)
web-packages/test/unit/src/effects-webgl/gl-texture.spec.ts (2)
packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-654)packages/effects-webgl/src/gl-renderer.ts (1)
GLRenderer(21-350)
web-packages/test/unit/src/effects-webgl/gl-mesh.spec.ts (3)
packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-654)packages/effects-webgl/src/gl-renderer.ts (1)
GLRenderer(21-350)packages/effects-webgl/src/gl-shader.ts (1)
GLShaderVariant(14-123)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (33)
web-packages/imgui-demo/src/ge.ts (1)
25-25: LGTM!The switch to loading a remote URL directly aligns with the PR's goal of simplifying the player initialization pattern and moving toward remote asset loading.
plugin-packages/model/test/src/plugin-unit.spec.ts (6)
465-466: LGTM: Proper Engine initialization with explicit canvas.The test now correctly creates a dedicated canvas-backed Engine instance, aligning with the refactored Engine-first initialization pattern described in the PR objectives.
698-699: LGTM: Proper Engine disposal for cleanup.The explicit
engine.dispose()call ensures proper cleanup of the canvas-backed Engine instance after the test completes.
703-704: LGTM: Consistent Engine initialization pattern.The Skybox test follows the same correct pattern as the Material test: explicit canvas creation and Engine instantiation.
731-732: LGTM: Consistent cleanup pattern.Proper Engine disposal at the end of the Skybox test ensures resource cleanup.
736-737: LGTM: Consistent Engine initialization for Mesh test.The Mesh test properly follows the same Engine-first initialization pattern with explicit canvas management.
1192-1193: LGTM: Proper cleanup after complex async test.The Engine disposal correctly occurs after all async operations and nested test logic complete, ensuring proper resource cleanup.
packages/effects-core/src/plugins/sprite/sprite-mesh.ts (1)
5-21: No action required—removed APIs have no active code references.Verification shows that all references to
setSpriteMeshMaxItemCountByGPU,setMaxSpriteMeshItemCount, andmaxSpriteMeshItemCountexist only in disabled/commented test code (web-packages/test/unit/src/effects-core/plugins/sprite/sprite-combine.spec.ts is ~81% commented out with no active test cases). No active code in the repository references the removed APIs, so the removal poses no breaking change to the codebase.Likely an incorrect or invalid review comment.
plugin-packages/model/test/src/memory-leak.spec.ts (1)
63-63: LGTM! Disposal API correctly updated.The change aligns with the simplified
dispose()API that no longer accepts akeepCanvasparameter.web-packages/test/unit/src/effects-webgl/gl-state.spec.ts (3)
1-2: LGTM! Import pattern correctly updated.The type-only import for
GLRendererand value import forGLEnginealign with the new engine-centric initialization pattern.
16-17: LGTM! Engine-centric initialization correctly implemented.The test now follows the unified Engine-first pattern, creating
GLEngineand deriving the renderer fromengine.renderer. This aligns with the broader architectural refactor.
23-23: LGTM! Disposal correctly updated.Disposal now happens at the engine level, which will internally dispose the renderer and other resources.
web-packages/test/unit/src/effects-webgl/gl-vertex-array-object.spec.ts (3)
2-3: LGTM! Import pattern correctly updated.Consistent with the engine-centric pattern,
GLRendereris imported as a type whileGLEngineis imported as a value.
28-31: LGTM! Cleanup correctly updated for engine-centric pattern.The test now obtains the engine from the renderer and disposes it, with canvas cleanup via
engine.context.canvas.
176-181: LGTM! Helper function correctly updated.The
createGLGPURendererhelper now creates aGLEngineand returns its renderer, aligning with the engine-first initialization pattern.packages/effects-core/src/index.ts (1)
36-36: LGTM! New SceneLoader export added.The export makes the new
SceneLoaderutility available from the core package, supporting the refactored scene loading workflow.packages/effects-core/src/vfx-item.ts (1)
717-717: LGTM! Canvas access correctly updated.The change updates the canvas access path from
renderer.canvastoengine.canvas, aligning with the engine-centric architecture where theEngineis the primary owner of the canvas.packages/effects-core/src/asset-service.ts (1)
25-25: LGTM! Built-in texture correctly registered.The addition of
engine.transparentTexturetobuiltinObjectsextends the set of built-in textures managed byAssetService, ensuring proper initialization and lifecycle management.web-packages/test/memory/src/memory-leak.spec.ts (1)
52-52: LGTM! Disposal API correctly updated.Both test cases now use the simplified
dispose()API that no longer accepts parameters, aligning with the broader refactor.Also applies to: 128-128
packages/effects-core/src/asset-manager.ts (1)
97-97: Consider the scope of AssetManager's options.The constructor parameter type has been broadened from
Omit<SceneLoadOptions, 'speed' | 'autoplay' | 'reusable'>to the fullSceneLoadOptions. While this simplifies the type signature, it may introduce ambiguity aboutAssetManager's responsibilities.
AssetManagerfocuses on asset loading and doesn't directly handle playback control options likespeed,autoplay, orreusable. If these properties are now managed bySceneLoaderor other components, the broadened type is acceptable but could benefit from documentation clarifying which options are relevant toAssetManager.Consider whether the previous explicit exclusion served a documentation purpose that's now lost. If the broader type is intentional for consistency with scene loading workflows, ensure it's clear in documentation which options
AssetManageractually uses.web-packages/test/unit/src/effects-webgl/gl-geometry.spec.ts (1)
31-33: Engine-first setup looks good.Creating GLEngine with glType and deriving renderer via engine.renderer aligns with the new lifecycle.
web-packages/test/unit/src/effects-webgl/renderer.spec.ts (1)
66-68: Engine-centric instantiation LGTM.Using GLEngine(canvas, { glType }) and deriving GLRenderer via engine.renderer matches the new API and simplifies teardown.
Also applies to: 80-82, 92-94, 118-120, 143-145, 168-170, 191-193
web-packages/test/unit/src/effects-webgl/gl-gpu-buffer.spec.ts (1)
13-16: Engine-based setup/cleanup looks right.Reusing canvas from gl and constructing GLEngine with glType: 'webgl2' plus proper canvas removal is consistent.
Also applies to: 19-22
packages/effects-threejs/src/three-renderer.ts (1)
1-7: Constructor refactor LGTM.ThreeRenderer now cleanly accepts Engine and delegates to base class.
packages/effects-threejs/src/three-engine.ts (1)
20-25: No changes required; original concern is not applicable to ThreeEngine.Verification confirms ThreeEngine does not expose
glas a public property, and no downstream code in the effects-threejs package accessesengine.gl. The constructor correctly receives a WebGL context, extracts the canvas, and passes it to the parent Engine class. The optional canvas overload suggestion remains valid for API consistency but is non-blocking.web-packages/test/unit/src/effects-webgl/gl-frame-buffer.spec.ts (1)
30-31: Verify duplicate renderer instantiation.Line 31 creates a
GLRendererinstance separately from the engine, butGLEnginealready creates and assigns its own renderer internally (as shown in the gl-engine.ts snippet wherethis.renderer = new GLRenderer(this)in constructor). This creates two renderer instances, which may not be the intended behavior.Consider using
engine.rendererdirectly instead of creating a separatefakeRendererinstance.web-packages/test/unit/src/effects-webgl/gl-material.spec.ts (1)
593-606: Non‑standard Chai assertioneqls— verify or replace.Chai typically uses
eql/deep.equal. If no custom plugin addseqls, replace withdeep.equalfor typed arrays.Apply:
- expect(new Float32Array(data2)).to.eqls(new Float32Array(testData)); + expect(new Float32Array(data2)).to.deep.equal(new Float32Array(testData)); ... - expect(new Float32Array(valData)).to.eqls(new Float32Array([1, 2, 3, 4])); + expect(new Float32Array(valData)).to.deep.equal(new Float32Array([1, 2, 3, 4]));packages/effects-threejs/src/index.ts (1)
25-68: Ensure static.createmethods are declared in core types.At runtime this works, but TS needs static members declared on the classes in
@galacean/effects-core. If not already added there, augment types in this package.Suggested augmentation (if needed):
// types/augment-core.d.ts import { Texture, Material, Geometry, Mesh, Engine } from '@galacean/effects-core'; declare module '@galacean/effects-core' { interface TextureConstructor { create(engine: Engine, options?: any): Texture; createWithData(engine: Engine, data?: any, options?: any): Texture; } interface MaterialConstructor { create(engine: Engine, props?: any): Material; } interface GeometryConstructor { create(engine: Engine, options?: any): Geometry; } interface MeshConstructor { create(engine: Engine, props?: any): Mesh; } }web-packages/test/unit/src/effects-webgl/gl-dispose.spec.ts (2)
25-32: Engine-first setup: LGTMCreating GLEngine and deriving renderer/engine/gl from it aligns with the new API and disposal flow.
46-46: Teardown: disposing the Engine is correctSwitching from renderer.dispose() to engine.dispose() matches the engine-managed lifecycle.
packages/effects-core/src/composition.ts (1)
301-303: Engine composition lifecycle registration: LGTMRegistering on construction and unregistering on dispose keeps
Engine.compositionsauthoritative.Also applies to: 929-930
packages/effects/src/player.ts (1)
155-161: Engine.create is properly assigned and initializedEngine.create is assigned in packages/effects/src/index.ts:81 as a factory function that returns a new GLEngine instance. This assignment occurs at module load time before any Player instances are created, so the initialization concern is resolved.
Clarification needed: Your original scratchpad mentions 5 concerns, but only the Engine.create verification is shown in the visible review comment. The scratchpad also references:
- getters
- loadScene wiring (348-353)
- doTick GPU timer cast (517-519) and condition (522-526)
- getCompositions in-place sort (227-229)
- dispose canvas removal (707-727)
Should I verify all of these concerns, or is this review comment focused only on Engine.create initialization?
packages/effects-core/src/engine.ts (1)
76-89: No renderer dependency issue found. Review comment is incorrect.The builtin textures (
whiteTextureandtransparentTexture) are created during Engine construction, but their initialization does not accessengine.renderer:
- GLTexture constructor only calls
this.fromData(), which merely sets properties (source,sourceType,sourceFrom,name)—no GPU operations occur.- GPU initialization (the
initialize()method) is invoked separately and usesthis.engine.gl, notthis.engine.renderer.- By the time
initialize()is called, the renderer is already assigned (set in GLEngine constructor aftersuper()returns).- No instances of
engine.rendereraccess were found in the texture codebase during construction.The textures are safely created before renderer initialization without any dependency issues.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/effects-webgl/src/gl-engine.ts (1)
57-67: Dispose order now correct; consider removing final reset() call.The disposal order (renderer → shaderLibrary → context) correctly addresses the past review concern by disposing GPU consumers before losing the GL context.
However,
reset()at Line 66 clears caches after the engine is fully disposed. Since the engine won't be reused post-disposal, this call appears unnecessary.Consider removing the final
reset()call:override dispose () { if (this.disposed) { return; } super.dispose(); this.renderer.dispose(); this.shaderLibrary?.dispose(); this.context.dispose(); - this.reset(); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/effects-core/src/composition.ts(7 hunks)packages/effects-core/src/scene-loader.ts(1 hunks)packages/effects-webgl/src/gl-engine.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/effects-core/src/scene-loader.ts (6)
packages/effects-core/src/scene.ts (3)
Scene(13-38)LoadType(46-46)SceneLoadOptions(65-143)packages/effects-core/src/engine.ts (1)
Engine(30-359)packages/effects-core/src/composition.ts (2)
MessageItem(46-63)Composition(132-1032)packages/effects-core/src/utils/index.ts (1)
isArray(71-73)packages/effects-core/src/asset-manager.ts (1)
AssetManager(24-473)packages/effects-core/src/constants.ts (1)
PLAYER_OPTIONS_ENV_EDITOR(8-8)
packages/effects-webgl/src/gl-engine.ts (4)
packages/effects-webgl/src/gl-context-manager.ts (1)
GLContextManager(4-67)packages/effects-core/src/engine.ts (1)
EngineOptions(21-25)packages/effects-core/src/utils/asserts.ts (1)
assertExist(1-5)packages/effects-webgl/src/gl-renderer.ts (1)
GLRenderer(21-350)
packages/effects-core/src/composition.ts (2)
packages/effects-threejs/src/three-display-object.ts (1)
renderer(41-43)packages/effects/src/player.ts (1)
renderer(77-79)
🔇 Additional comments (7)
packages/effects-webgl/src/gl-engine.ts (1)
31-55: LGTM! Constructor refactoring is well-structured.The new canvas-based initialization flow with GLContextManager properly centralizes context management. The initialization order is correct: context creation → gl extraction → assertion → cache reset → capability detection → shader library → renderer setup.
packages/effects-core/src/scene-loader.ts (3)
10-24: LGTM! Method signature and initialization are well-structured.The static
loadmethod properly handles both single and multiple scene inputs, with sensible defaults for autoplay. The fixes for circular references (usingengine.gpuCapabilityandengine.assetManagersdirectly) from past reviews are correctly applied.
26-69: LGTM! Asset management and composition creation flow is correct.The lifecycle is well-orchestrated: dispose old assets → parallel scene loading → sequential composition creation with per-scene autoplay configuration. The precompilation and initialization order are appropriate.
71-92: LGTM! Compilation synchronization and statistics tracking are correct.The Promise wrapper properly converts the callback-based
compileAllShadersto an awaitable operation. Statistics are recorded and logged appropriately, and the return type correctly matches the input type.packages/effects-core/src/composition.ts (3)
108-108: LGTM! Optional callback pattern is correctly implemented.The
onItemMessagecallback is properly declared as optional, conditionally assigned, and safely invoked with optional chaining at the call sites. This approach is idiomatic and addresses the past review concern about unguarded calls.Also applies to: 231-236, 298-298, 375-377
813-818: LGTM! Callback invocations are safely guarded.The optional chaining (
this.onItemMessage?.) correctly prevents errors when the callback is not provided, and theMessageItemstructure with begin/end phrases matches the interface definition.Also applies to: 837-842
302-302: LGTM! Engine lifecycle registration is correctly implemented.The composition properly registers with the engine during construction (Line 302) and unregisters during disposal (Line 929). The editor environment check ensures appropriate behavior for different runtime contexts.
Also applies to: 929-929
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/effects/src/player.ts (1)
162-163: Potentially redundantrenderer.envassignment.Line 163 sets
this.renderer.envafter settingthis.engine.envat line 162. Since the renderer is now managed by the engine (accessed viathis.engine.renderer), the engine should ideally be responsible for initializing the renderer's environment. Setting it directly here might indicate unclear ownership boundaries.Consider removing line 163 if the Engine constructor or Engine.create() already handles setting
renderer.envfrom the engine's env property.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/effects-core/src/scene-loader.ts(1 hunks)packages/effects/src/player.ts(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/effects-core/src/scene-loader.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/effects/src/player.ts (4)
packages/effects-core/src/engine.ts (1)
Engine(30-359)packages/effects-core/src/composition.ts (3)
time(409-411)Composition(132-1032)MessageItem(46-63)packages/effects-core/src/scene-loader.ts (1)
SceneLoader(10-117)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-654)
🔇 Additional comments (4)
packages/effects/src/player.ts (4)
67-97: LGTM! Clean proxy pattern to engine properties.The public getters for
tickerandrendererprovide a clean API surface, and the private getters properly delegate to engine-managed state. This refactoring maintains backward compatibility while centralizing state in the engine.
349-351: LGTM! Proper delegation to SceneLoader.The scene loading now correctly delegates to
SceneLoader.load()with the engine parameter, centralizing the loading logic as intended by this refactor.
488-488: LGTM! Correct access to engine-managed render errors.The refactor properly accesses
renderErrorsfrom the engine, aligning with the centralized error tracking approach.
658-696: LGTM! WebGL context restoration logic is correct.The pattern of caching compositions on context loss (line 648) and restoring them (line 660) is appropriate for handling WebGL context recovery. The cache is properly cleared after restoration at line 685.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/effects/src/player.ts (1)
749-749: Unsafe type assertion to GLEngine.The code assumes
this.engineis aGLEnginewithout runtime verification. If a different engine implementation is used, accessing.contextwill fail.Apply this diff to add a runtime check:
- if (this.canvas instanceof HTMLCanvasElement && (this.engine as GLEngine).context) { + if (this.canvas instanceof HTMLCanvasElement && 'context' in this.engine && this.engine.context) {
🧹 Nitpick comments (2)
packages/effects-core/src/scene-loader.ts (1)
33-35: Consider adding a timeout safeguard for shader compilation.The
compileAllShaderscallback could theoretically hang indefinitely if the library fails to invoke the callback. While rare, adding a timeout would make the loading more robust.Apply this diff to add a timeout:
- await new Promise(resolve => { - engine.renderer.getShaderLibrary()?.compileAllShaders(() => resolve(null)); - }); + await Promise.race([ + new Promise(resolve => { + engine.renderer.getShaderLibrary()?.compileAllShaders(() => resolve(null)); + }), + new Promise((_, reject) => + setTimeout(() => reject(new Error('Shader compilation timeout')), 30000) + ), + ]);packages/effects/src/player.ts (1)
377-383: Minor formatting: Add spaces after semicolons in for loop.The for loop declaration is missing spaces after semicolons. While not critical, this is inconsistent with standard JavaScript/TypeScript formatting conventions.
Apply this diff:
- for (let i = 0;i < compositions.length; i++) { + for (let i = 0; i < compositions.length; i++) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/effects-core/src/scene-loader.ts(1 hunks)packages/effects/src/player.ts(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/effects-core/src/scene-loader.ts (5)
packages/effects-core/src/scene.ts (3)
Scene(13-38)LoadType(46-46)SceneLoadOptions(65-143)packages/effects-core/src/engine.ts (1)
Engine(30-359)packages/effects-core/src/composition.ts (1)
Composition(132-1032)packages/effects-core/src/asset-manager.ts (1)
AssetManager(24-473)packages/effects-core/src/constants.ts (1)
PLAYER_OPTIONS_ENV_EDITOR(8-8)
🪛 Biome (2.1.2)
packages/effects/src/player.ts
[error] 364-364: Invalid decorator handleThrowError<Player>
(parse)
🔇 Additional comments (4)
packages/effects-core/src/scene-loader.ts (1)
52-73: LGTM!The composition creation logic is clean and correctly wires the engine context. The FPS throttling for mid/low-end devices and the conditional shader variant creation are appropriate optimizations.
packages/effects/src/player.ts (3)
71-97: LGTM!The getter-based abstraction provides clean access to engine properties while maintaining backward compatibility for the public API.
676-726: LGTM!The context loss/restoration handling correctly caches compositions, reloads them via
loadScene, and preserves their state (time, speed, transforms, video states).
155-163: ****Both
this.engine.envandthis.renderer.envassignments are necessary and not redundant. Theenvproperty is accessed independently via engine references in scene-loader.ts and via renderer references in composition.ts, so setting both ensures all code paths have the environment value available.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/effects-webgl/src/gl-engine.ts (1)
60-69: Disposal order corrected.The disposal order now correctly disposes GPU consumers (renderer, shaderLibrary) before the GL context, addressing the previous critical issue.
packages/effects/src/player.ts (1)
662-662: Unsafe type assertion to GLEngine.The code assumes
this.engineis aGLEnginewithout runtime verification. If a different engine implementation is used, accessing.contextwill fail.Apply this diff to add a runtime check:
- if (this.canvas instanceof HTMLCanvasElement && (this.engine as GLEngine).context) { + if (this.canvas instanceof HTMLCanvasElement && 'context' in this.engine && this.engine.context) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/effects-core/src/constants.ts(1 hunks)packages/effects-core/src/engine.ts(6 hunks)packages/effects-webgl/src/gl-engine.ts(3 hunks)packages/effects-webgl/src/gl-shader-library.ts(1 hunks)packages/effects/src/player.ts(18 hunks)packages/effects/src/types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/effects-webgl/src/gl-engine.ts (4)
packages/effects-webgl/src/gl-context-manager.ts (1)
GLContextManager(4-67)packages/effects-core/src/engine.ts (1)
EngineOptions(22-27)packages/effects-core/src/utils/asserts.ts (1)
assertExist(1-5)packages/effects-webgl/src/gl-renderer.ts (1)
GLRenderer(21-350)
packages/effects/src/player.ts (3)
packages/effects-core/src/engine.ts (1)
Engine(32-502)packages/effects-core/src/scene-loader.ts (1)
SceneLoader(9-74)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-657)
packages/effects-core/src/engine.ts (6)
packages/effects-core/src/gl/create-gl-context.ts (1)
GLType(1-1)packages/effects-core/src/composition.ts (2)
Composition(132-1032)addItem(582-585)packages/effects-core/src/asset-service.ts (1)
AssetService(14-142)packages/effects-core/src/plugins/interact/event-system.ts (1)
EventSystem(28-181)packages/effects-core/src/ticker.ts (1)
Ticker(8-141)packages/effects-core/src/utils/device.ts (1)
getPixelRatio(1-11)
🪛 Biome (2.1.2)
packages/effects/src/player.ts
[error] 395-395: Invalid decorator handleThrowError<Player>
(parse)
packages/effects-core/src/engine.ts
[error] 251-252: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
packages/effects/src/types.ts (1)
72-74: Remove incorrect migration guidance from deprecation comment.The original suggestion to migrate to "the new Engine class's GPU timing API" is incorrect—no such API exists in the Engine class. The property
reportGPUTimeis not destructured or used in the Player constructor, and no replacement implementation is present in the codebase.Simply add the deprecation tag without a non-existent migration path, or verify the actual replacement strategy before suggesting one:
/** * 每帧渲染调用后的回调，WebGL2 上下文生效 * @param time - GPU 渲染使用的时间，秒 * @deprecated 2.7.0 */ reportGPUTime?: (time: number) => void,If this property is being removed entirely, the deprecation tag alone is sufficient. If a replacement exists or is planned, clarify the migration path before documenting it.
Likely an incorrect or invalid review comment.
packages/effects-webgl/src/gl-shader-library.ts (1)
145-145: LGTM: Lifecycle flag rename.The change from
isDestroyedtoisDisposedcorrectly aligns with the broader engine lifecycle refactor.packages/effects-core/src/constants.ts (1)
13-16: LGTM: Documentation links added.The new HELP_LINK entries provide useful documentation references for container sizing and DPI validation errors.
packages/effects-core/src/engine.ts (3)
22-27: LGTM: Well-structured engine options.The
EngineOptionsinterface cleanly extendsWebGLContextAttributesand adds rendering configuration options.
93-107: LGTM: Engine initialization.The constructor correctly initializes core services (ticker, eventSystem, assetLoader, assetService) and sets up the render loop when
manualRenderis not enabled.
265-314: Based on the verification, the existing tests comprehensively cover the resize logic and container size validation edge cases. The test suite includes scenarios for extreme aspect ratios (such as 1:18), oversized containers clamped to max texture sizes (both with and without environment specifications), and pixelRatio variations.Most importantly, the code includes a protective guard (
if (containerWidth && containerHeight)) before the aspect ratio division, preventing any division-by-zero errors. The DPI overflow and container size overflow checks are validated through error logging with helper links, and the maxSize clamping logic correctly handles extreme cases by preserving aspect ratios during downscaling.
packages/effects/src/player.ts (3)
59-67: LGTM: Engine-backed accessors.The
tickerandrenderergetters correctly proxy to the engine instance.
179-200: LGTM: Engine initialization with callbacks.The engine is correctly initialized with the required options and lifecycle callbacks (
onRenderError,onRenderCompositions).
389-421: LGTM: Scene loading refactored.The scene loading logic correctly uses
SceneLoader.loadand manages compositions through the engine.packages/effects-webgl/src/gl-engine.ts (1)
31-58: Verify initialization sequence after context manager integration.The constructor creates GLContextManager, extracts
gl, then initializes gpuCapability, shaderLibrary, and renderer sequentially. Theresize()call at the end requires a fully initialized renderer.Run the following script to verify GLRenderer's constructor dependencies and confirm it doesn't access GL before engine initialization completes:
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/effects/src/player.ts (1)
660-660: Unsafe type assertion requires runtime check.The code assumes
this.engineis aGLEnginewithout verifying. If a different engine implementation is used, accessing.contextwill fail at runtime.Apply this diff to add a runtime check:
- if (this.canvas instanceof HTMLCanvasElement && (this.engine as GLEngine).context) { + if (this.canvas instanceof HTMLCanvasElement && 'context' in this.engine && this.engine.context) {packages/effects-core/src/engine.ts (1)
222-266: Fix void return type violation on line 250.The function
renderCompositionsis declared to returnvoid, but line 250 usesreturn this.ticker?.pause(). The return value frompause()should not be returned.Apply this diff:
if (skipRender) { this.onRenderError?.(new Error('Play when texture offloaded.')); - return this.ticker?.pause(); + this.ticker?.pause(); + return; }
🧹 Nitpick comments (2)
packages/effects-core/src/plugins/particle/trail-mesh.ts (1)
90-99: Avoid nullish destructure; use direct Engine.env for stronger typing.engine is non-null here; destructuring with engine ?? {} can mask wiring bugs and widens env to possibly-undefined.
Apply:
- const { env } = engine ?? {}; + const env = engine.env;packages/effects-core/src/plugins/particle/particle-mesh.ts (1)
148-165: Use direct Engine.env instead of nullish destructure.Keeps typing strict and consistent with other modules.
- const { env } = engine ?? {}; + const env = engine.env;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/effects-core/src/composition.ts(8 hunks)packages/effects-core/src/engine.ts(6 hunks)packages/effects-core/src/plugins/interact/interact-item.ts(2 hunks)packages/effects-core/src/plugins/interact/interact-mesh.ts(1 hunks)packages/effects-core/src/plugins/particle/particle-mesh.ts(1 hunks)packages/effects-core/src/plugins/particle/trail-mesh.ts(1 hunks)packages/effects-core/src/render/renderer.ts(2 hunks)packages/effects-threejs/src/three-renderer.ts(1 hunks)packages/effects/src/player.ts(18 hunks)plugin-packages/spine/src/spine-component.ts(1 hunks)plugin-packages/spine/src/spine-mesh.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
packages/effects-core/src/plugins/interact/interact-mesh.ts (1)
packages/effects-core/src/constants.ts (1)
PLAYER_OPTIONS_ENV_EDITOR(8-8)
packages/effects-core/src/render/renderer.ts (2)
packages/effects-core/src/engine.ts (1)
Engine(33-522)packages/effects-core/src/render/render-frame.ts (1)
RenderingData(37-50)
packages/effects-core/src/composition.ts (1)
packages/effects-core/src/constants.ts (1)
PLAYER_OPTIONS_ENV_EDITOR(8-8)
packages/effects-core/src/engine.ts (11)
packages/effects-core/src/gl/create-gl-context.ts (1)
GLType(1-1)packages/effects-core/src/utils/index.ts (3)
Disposable(36-38)LostHandler(44-46)RestoreHandler(40-42)packages/effects-core/src/composition.ts (2)
Composition(132-1032)addItem(582-585)packages/effects-core/src/asset-manager.ts (1)
AssetManager(24-473)packages/effects-core/src/asset-service.ts (1)
AssetService(14-142)packages/effects-core/src/plugins/interact/event-system.ts (1)
EventSystem(28-181)packages/effects-core/src/ticker.ts (1)
Ticker(8-141)packages/effects-core/src/texture/texture.ts (2)
generateWhiteTexture(235-249)generateTransparentTexture(251-265)packages/effects-core/src/asset-loader.ts (1)
AssetLoader(13-71)packages/effects-core/src/utils/device.ts (1)
getPixelRatio(1-11)packages/effects-core/src/constants.ts (1)
HELP_LINK(10-17)
packages/effects/src/player.ts (3)
packages/effects-core/src/engine.ts (1)
Engine(33-522)packages/effects-core/src/scene-loader.ts (1)
SceneLoader(9-74)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-657)
packages/effects-threejs/src/three-renderer.ts (2)
packages/effects-core/src/render/renderer.ts (1)
Renderer(12-139)packages/effects-core/src/engine.ts (1)
Engine(33-522)
🪛 Biome (2.1.2)
packages/effects-core/src/engine.ts
[error] 257-258: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
packages/effects/src/player.ts
[error] 393-393: Invalid decorator handleThrowError<Player>
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (20)
plugin-packages/spine/src/spine-component.ts (1)
328-345: Env source move looks good; please confirm Engine.env is always initialized.Switching to this.engine.env aligns with the refactor. If Engine.env can be undefined during early lifecycle, hit-testing in editor may silently disable. Verify initialization order.
plugin-packages/spine/src/spine-mesh.ts (1)
160-165: Shader macro env source updated — consistent with Engine API.Looks correct. Please ensure all shader creators now read env from Engine for consistency with this change.
packages/effects-core/src/plugins/interact/interact-mesh.ts (1)
95-100: LGTM — env check moved to Engine.Change is correct and consistent with the new Engine-centric model.
packages/effects-core/src/render/renderer.ts (3)
21-23: Constructor change verified—all Renderer subclasses correctly call super(engine).Both direct subclasses of Renderer have been verified:
- GLRenderer calls super(engine) in its constructor
- ThreeRenderer calls super(engine) in its constructor
No issues found.
136-138: Review comment is incorrect; no signature narrowing occurred.The
Renderer.dispose()method currently accepts no parameters:dispose (): void. TheGLRendereroverride also accepts no parameters,ThreeRendererdoes not override it, and all call sites in the codebase invokerenderer.dispose()without arguments. There is no evidence of ahaltGLparameter or any broader signature being narrowed. No audit or migration is necessary.Likely an incorrect or invalid review comment.
13-16: No changes required—implementation already correct.The API change is already properly implemented across the codebase:
- Renderer.create is assigned to
(engine: Engine) => new GLRenderer(engine)in packages/effects/src/index.ts- Both GLRenderer and ThreeRenderer constructors accept Engine parameter
- GLEngine and ThreeEngine both instantiate renderers with the correct Engine parameter
All call sites are already compatible with the new signature.
Likely an incorrect or invalid review comment.
packages/effects-core/src/plugins/interact/interact-item.ts (1)
119-119: LGTM! Environment access correctly refactored.The change from
this.item.engine.renderer.envtothis.item.engine.envcorrectly aligns with the Engine-centric architecture whereenvis now a direct property of Engine rather than accessed through the Renderer.Also applies to: 151-151
packages/effects-threejs/src/three-renderer.ts (1)
5-6: LGTM! Constructor signature correctly updated.The constructor now accepts an
Engineinstance and delegates to the parentRendererclass viasuper(engine), which aligns with the Engine-centric architecture and matches the baseRendererclass signature shown in the relevant code snippets.packages/effects/src/player.ts (4)
47-47: LGTM! Engine field correctly declared.The
readonly engine: Enginefield properly encapsulates the Engine instance and prevents accidental reassignment, which is a good practice for this architectural refactor.
59-67: LGTM! Delegation pattern correctly implemented.The public getters for
ticker,renderer, andgpuCapabilitycorrectly delegate to the internalengineinstance, maintaining the Player's public API while centralizing state in the Engine.
179-201: LGTM! Engine initialization correctly implemented.The Engine is properly created with the canvas and options, then configured with environment, name, callbacks, and event handlers. The pattern of registering lost/restore handlers aligns with the Engine-centric lifecycle management.
393-404: LGTM! Scene loading correctly migrated to SceneLoader.The refactored
loadScenecorrectly usesSceneLoader.loadwith the engine, and properly setscomposition.onItemMessageto wire up message events. The async/await pattern and composition management look correct.packages/effects-core/src/composition.ts (4)
108-108: LGTM! onItemMessage correctly made optional with proper initialization.The refactor from
handleItemMessageto the optionalonItemMessageis correctly implemented:
- The prop is properly typed as optional in
CompositionProps- The field is declared as optional in the class
- Conditional assignment in the constructor only sets it when provided
Also applies to: 231-236, 375-377
813-819: LGTM! Optional chaining correctly handles undefined onItemMessage.The invocations use optional chaining (
this.onItemMessage?.({...})) which safely handles the case whenonItemMessageis undefined. This addresses the concerns from previous reviews about null checks.Also applies to: 837-843
302-302: LGTM! Composition correctly registers/unregisters with Engine.The composition properly registers itself with the engine during construction (line 302) and removes itself during disposal (line 929), ensuring proper lifecycle management in the Engine-centric architecture.
Also applies to: 929-929
918-918: LGTM! Environment access correctly refactored.Changed from
this.renderer.envtothis.getEngine().env, aligning with the Engine-centric architecture whereenvis now a property of Engine.packages/effects-core/src/engine.ts (4)
22-28: LGTM! EngineOptions interface well-defined.The
EngineOptionsinterface appropriately extendsWebGLContextAttributesand adds engine-specific options likemanualRender,glType,fps,pixelRatio, andnotifyTouch. This provides a clean API for engine configuration.
94-113: LGTM! Engine constructor correctly implemented.The constructor properly initializes the engine with:
- Canvas and basic state
- Optional ticker (unless
manualRenderis true)- Event system with touch propagation configuration
- Asset loader and service
- Pixel ratio with sensible default
457-469: LGTM! Composition management methods correctly implemented.The
addCompositionandremoveCompositionmethods properly guard against disposed state and use theaddItem/removeItemutilities to manage the compositions array.
478-521: LGTM! Disposal correctly updated to Engine-centric model.The
disposemethod correctly:
- Checks and sets the
disposedflag (renamed fromdestroyed)- Stops ticker and disposes event system and asset service
- Disposes all tracked resources including the new
assetManagersandcompositionsarrays- Clears resource arrays
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/effects/src/player.ts (1)
673-673: Unsafe type assertion to GLEngine remains.The code still assumes
this.engineis aGLEnginewithout checking. If a different engine implementation is used, accessing.contextwill fail at runtime. Consider using a type guard or checking for the property's existence.Apply this diff to safely check for the context property:
- if (this.canvas instanceof HTMLCanvasElement && (this.engine as GLEngine).context) { + if (this.canvas instanceof HTMLCanvasElement && 'context' in this.engine && this.engine.context) {Or implement a type guard:
function isGLEngine(engine: Engine): engine is GLEngine { return 'context' in engine; } // Usage: if (this.canvas instanceof HTMLCanvasElement && isGLEngine(this.engine)) { // Safe to access this.engine.context }packages/effects-core/src/engine.ts (1)
257-257: Fix void return type violation.The function returns
this.ticker?.pause()butrenderCompositionshas avoidreturn type. Split this into two statements to respect the function signature.Apply this diff:
if (skipRender) { this.onRenderError?.(new Error('Play when texture offloaded.')); - return this.ticker?.pause(); + this.ticker?.pause(); + return; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/effects-core/src/comp-vfx-item.ts(2 hunks)packages/effects-core/src/engine.ts(6 hunks)packages/effects-core/src/plugins/interact/click-handler.ts(2 hunks)packages/effects-webgl/src/gl-shader-library.ts(1 hunks)packages/effects/src/player.ts(17 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
packages/effects-core/src/engine.ts (10)
packages/effects-core/src/gl/create-gl-context.ts (1)
GLType(1-1)packages/effects-core/src/utils/index.ts (3)
Disposable(36-38)LostHandler(44-46)RestoreHandler(40-42)packages/effects-core/src/composition.ts (1)
Composition(132-1032)packages/effects-core/src/asset-manager.ts (1)
AssetManager(24-473)packages/effects-core/src/asset-service.ts (1)
AssetService(14-142)packages/effects-core/src/plugins/interact/event-system.ts (3)
EventSystem(28-181)EVENT_TYPE_CLICK(4-4)TouchEventType(9-20)packages/effects-core/src/ticker.ts (1)
Ticker(8-141)packages/effects-core/src/utils/device.ts (1)
getPixelRatio(1-11)packages/effects-core/src/constants.ts (1)
HELP_LINK(10-17)packages/effects-core/src/plugins/interact/click-handler.ts (1)
Region(58-67)
packages/effects-core/src/comp-vfx-item.ts (1)
packages/effects-core/src/composition.ts (1)
Composition(132-1032)
packages/effects-core/src/plugins/interact/click-handler.ts (1)
packages/effects-core/src/composition.ts (1)
Composition(132-1032)
packages/effects/src/player.ts (5)
packages/effects-core/src/engine.ts (1)
Engine(37-565)packages/effects-core/src/composition.ts (1)
Composition(132-1032)packages/effects-core/src/render/renderer.ts (1)
Renderer(12-139)packages/effects-core/src/scene-loader.ts (1)
SceneLoader(9-74)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-657)
🪛 Biome (2.1.2)
packages/effects-core/src/engine.ts
[error] 264-265: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
packages/effects/src/player.ts
[error] 406-406: Invalid decorator handleThrowError<Player>
(parse)
🔇 Additional comments (6)
packages/effects-webgl/src/gl-shader-library.ts (1)
145-145: LGTM! Lifecycle flag updated consistently.The guard check now uses
this.engine.disposedinstead of the oldisDestroyedflag, aligning with the engine lifecycle refactoring across the codebase.packages/effects-core/src/plugins/interact/click-handler.ts (1)
4-4: LGTM! Region type extended to include composition reference.The addition of the
composition: Compositionfield to theRegiontype enables regions to carry a reference to their composition, supporting richer interaction handling. This aligns with the changes incomp-vfx-item.tswhere the field is populated during hit-testing.Also applies to: 66-66
packages/effects/src/player.ts (1)
178-219: LGTM! Engine initialization and event wiring.The Player now properly delegates to Engine for core functionality. Engine initialization with options, event wiring (
onRenderError,onRenderCompositions,onClick), and lifecycle handlers (lost,restore) are all correctly configured.packages/effects-core/src/engine.ts (3)
24-32: LGTM! Well-structured engine options interface.The
EngineOptionsinterface appropriately extendsWebGLContextAttributesand provides optional configuration for manual rendering, GL type, FPS, pixel ratio, touch notifications, interactivity, and environment. This enables flexible engine initialization.
278-327: LGTM! Comprehensive resize implementation.The
resize()method properly computes canvas dimensions based on the parent container, handles display aspect and scale, validates against max texture sizes, and propagates dimension updates to all compositions' cameras. Error handling for DPI overflow and invalid container sizes is appropriate.
500-512: LGTM! Consistent composition lifecycle management.The
addCompositionandremoveCompositionmethods follow the established pattern for resource management in the Engine, with appropriate disposal guards. This enables proper tracking of compositions in the engine's lifecycle.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/effects-core/src/engine.ts (1)
226-270: Fix void return on line 254.The function returns
this.ticker?.pause()butrenderCompositionshas a void return type. Use separate statements instead.Apply this diff:
if (skipRender) { this.onRenderError?.(new Error('Play when texture offloaded.')); - return this.ticker?.pause(); + this.ticker?.pause(); + return; }
🧹 Nitpick comments (1)
packages/effects-core/src/plugins/interact/event-system.ts (1)
147-180: Translate the Chinese comment to English.The click handling logic is well-structured and correct. However, the comment on line 154 should be translated to English for consistency with the codebase.
Apply this diff:
- // 收集所有的点击测试结果,click 回调执行可能会对 composition 点击结果有影响,放在点击测试执行完后再统一触发。 + // Collect all hit test results; click callbacks may affect composition click results, so trigger them after all hit tests complete. this.engine.compositions.forEach(composition => {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/effects-core/src/engine.ts(6 hunks)packages/effects-core/src/plugins/interact/event-system.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/effects-core/src/plugins/interact/event-system.ts (3)
packages/effects-core/src/engine.ts (1)
Engine(36-528)packages/effects-core/src/plugins/interact/click-handler.ts (1)
Region(58-67)packages/effects-core/src/composition.ts (1)
Composition(132-1032)
packages/effects-core/src/engine.ts (10)
packages/effects-core/src/gl/create-gl-context.ts (1)
GLType(1-1)packages/effects-core/src/utils/index.ts (3)
Disposable(36-38)LostHandler(44-46)RestoreHandler(40-42)packages/effects-core/src/composition.ts (1)
Composition(132-1032)packages/effects-core/src/asset-manager.ts (1)
AssetManager(24-473)packages/effects-core/src/asset-service.ts (1)
AssetService(14-142)packages/effects-core/src/plugins/interact/event-system.ts (1)
EventSystem(32-221)packages/effects-core/src/ticker.ts (1)
Ticker(8-141)packages/effects-core/src/utils/device.ts (1)
getPixelRatio(1-11)packages/effects-core/src/constants.ts (1)
HELP_LINK(10-17)packages/effects-core/src/plugins/interact/click-handler.ts (1)
Region(58-67)
🪛 Biome (2.1.2)
packages/effects-core/src/engine.ts
[error] 261-262: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
packages/effects-core/src/plugins/interact/event-system.ts (3)
1-6: LGTM!The new imports support the refactored constructor and click handling logic.
37-42: LGTM!The refactored constructor now accepts
Engineinstead ofHTMLCanvasElement, aligning with the PR objective to centralize runtime under Engine. The canvas target is now set viabindListeners, providing cleaner separation of initialization concerns.
44-45: LGTM!The updated
bindListenerssignature correctly accepts and stores the target canvas.packages/effects-core/src/engine.ts (6)
23-31: LGTM!The
EngineOptionsinterface is well-designed and provides appropriate configuration options for the new Engine API.
97-117: LGTM!The constructor implementation correctly initializes all engine components, including conditional ticker creation for non-manual rendering and proper EventSystem setup.
272-375: LGTM!The
resizemethod and its helpergetTargetSizehandle complex container sizing, DPI constraints, and camera updates comprehensively. The length is justified by the thoroughness of the logic.
463-475: LGTM!The composition management methods correctly guard against disposed state and maintain the compositions array.
393-461: LGTM!All resource management methods correctly guard against the disposed state, maintaining consistency across the engine lifecycle.
484-527: LGTM!The
disposemethod correctly cleans up all engine resources, including the newly added ticker, eventSystem, assetService, assetManagers, and compositions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/effects/src/player.ts (1)
682-682: Unsafe type assertion to GLEngine.This concern was already raised in a previous review. The code assumes
this.engineis aGLEnginewithout runtime checks.packages/effects-core/src/engine.ts (1)
260-264: Fix void return on Line 263.This issue was already identified in a previous review. The function returns
this.ticker?.pause()but has a void return type.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/effects-core/src/engine.ts(7 hunks)packages/effects-core/src/render/renderer.ts(2 hunks)packages/effects-webgl/src/gl-engine.ts(3 hunks)packages/effects-webgl/src/gl-renderer.ts(2 hunks)packages/effects/src/player.ts(17 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/effects-core/src/render/renderer.ts (2)
packages/effects-core/src/engine.ts (1)
Engine(44-521)packages/effects-core/src/render/render-frame.ts (1)
RenderingData(37-50)
packages/effects-webgl/src/gl-renderer.ts (2)
packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-667)packages/effects-core/src/engine.ts (1)
Engine(44-521)
packages/effects-core/src/engine.ts (13)
packages/effects-core/src/gl/create-gl-context.ts (1)
GLType(1-1)packages/effects-core/src/events/event-emitter.ts (1)
EventEmitter(19-91)packages/effects-core/src/utils/index.ts (1)
Disposable(36-38)packages/effects-core/src/composition.ts (1)
Composition(132-1032)packages/effects-core/src/asset-manager.ts (1)
AssetManager(24-473)packages/effects-core/src/asset-service.ts (1)
AssetService(14-142)packages/effects-core/src/plugins/interact/event-system.ts (1)
EventSystem(32-221)packages/effects-core/src/ticker.ts (1)
Ticker(8-141)packages/effects-core/src/plugins/interact/click-handler.ts (1)
Region(58-67)packages/effects-core/src/texture/texture.ts (2)
generateWhiteTexture(235-249)generateTransparentTexture(251-265)packages/effects-core/src/asset-loader.ts (1)
AssetLoader(13-71)packages/effects-core/src/utils/device.ts (1)
getPixelRatio(1-11)packages/effects-core/src/constants.ts (1)
HELP_LINK(10-17)
packages/effects-webgl/src/gl-engine.ts (5)
packages/effects-webgl/src/gl-context-manager.ts (1)
GLContextManager(4-67)packages/effects-webgl/src/gl-renderer-internal.ts (1)
canvas(72-74)packages/effects-core/src/engine.ts (1)
EngineOptions(24-32)packages/effects-core/src/utils/asserts.ts (1)
assertExist(1-5)packages/effects-webgl/src/gl-renderer.ts (1)
GLRenderer(21-342)
packages/effects/src/player.ts (4)
packages/effects-core/src/engine.ts (1)
Engine(44-521)packages/effects-core/src/composition.ts (2)
Composition(132-1032)MessageItem(46-63)packages/effects-core/src/scene-loader.ts (1)
SceneLoader(9-74)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-667)
🪛 Biome (2.1.2)
packages/effects-core/src/engine.ts
[error] 270-271: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
packages/effects/src/player.ts
[error] 415-415: Invalid decorator handleThrowError<Player>
(parse)
🔇 Additional comments (8)
packages/effects-core/src/render/renderer.ts (1)
13-13: LGTM: Engine-centric renderer initialization.The migration from canvas-based factory methods to Engine-based initialization aligns with the PR's architectural goals. The simplified
dispose()signature (removinghaltGLparameter) is cleaner, as lifecycle management now resides in the Engine.Also applies to: 21-23, 118-120
packages/effects/src/player.ts (2)
130-245: Well-structured Engine initialization and event wiring.The constructor properly:
- Distinguishes between external and internally-created canvases (Line 159-171)
- Creates the Engine with appropriate options (Lines 179-192)
- Wires engine events to player events (Lines 194-228)
- Handles cleanup correctly on error (Line 240)
48-124: Clean delegation pattern to Engine.The accessor pattern effectively:
- Maintains backward compatibility for public APIs (ticker, renderer, gpuCapability)
- Encapsulates engine state access through private getters/setters
- Provides clear separation between Player's public surface and internal Engine state
packages/effects-core/src/engine.ts (2)
105-126: Well-organized Engine initialization.The constructor properly:
- Initializes core textures (whiteTexture, transparentTexture)
- Conditionally creates ticker based on
manualRenderoption (Lines 114-117)- Sets up EventSystem with correct configuration (Lines 119-121)
- Initializes asset management services (Lines 123-125)
284-333: Robust resize implementation with comprehensive validation.The resize logic effectively handles:
- Parent container size calculation with fallbacks (Lines 285-302)
- DPI overflow detection (Lines 307-309)
- Container size overflow with aspect-preserving adjustment (Lines 312-321)
- Camera property updates for all compositions (Lines 327-331)
Helpful error messages with HELP_LINK references guide users to documentation.
packages/effects-webgl/src/gl-renderer.ts (1)
27-29: Clean integration with Engine-managed GL context.The refactor successfully:
- Delegates context access to the Engine (Lines 27-29)
- Simplifies constructor to accept a single Engine parameter (Lines 31-32)
- Removes context disposal responsibility from GLRenderer (Lines 296-301)
The type assertion
(this.engine as GLEngine)is safe here since GLRenderer is only instantiated by GLEngine.Also applies to: 31-53, 296-301
packages/effects-webgl/src/gl-engine.ts (2)
31-68: Proper GLContextManager integration and initialization sequence.The constructor correctly:
- Merges sensible default options (Lines 33-42)
- Creates GLContextManager and wires context loss/restore events (Lines 44-54)
- Follows proper initialization order: context → capabilities → shader library → renderer → resize (Lines 56-67)
- Calls resize after renderer initialization, ensuring the renderer is ready (Lines 66-67)
70-80: Correct disposal order resolves previous concern.The disposal sequence now correctly disposes GPU consumers before the GL context:
renderer.dispose()(Line 76)shaderLibrary?.dispose()(Line 77)context.dispose()(Line 78)This resolves the critical issue flagged in the previous review, where the context was being disposed before the renderer, potentially breaking GLRendererInternal cleanup.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/effects/src/player.ts (2)
388-388: Fix invalid decorator syntax.The decorator
@handleThrowError<Player>uses generic type parameter syntax, which is not valid in TypeScript. Decorators cannot accept type parameters directly.Apply this diff:
- @handleThrowError<Player> + @handleThrowError async loadScene (Alternatively, if type information is essential, convert
handleThrowErrorinto a decorator factory that can be invoked with a generic parameter.
682-682: Unsafe type assertion to GLEngine.The code assumes
this.engineis aGLEnginewithout checking. If a different engine implementation is used in the future, accessing.contextwill fail.Apply this diff to safely check for the context:
- if (this.canvas instanceof HTMLCanvasElement && (this.engine as GLEngine).context) { + if (this.canvas instanceof HTMLCanvasElement && 'context' in this.engine && this.engine.context) {This uses a runtime check for the
contextproperty before accessing it, avoiding hard cast errors.packages/effects-core/src/engine.ts (1)
259-263: Fix void return type violation.The function returns
this.ticker?.pause()butrenderhas a void return type. This violates the function signature.Apply this diff:
if (skipRender) { this.emit('rendererror', new Error('Play when texture offloaded.')); - return this.ticker?.pause(); + this.ticker?.pause(); + return; }
🧹 Nitpick comments (1)
packages/effects-core/src/engine.ts (1)
85-86: Consider makingonClickprivate or protected.The TODO comment indicates this field should eventually be replaced with composition click events. Since it's transitional, consider reducing its visibility to
privateor marking it with@internalJSDoc to discourage external usage.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/effects-core/src/engine.ts(7 hunks)packages/effects/src/player.ts(17 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/effects-core/src/engine.ts (9)
packages/effects-core/src/gl/create-gl-context.ts (1)
GLType(1-1)packages/effects-core/src/events/event-emitter.ts (1)
EventEmitter(19-91)packages/effects-core/src/composition.ts (2)
Composition(132-1032)addItem(582-585)packages/effects-core/src/asset-service.ts (1)
AssetService(14-142)packages/effects-core/src/plugins/interact/event-system.ts (1)
EventSystem(32-221)packages/effects-core/src/ticker.ts (1)
Ticker(8-141)packages/effects-core/src/plugins/interact/click-handler.ts (1)
Region(58-67)packages/effects-core/src/utils/device.ts (1)
getPixelRatio(1-11)packages/effects-core/src/constants.ts (1)
HELP_LINK(10-17)
packages/effects/src/player.ts (5)
packages/effects-core/src/engine.ts (1)
Engine(43-518)packages/effects-core/src/plugins/interact/click-handler.ts (1)
Region(58-67)packages/effects-core/src/scene.ts (2)
Scene(13-38)LoadType(46-46)packages/effects-core/src/scene-loader.ts (1)
SceneLoader(9-74)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-667)
🪛 Biome (2.1.2)
packages/effects-core/src/engine.ts
[error] 269-270: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
packages/effects/src/player.ts
[error] 415-415: Invalid decorator handleThrowError<Player>
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
packages/effects-core/src/engine.ts (4)
24-38: LGTM! Well-structured options and events.The
EngineOptionsinterface cleanly consolidates engine configuration, andEngineEventproperly types the event system for context loss/restore and render errors.
104-125: LGTM! Clean engine initialization.The constructor properly initializes all services, conditionally creates the ticker based on
manualRender, and sets up the event system with appropriate defaults.
453-465: LGTM! Proper composition lifecycle management.The
addCompositionandremoveCompositionmethods correctly guard against disposed state and follow the same pattern as other resource management methods.
474-517: LGTM! Comprehensive disposal logic.The disposal method correctly:
- Guards against double disposal
- Stops the ticker before resource cleanup
- Disposes services and resources in proper order
- Clears all collections
packages/effects/src/player.ts (5)
2-18: LGTM! Imports aligned with Engine-centric architecture.The imports correctly bring in Engine and related types, replacing the previous renderer-centric imports.
118-124: Verify if pixelRatio changes require resize.The
pixelRatiosetter updatesengine.pixelRatiobut doesn't trigger a resize. If changing pixelRatio should affect canvas dimensions, callers need to explicitly callresize()afterward. Consider documenting this requirement or triggering resize automatically.
179-228: LGTM! Clean Engine integration with proper event wiring.The constructor correctly:
- Creates Engine with appropriate options
- Binds engine events to player event handlers
- Sets up render loop callback for autoPlaying state
- Handles onClick behavior routing (NOTIFY, RESUME_PLAYER)
564-603: LGTM! Proper delegation to Engine.The
tick,resize, andclearCanvasmethods correctly delegate to engine methods, simplifying the player's rendering logic.
609-648: LGTM! Proper context loss/restore handling.The
lostandrestoremethods correctly cache compositions before context loss and restore them afterward with preserved state (time, speed, transforms, video state).
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/effects/src/player.ts (2)
388-388: Fix invalid decorator syntax.The decorator
@handleThrowError<Player>uses generic type parameter syntax, which is not valid in TypeScript. This issue was flagged in a previous review.Apply this diff:
- @handleThrowError<Player> + @handleThrowError async loadScene (Alternatively, if type information is essential, convert to a decorator factory:
@handleThrowError<Player>().
640-650: Engine disposal is correct, but type assertion remains unsafe.Line 640 properly calls
engine.dispose(). However, line 642's unsafe type assertion(this.engine as GLEngine).contextwas flagged in a previous review. If a different Engine implementation is used, accessing.contextwill fail.As suggested in the previous review, use a type guard or check for the property:
- if (this.canvas instanceof HTMLCanvasElement && (this.engine as GLEngine).context) { + if (this.canvas instanceof HTMLCanvasElement && 'context' in this.engine && (this.engine as GLEngine).context) {Or implement a type guard:
function isGLEngine(engine: Engine): engine is GLEngine { return 'context' in engine; }Then use:
- if (this.canvas instanceof HTMLCanvasElement && (this.engine as GLEngine).context) { + if (this.canvas instanceof HTMLCanvasElement && isGLEngine(this.engine) && this.engine.context) {
🧹 Nitpick comments (1)
packages/effects-webgl/src/gl-engine.ts (1)
113-123: Disposal order is correct, but consider consistent null-safety.The disposal order now correctly disposes GPU consumers (renderer, shaderLibrary) before the GL context, which addresses the previous review concern. ✅
However, there's an inconsistency: line 119 calls
this.renderer.dispose()without optional chaining, while line 120 usesthis.shaderLibrary?.dispose(). If the constructor throws before line 106 (e.g., during GLContextManager creation),renderermay not exist whendispose()is called during cleanup.Apply this diff for consistent null-safety:
super.dispose(); - this.renderer.dispose(); + this.renderer?.dispose(); this.shaderLibrary?.dispose(); this.context.dispose();Based on past review comment: The disposal order issue has been properly addressed. ✅
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/effects-webgl/src/gl-engine.ts(3 hunks)packages/effects/src/player.ts(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/effects/src/player.ts (4)
packages/effects-core/src/engine.ts (1)
Engine(44-520)packages/effects-core/src/composition.ts (2)
Composition(132-1032)MessageItem(46-63)packages/effects-core/src/scene-loader.ts (1)
SceneLoader(9-74)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-710)
packages/effects-webgl/src/gl-engine.ts (7)
packages/effects-webgl/src/gl-context-manager.ts (1)
GLContextManager(4-67)packages/effects-core/src/composition.ts (1)
Composition(132-1032)packages/effects-core/src/engine.ts (1)
EngineOptions(24-33)packages/effects-core/src/scene-loader.ts (1)
SceneLoader(9-74)packages/effects-core/src/texture/types.ts (1)
Texture2DSourceOptionsVideo(144-149)packages/effects-core/src/utils/asserts.ts (1)
assertExist(1-5)packages/effects-webgl/src/gl-renderer.ts (1)
GLRenderer(21-342)
🪛 Biome (2.1.2)
packages/effects/src/player.ts
[error] 415-415: Invalid decorator handleThrowError<Player>
(parse)
🔇 Additional comments (8)
packages/effects/src/player.ts (6)
1-24: LGTM! Imports align with Engine-based refactoring.The new imports (Engine, SceneLoader, MessageItem) properly support the refactored Engine-centric architecture.
60-124: LGTM! Clean delegation to Engine properties.The getters and setters properly proxy to the underlying Engine, maintaining the Player's public API while centralizing state in the Engine.
130-245: LGTM! Solid Engine integration in constructor.The Engine is properly initialized with all necessary options, event handlers are correctly bound, and the render loop is set up cleanly. Error handling with the external canvas cleanup is appropriate.
409-441: LGTM! Clean scene loading via SceneLoader.The refactored logic properly:
- Disposes old asset managers before loading
- Uses
SceneLoader.load()with the Engine- Sets up
onItemMessageto forward composition messages to the player- Applies autoplay flags correctly
564-566: LGTM! Correct delegation to Engine.The
tickmethod properly delegates toengine.render(dt), aligning with the Engine-based architecture.
594-603: LGTM! Explicit clear options improve clarity.The refactored
clearCanvasmethod uses explicitTextureLoadActionoptions, making the clearing behavior clear and maintainable.packages/effects-webgl/src/gl-engine.ts (2)
88-93: IOS context restoration workaround is appropriate.This display-toggle workaround is a well-known solution for IOS WebGL context restoration issues. The temporary hide/show forces a layout recalculation that helps IOS properly restore the rendering context.
99-111: Initialization sequence is correct.The initialization order properly ensures dependencies are satisfied:
- GL context extraction and validation
- State reset
- GPU capability detection (requires GL)
- Shader library creation (requires engine)
- Renderer creation (requires all above)
- Texture count calculation (requires GL)
- Final resize (requires initialized renderer)
The comment on line 109 helpfully documents why resize is called last.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/effects/src/player.ts (1)
239-243: Don’t remove user-provided canvas on constructor failure.Logic is inverted; this removes external canvases and leaves internal ones orphaned.
- if (this.canvas && useExternalCanvas) { + if (this.canvas && !useExternalCanvas) { this.canvas.remove(); }
♻️ Duplicate comments (2)
packages/effects/src/player.ts (2)
387-392: Decorator generic is invalid TypeScript; fix usage.Decorators can’t take type parameters. Use the decorator directly (or a factory if implemented).
- @handleThrowError<Player> + @handleThrowError async loadScene (
641-649: Avoid unsafe cast to GLEngine; use a runtime type guard for.context.Hard-casting risks runtime errors with non-GL engines. Check property presence instead.
- if (this.canvas instanceof HTMLCanvasElement && (this.engine as GLEngine).context) { + if (this.canvas instanceof HTMLCanvasElement && hasContext(this.engine)) { // TODO: 数据模版下掉可以由文本模块单独管理 canvasPool.dispose(); // canvas will become a cry emoji in Android if still in dom if (this.canvas.parentNode) { this.canvas.parentNode.removeChild(this.canvas); } this.canvas.remove(); }Add helper (near imports):
function hasContext(e: Engine): e is Engine & { context: unknown } { return e != null && 'context' in (e as any) && Boolean((e as any).context); }
🧹 Nitpick comments (6)
packages/effects/src/player.ts (6)
412-413: Preserve array identity when clearingengine.assetManagers.Reassigning breaks any cached references. Clear in place.
- this.engine.assetManagers = []; + this.engine.assetManagers.length = 0;
220-227: Gate ‘update’ by actual playing state, notautoPlaying.
autoPlayingmay remain true after pause, causing misleading updates.- this.engine.runRenderLoop((dt: number) => { - if (this.autoPlaying) { + this.engine.runRenderLoop((dt: number) => { + const playing = this.compositions.some(c => !c.getPaused()); + if (playing) { this.emit('update', { player: this, - playing: true, + playing, }); } });
518-536: KeepautoPlayingin sync on pause/resume.Prevents stale ‘playing’ signals and aligns state with behavior.
pause (options?: { offloadTexture?: boolean }) { if (this.paused) { return; } + this.autoPlaying = false;async resume () { if (this.resumePending) { return; } @@ this.compositions.map(composition => { composition.resume(); }); this.ticker?.resume(); + this.autoPlaying = true; }Also applies to: 543-557
307-309: Normalizepausedto a boolean.Avoids
undefinedand clarifies manualRender semantics.- get paused () { - return this.ticker?.getPaused(); - } + get paused (): boolean { + return this.ticker ? this.ticker.getPaused() : true; + }
593-601: JSDoc mismatch: parameterimmediateno longer exists.Update docs or reintroduce the parameter. Recommend updating docs for now.
/** * 清空 canvas 的画面 - * @param immediate - 如果立即清理，当前画面将会消失，如果 player 还有合成在渲染，可能出现闪烁 */
630-666: BlockclearCanvasafter dispose to prevent calls on a disposed engine.You already guard other methods; include
clearCanvasas well.this.tick = throwErrorFunc; this.resize = throwErrorFunc; this.loadScene = throwErrorPromiseFunc; this.play = throwErrorPromiseFunc; this.gotoAndPlay = throwErrorPromiseFunc; this.gotoAndStop = throwErrorPromiseFunc; this.playSequence = throwErrorFunc; this.destroyCurrentCompositions = throwErrorFunc; this.resume = throwErrorPromiseFunc; + this.clearCanvas = throwErrorFunc as unknown as typeof this.clearCanvas;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/effects/src/player.ts(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/effects/src/player.ts (3)
packages/effects-core/src/engine.ts (1)
Engine(44-520)packages/effects-core/src/scene-loader.ts (1)
SceneLoader(9-74)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-710)
🪛 Biome (2.1.2)
packages/effects/src/player.ts
[error] 414-414: Invalid decorator handleThrowError<Player>
(parse)
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/effects-core/src/render/render-pass.ts (1)
596-602: Bug: getStencilAttachment calls getDepthTexturegetStencilAttachment returns the stencil texture but calls getDepthTexture, which populates the wrong cache (depthTexture). This can corrupt subsequent depth/stencil reads.
Apply this diff:
- const stencilTexture = framebuffer.getStencilTexture(); - const texture = stencilTexture ? this.getDepthTexture(stencilTexture, framebuffer.externalStorage) : undefined; + const stencilTexture = framebuffer.getStencilTexture(); + const texture = stencilTexture ? this.getStencilTexture(stencilTexture, framebuffer.externalStorage) : undefined;
♻️ Duplicate comments (3)
packages/effects-core/src/engine.ts (3)
382-382: Fix redundant condition check (copy-paste error).Line 382 checks
targetHeight < 1twice instead of validating both dimensions. This means the guard fails to catch cases wheretargetWidth < 1buttargetHeight >= 1.Apply this diff:
- if (targetHeight < 1 || targetHeight < 1) { + if (targetWidth < 1 || targetHeight < 1) {This issue was identified in a past review and remains unfixed.
310-310: Add null-safety for gpuCapability access (issue from past review persists).The code uses
this.envto gate access togpuCapability.detail.maxTextureSize, but this is unsafe because:
- The base
Engineclass never initializesgpuCapability(only subclasses likeGLEnginedo)envbeing truthy doesn't guaranteegpuCapabilityis initialized- This can cause a runtime error if
envis set butgpuCapabilityis undefinedApply this diff:
- const maxSize = this.env ? this.gpuCapability.detail.maxTextureSize : 2048; + const maxSize = this.gpuCapability?.detail?.maxTextureSize ?? 2048;Based on past review feedback that identified this same issue.
237-279: Fix void return type violation (still present).Line 265 returns a value from a void function, which violates the declared return type. Although a past review flagged this and noted it was addressed, the issue remains in the current code.
Apply this diff:
if (skipRender) { this.emit('rendererror', new Error('Play when texture offloaded.')); - return this.ticker?.pause(); + this.ticker?.pause(); + return; }
🧹 Nitpick comments (7)
packages/effects-webgl/src/ext-wrap.ts (1)
163-165: Consider documenting the deprecation reason.The
@deprecatedannotation would be more helpful with context explaining why this class is deprecated and what approach should be used instead for future development.Example:
/** - * @deprecated + * @deprecated Internal legacy class for WebGL1 copy operations. + * This class is only used for backward compatibility with WebGL1 rendering paths. + * New code should use the WebGL2 copy path instead. */packages/effects-core/src/render/render-pass.ts (1)
334-338: Expose viewportScale to subclasses — OKMaking viewportScale protected is appropriate for subclass resize logic (e.g., DrawObjectPass). Keep it write-controlled in base only. Consider a protected getter if you want to discourage subclass mutation.
packages/effects-core/src/render/render-frame.ts (1)
261-268: Wiring to DrawObjectPass and level-aware Gaussian passes — looks good
- Switching to DrawObjectPass aligns with engine-driven resize.
- Passing level into Gaussian passes matches their new onResize math.
- Deprecation tag for FinalCopyRP noted.
Two checks:
- Gaussian size math can hit 0 for very small canvases and high gaussianStep. Clamp to at least 1x1 to avoid invalid framebuffer sizes.
- With many levels, verify textures/attachments are cloned (they are via slice in setViewportOptions) — OK.
I can send a small clamp patch if desired.
Also applies to: 299-311, 312-326, 336-347, 702-705
packages/effects-core/src/render/post-process-pass.ts (2)
147-149: GaussianDownSample dispose orderingUnsubscribe before calling super to avoid future coupling to base internals.
- override dispose (options?: RenderPassDestroyOptions | undefined): void { - super.dispose(options); - - this.renderer.engine.off('resize', this.onResize); - } + override dispose (options?: RenderPassDestroyOptions): void { + this.renderer.engine.off('resize', this.onResize); + super.dispose(options); + }Also applies to: 177-181
225-227: GaussianUpSample dispose orderingSame recommendation: off first, then super.
- override dispose (options?: RenderPassDestroyOptions): void { - super.dispose(options); - this.renderer.engine.off('resize', this.onResize); - } + override dispose (options?: RenderPassDestroyOptions): void { + this.renderer.engine.off('resize', this.onResize); + super.dispose(options); + }Also applies to: 253-256
web-packages/test/unit/src/effects-webgl/gl-dispose.spec.ts (1)
7-8: Engine-first test updates — LGTM; add listener cleanup checks
- Creating GLEngine and disposing via engine.dispose aligns with the new lifecycle. Assertions for attachments and framebuffer.renderer match current semantics. Good.
Suggestion:
- Add a test ensuring passes that subscribe to engine resize (e.g., DrawObjectPass, BloomThresholdPass) remove their listeners after dispose to prevent leaks.
I can draft the test if helpful.
Also applies to: 27-32, 46-47, 315-315, 362-362, 411-411, 460-460, 506-506, 552-552, 611-611
web-packages/test/unit/src/effects-webgl/gl-render-pass.spec.ts (1)
1106-1106: Clarify or remove the commented-out line.The commented
//engine.setSize(oldWidth,oldHeight);may indicate incomplete refactoring or may be intentionally omitted. If the default size (300x150) is sufficient for the test, remove the comment to avoid confusion. Otherwise, uncomment and explain why explicit initialization is needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/effects-core/src/engine.ts(7 hunks)packages/effects-core/src/render/draw-object-pass.ts(1 hunks)packages/effects-core/src/render/post-process-pass.ts(8 hunks)packages/effects-core/src/render/render-frame.ts(7 hunks)packages/effects-core/src/render/render-pass.ts(2 hunks)packages/effects-webgl/src/ext-wrap.ts(1 hunks)packages/effects-webgl/src/gl-renderer-internal.ts(0 hunks)web-packages/test/unit/src/effects-webgl/gl-dispose.spec.ts(10 hunks)web-packages/test/unit/src/effects-webgl/gl-render-pass.spec.ts(9 hunks)
💤 Files with no reviewable changes (1)
- packages/effects-webgl/src/gl-renderer-internal.ts
🧰 Additional context used
🧬 Code graph analysis (6)
web-packages/test/unit/src/effects-webgl/gl-render-pass.spec.ts (3)
web-packages/test/unit/src/effects-webgl/gl-utils.ts (1)
getGL2(13-23)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-710)packages/effects-core/src/render/draw-object-pass.ts (1)
DrawObjectPass(5-25)
web-packages/test/unit/src/effects-webgl/gl-dispose.spec.ts (2)
packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(17-710)packages/effects-webgl/src/gl-renderer.ts (1)
GLRenderer(21-342)
packages/effects-core/src/render/draw-object-pass.ts (2)
packages/effects-core/src/render/render-pass.ts (5)
RenderPass(287-702)RenderPassOptions(271-280)width(173-175)height(177-179)RenderPassDestroyOptions(216-221)packages/effects-core/src/render/renderer.ts (1)
Renderer(12-121)
packages/effects-core/src/render/render-frame.ts (2)
packages/effects-core/src/render/draw-object-pass.ts (1)
DrawObjectPass(5-25)packages/effects-core/src/render/post-process-pass.ts (2)
HQGaussianDownSamplePass(94-182)HQGaussianUpSamplePass(184-257)
packages/effects-core/src/render/post-process-pass.ts (2)
packages/effects-core/src/render/render-pass.ts (4)
width(173-175)height(177-179)RenderPassDestroyOptions(216-221)RenderPass(287-702)packages/effects-core/src/render/renderer.ts (1)
Renderer(12-121)
packages/effects-core/src/engine.ts (10)
packages/effects-core/src/gl/create-gl-context.ts (1)
GLType(1-1)packages/effects-core/src/events/event-emitter.ts (1)
EventEmitter(19-91)packages/effects-core/src/composition.ts (2)
Composition(132-1032)addItem(582-585)packages/effects-core/src/asset-manager.ts (1)
AssetManager(24-473)packages/effects-core/src/asset-service.ts (1)
AssetService(14-142)packages/effects-core/src/plugins/interact/event-system.ts (1)
EventSystem(32-221)packages/effects-core/src/ticker.ts (1)
Ticker(8-141)packages/effects-core/src/plugins/interact/click-handler.ts (1)
Region(58-67)packages/effects-core/src/utils/device.ts (1)
getPixelRatio(1-11)packages/effects-core/src/constants.ts (1)
HELP_LINK(10-17)
🪛 Biome (2.1.2)
packages/effects-core/src/engine.ts
[error] 272-273: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (14)
packages/effects-webgl/src/ext-wrap.ts (1)
174-183: No issues found.The code is semantically correct. The
renderer.clear()method accepts action objects that define both clearing operations and store actions. This pattern is used consistently throughout the codebase, including inpackages/effects-core/src/render/render-pass.tswhere the same sequence appears:renderer.clear(this.clearAction), render meshes, thenrenderer.clear(this.storeAction). The method handles both types of actions polymorphically—when the action specifiesTextureStoreAction.clear, it clears; otherwise, it stores or discards as specified.Likely an incorrect or invalid review comment.
packages/effects-core/src/render/render-pass.ts (1)
698-701: Do not null renderer in base dispose — fine, but document expectationLeaving this.renderer intact (only nulling options) lets subclasses safely unsubscribe after super.dispose. Please document that subclass disposes should remove listeners either before or after super without relying on base internals.
Confirm no external code depends on renderPass.renderer being nulled post-dispose.
packages/effects-core/src/render/draw-object-pass.ts (1)
9-11: Event listener lifecycleBinding on construction is fine; ensure there’s no duplicate registration on re-initialization flows. Current dispose unbind is present (after refactor above).
web-packages/test/unit/src/effects-webgl/gl-render-pass.spec.ts (4)
3-3: LGTM: Import updates align with engine-centric API.The addition of
GLEngineandDrawObjectPassimports correctly reflects the architectural shift where engine is the primary entry point and render passes wire to engine lifecycle events.Also applies to: 6-6
12-13: LGTM: Engine lifecycle correctly updated.The test properly instantiates
GLEngineas the primary object, accesses the renderer viaengine.renderer, and cleans up viaengine.dispose(). This aligns with the engine-centric lifecycle described in the relevant snippets.Also applies to: 17-17, 21-21
1108-1108: LGTM: DrawObjectPass correctly used for resize tests.
DrawObjectPasssubscribes to engineresizeevents and automatically updates its framebuffer dimensions. Using it in these tests ensures render passes respond correctly toengine.setSize()calls, which is the intended behavior being validated.Also applies to: 1278-1278
1151-1151: LGTM: Correct use of engine.setSize() API.The tests properly use
engine.setSize(width, height)to trigger resizing, which propagates throughrenderer.resize()and emits theresizeevent thatDrawObjectPasslistens to. This validates the complete resize chain from engine to framebuffer.Also applies to: 1154-1154, 1179-1179, 1190-1190, 1222-1222, 1225-1225, 1333-1333, 1336-1336, 1347-1347
packages/effects-core/src/engine.ts (7)
24-40: LGTM: Well-defined type interfaces.
EngineOptionsandEngineEventprovide clear type contracts for engine configuration and event emission, supporting the engine-centric API refactoring.
45-128: LGTM: Engine initialization comprehensive and well-ordered.The constructor properly initializes core services (ticker, eventSystem, assetLoader, assetService) with appropriate conditional logic for
manualRenderandinteractivemodes. The initialization order ensures dependencies are available when needed.
331-340: LGTM: setSize propagates changes correctly.The method properly updates renderer dimensions, adjusts composition camera properties, and emits the
resizeevent thatDrawObjectPassand other subsystems listen to.
393-461: LGTM: Proper disposal guards on resource management.All mutating methods correctly check
this.disposedto prevent operations on a disposed engine, ensuring safe lifecycle management.
463-475: LGTM: Composition lifecycle methods complete the engine-centric pattern.
addCompositionandremoveCompositionallow the engine to own and manage composition lifecycle, which aligns with the PR's goal of centralizing runtime around the Engine API.
484-527: LGTM: Comprehensive disposal implementation.The
disposemethod properly tears down all engine subsystems in the correct order (stop ticker/events first, then dispose resources), logs helpful warnings, and clears all resource arrays to prevent leaks.
9-9: LGTM: Import additions support new engine features.The added imports are all utilized by the engine-centric refactoring, including event emission, asset services, rendering loop, and resize/layout functionality.
Also applies to: 11-11, 14-22
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/effects-webgl/src/gl-engine.ts (1)
70-77: Add robustness to video restoration.The video restoration logic lacks defensive checks and error handling:
- Line 72: No bounds check—
newComposition.textures[i]may not exist if the composition structure changed- Line 72: No
sourceTypeverification before casting toTexture2DSourceOptionsVideo- Line 75:
video.play()can reject due to autoplay policies or invalid state, which would abort the entire restorationApply this diff to add guards and error handling:
for (let i = 0; i < videoState.length; i++) { - if (videoState[i]) { - const video = (newComposition.textures[i].source as Texture2DSourceOptionsVideo).video; - - video.currentTime = videoState[i] ?? 0; - await video.play(); + if (videoState[i] && i < newComposition.textures.length) { + const texture = newComposition.textures[i]; + const source = texture.source as Texture2DSourceOptionsVideo; + + if (source.video && source.video instanceof HTMLVideoElement) { + try { + source.video.currentTime = videoState[i] ?? 0; + await source.video.play(); + } catch (error) { + console.warn(`Failed to restore video at texture index ${i}:`, error); + } + } } }
🧹 Nitpick comments (1)
packages/effects-core/src/render/render-pass.ts (1)
699-699: Verify that not nullifying renderer doesn't cause memory leaks.The
rendererfield is no longer nullified during disposal. While this may be intentional if the renderer lifecycle is managed externally or if references are needed post-dispose, please ensure this change doesn't introduce memory leaks, particularly in scenarios where RenderPass instances are frequently created and destroyed.If the renderer should remain accessible after disposal for debugging or cleanup purposes, consider documenting this behavior. Otherwise, if it's an oversight, apply this diff:
// @ts-expect-error safe to assign -this.options = null; +this.options = this.renderer = null;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
packages/effects-core/src/render/render-pass.ts(3 hunks)packages/effects-webgl/src/ext-wrap.ts(3 hunks)packages/effects-webgl/src/gl-engine.ts(3 hunks)packages/effects-webgl/src/gl-framebuffer.ts(9 hunks)packages/effects-webgl/src/gl-renderbuffer.ts(3 hunks)packages/effects-webgl/src/gl-renderer-internal.ts(0 hunks)packages/effects-webgl/src/gl-renderer.ts(7 hunks)web-packages/test/unit/src/effects-webgl/gl-frame-buffer.spec.ts(2 hunks)web-packages/test/unit/src/effects-webgl/gl-geometry.spec.ts(2 hunks)web-packages/test/unit/src/effects-webgl/gl-material.spec.ts(16 hunks)web-packages/test/unit/src/effects-webgl/gl-program.spec.ts(12 hunks)web-packages/test/unit/src/effects-webgl/gl-render-frame.spec.ts(2 hunks)web-packages/test/unit/src/effects-webgl/gl-render-pass.spec.ts(30 hunks)web-packages/test/unit/src/effects-webgl/gl-shader-library.spec.ts(7 hunks)web-packages/test/unit/src/effects-webgl/gl-state.spec.ts(1 hunks)web-packages/test/unit/src/effects-webgl/gl-texture.spec.ts(3 hunks)web-packages/test/unit/src/effects-webgl/gl-vertex-array-object.spec.ts(7 hunks)web-packages/test/unit/src/effects-webgl/renderer.spec.ts(8 hunks)
💤 Files with no reviewable changes (1)
- packages/effects-webgl/src/gl-renderer-internal.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- web-packages/test/unit/src/effects-webgl/gl-frame-buffer.spec.ts
- web-packages/test/unit/src/effects-webgl/gl-texture.spec.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-18T09:15:08.038Z
Learnt from: wumaolinmaoan
PR: galacean/effects-runtime#691
File: packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts:39-42
Timestamp: 2024-10-18T09:15:08.038Z
Learning: In `packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts`, when accumulating color components, avoid using `clone()` methods because it can cause additional garbage collection overhead.
Applied to files:
packages/effects-webgl/src/gl-framebuffer.tsweb-packages/test/unit/src/effects-webgl/gl-shader-library.spec.tsweb-packages/test/unit/src/effects-webgl/gl-vertex-array-object.spec.tspackages/effects-webgl/src/gl-engine.tsweb-packages/test/unit/src/effects-webgl/gl-render-frame.spec.tsweb-packages/test/unit/src/effects-webgl/gl-material.spec.tspackages/effects-webgl/src/gl-renderer.tspackages/effects-webgl/src/ext-wrap.ts
🧬 Code graph analysis (12)
packages/effects-webgl/src/gl-framebuffer.ts (3)
packages/effects-webgl/src/gl-renderer.ts (3)
gl(39-41)width(47-49)height(43-45)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(16-705)packages/effects-webgl/src/gl-texture.ts (1)
GLTexture(31-495)
web-packages/test/unit/src/effects-webgl/gl-shader-library.spec.ts (2)
packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(16-705)packages/effects-webgl/src/gl-renderer.ts (2)
GLRenderer(29-506)canvas(51-53)
web-packages/test/unit/src/effects-webgl/renderer.spec.ts (2)
packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(16-705)packages/effects-webgl/src/gl-renderer.ts (1)
GLRenderer(29-506)
packages/effects-webgl/src/gl-renderbuffer.ts (2)
packages/effects-webgl/src/gl-renderer.ts (1)
gl(39-41)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(16-705)
web-packages/test/unit/src/effects-webgl/gl-vertex-array-object.spec.ts (2)
packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(16-705)packages/effects-webgl/src/gl-renderer.ts (3)
canvas(51-53)gl(39-41)GLRenderer(29-506)
packages/effects-webgl/src/gl-engine.ts (6)
packages/effects-webgl/src/gl-context-manager.ts (1)
GLContextManager(4-67)packages/effects-core/src/composition.ts (1)
Composition(132-1032)packages/effects-webgl/src/gl-renderer.ts (3)
canvas(51-53)gl(39-41)GLRenderer(29-506)packages/effects-core/src/scene-loader.ts (1)
SceneLoader(9-74)packages/effects-core/src/texture/types.ts (1)
Texture2DSourceOptionsVideo(144-149)packages/effects-core/src/utils/asserts.ts (1)
assertExist(1-5)
web-packages/test/unit/src/effects-webgl/gl-state.spec.ts (2)
packages/effects-webgl/src/gl-renderer.ts (3)
canvas(51-53)GLRenderer(29-506)gl(39-41)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(16-705)
web-packages/test/unit/src/effects-webgl/gl-render-frame.spec.ts (2)
packages/effects-webgl/src/gl-renderer.ts (3)
canvas(51-53)GLRenderer(29-506)gl(39-41)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(16-705)
web-packages/test/unit/src/effects-webgl/gl-render-pass.spec.ts (4)
packages/effects-webgl/src/gl-renderer.ts (1)
gl(39-41)web-packages/test/unit/src/effects-webgl/gl-utils.ts (1)
getGL2(13-23)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(16-705)packages/effects-core/src/render/draw-object-pass.ts (1)
DrawObjectPass(5-25)
web-packages/test/unit/src/effects-webgl/gl-material.spec.ts (2)
packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(16-705)packages/effects-webgl/src/gl-renderer.ts (3)
canvas(51-53)GLRenderer(29-506)gl(39-41)
packages/effects-webgl/src/gl-renderer.ts (12)
packages/effects-core/src/render/renderer.ts (1)
Renderer(12-121)packages/effects-webgl/src/ext-wrap.ts (1)
ExtWrap(20-161)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(16-705)packages/effects-webgl/src/gl-geometry.ts (1)
GLGeometry(29-593)packages/effects-webgl/src/gl-material.ts (1)
GLMaterial(24-755)packages/effects-webgl/src/gl-shader.ts (1)
GLShaderVariant(14-123)packages/effects-webgl/src/gl-texture.ts (1)
GLTexture(31-495)packages/effects-webgl/src/gl-framebuffer.ts (1)
GLFramebuffer(16-364)packages/effects-webgl/src/gl-renderbuffer.ts (1)
GLRenderbuffer(6-65)packages/effects-webgl/src/gl-renderer-internal.ts (1)
assignInspectorName(1-22)packages/effects-webgl/src/gl-vertex-array-object.ts (1)
GLVertexArrayObject(6-65)packages/effects-webgl/src/gl-gpu-buffer.ts (1)
GLGPUBuffer(39-152)
web-packages/test/unit/src/effects-webgl/gl-geometry.spec.ts (2)
packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(16-705)packages/effects-webgl/src/gl-renderer.ts (2)
canvas(51-53)gl(39-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (29)
packages/effects-core/src/render/render-pass.ts (1)
336-336: LGTM: Visibility change enables subclass extension.Changing
viewportScalefromprivatetoprotectedallows subclasses to access this field, which is appropriate if derived RenderPass implementations need to customize viewport scaling behavior.web-packages/test/unit/src/effects-webgl/gl-program.spec.ts (2)
1-751: Clarify the status of this commented-out test file.The entire test file is commented out, yet changes are being applied to align with the engine-centric refactor. If these tests are meant to be re-enabled as part of this PR, they should be uncommented and verified to work with the new API. If they're obsolete, consider removing them to avoid maintaining dead code.
Are these tests intended to be re-enabled with this refactor? If so, please uncomment and validate them. If not, consider removing the file to reduce maintenance burden.
48-48: Update commented test code to use idiomaticrenderer.glpattern for consistency.Both
renderer.engine.glandrenderer.glaccess the same GL context (the latter is a getter in GLRenderer that delegates to the former), but production code consistently usesrenderer.gl. Since the test file is currently entirely commented out, if it's reactivated, line 48 should be updated to match the codebase convention.// const gl = renderer.gl;packages/effects-webgl/src/ext-wrap.ts (2)
65-65: LGTM! API surface cleanup.The refactor correctly replaces internal
glRendereraccess with public renderer APIs (copy2andresetColorAttachments), aligning with the engine-centric architecture.Also applies to: 81-81
163-165: LGTM! Deprecation marker added.The
@deprecatedannotation appropriately signals thatCopyTexturePassis being phased out. The class is still used internally at line 144, which is expected during a refactor transition.web-packages/test/unit/src/effects-webgl/gl-vertex-array-object.spec.ts (1)
2-3: LGTM! Test migration to engine-centric architecture.The test correctly adopts the new GLEngine-based workflow:
- GLRenderer imported as type-only (line 2)
- GLEngine instantiated with canvas and glType (line 179)
- Renderer derived from
engine.renderer(line 181)- Disposal uses
engine.dispose()(line 30)This aligns with the PR's engine-centric refactor.
Also applies to: 28-31, 176-181
packages/effects-webgl/src/gl-framebuffer.ts (3)
114-114: LGTM! Framebuffer creation and renderbuffer management updated.The refactor correctly updates:
renderer.createGLFramebuffer(this.name)instead of passingthisto internal API (line 114)- GLRenderbuffer constructors now receive
rendererinstead of internal references (lines 131, 149, 166)This simplifies the API surface and aligns with engine-centric architecture.
Also applies to: 131-131, 149-149, 166-166
257-270: LGTM! White texture pattern for placeholder bindings.The introduction of
whiteTexturefromengine.whiteTexture(line 257) provides a standardized placeholder for texture bindings, replacing previous empty/placeholder texture approaches. This ensures consistent texture binding behavior across the engine.
209-209: LGTM! WebGL context access routed through renderer.All WebGL context accesses now use
this.renderer.glinstead ofthis.renderer.glRenderer.gl, and disposal usesthis.renderer.deleteGLFramebuffer(this). This centralizes GL access through the renderer interface.Also applies to: 235-235, 249-249, 309-309, 347-347
packages/effects-webgl/src/gl-renderbuffer.ts (2)
3-4: LGTM! Public API migration.The refactor correctly updates from internal
GLRendererInternalto publicGLRenderertype:
- Constructor and initialize method now accept
GLRenderer(lines 14, 23)- Renderer field typed as
GLRenderer(line 10)- GLEngine import added for type casting (line 4)
This aligns with the public API surface changes in the PR.
Also applies to: 10-10, 14-14, 23-23
47-47: LGTM! Engine cast for renderbuffer binding.The cast
(state as GLEngine)is necessary becauserenderer.engineis typed as the baseEngineclass, butbindRenderbufferis a GLEngine-specific method. This is consistent with the pattern used throughout the codebase for accessing GL-specific engine methods.packages/effects-webgl/src/gl-renderer.ts (7)
39-62: LGTM! Engine-centric getters.The new getters correctly delegate to the engine:
gl,context: access via(this.engine as GLEngine)canvas: derived fromthis.gl.canvaswidth,height: fromthis.gl.drawingBufferWidth/HeightisDisposed: exposes the privatedisposedflagThis provides a clean public API surface for accessing engine resources.
63-86: LGTM! Constructor refactored for engine-centric architecture.The constructor now accepts a single
Engineparameter instead of canvas/framework/options, correctly:
- Calls
super(engine)(line 64)- Assigns a unique name (line 66)
- Accesses GL context via
this.context(line 68)- Creates ExtWrap and default framebuffer using engine resources
This aligns with the PR's engine-centric refactor.
97-101: LGTM! Disposal guard added.The check
if (this.isDisposed)(line 97) prevents rendering after disposal, with an early return and console error. This improves robustness by guarding against use-after-dispose bugs.
201-243: LGTM! DrawGeometry implementation detailed.The refactored
drawGeometrymethod correctly:
- Guards against missing gl context (lines 203-207)
- Casts geometry/material to GL-specific types (lines 209-211)
- Retrieves shader program and VAO (lines 213-217)
- Handles subMeshes and indices (lines 224-242)
- Calls
gl.drawElementsorgl.drawArraysappropriately (lines 238-242)- Unbinds VAO after drawing (line 243)
The implementation is thorough and handles edge cases well.
359-365: LGTM! Disposal lifecycle improved.The updated
disposemethod:
- Guards against double-disposal with early return (lines 359-361)
- Calls
extension.dispose()anddeleteResource()(lines 362-363)- Sets
disposed = true(line 364)This ensures clean disposal and prevents double-free issues.
409-437: LGTM! WebGL2 copy2 implementation.The
copy2method correctly implements WebGL2 texture copying using framebuffer blitting:
- Guards against missing GL context (lines 412-414)
- Lazily creates source/target framebuffers (lines 416-421)
- Uses engine's
bindFramebufferfor state management (lines 424-429)- Chooses appropriate filter based on size equality (line 431)
- Calls
gl.blitFramebuffer(line 433)- Cleans up framebuffer bindings (lines 434-436)
The implementation is correct and efficient.
439-496: LGTM! GL resource management methods.The new resource management methods provide clean abstractions:
resetColorAttachments: binds and resets framebuffer attachments (lines 439-442)createGLRenderbuffer: creates WebGL renderbuffer (lines 444-448)createGLFramebuffer: creates and names framebuffer with error handling (lines 450-460)createVAO: wraps VAO creation (lines 462-467)deleteGLTexture,deleteGPUBuffer,deleteGLFramebuffer,deleteGLRenderbuffer: safe deletion with disposal guards (lines 469-496)These methods centralize GL resource lifecycle management.
web-packages/test/unit/src/effects-webgl/gl-geometry.spec.ts (1)
1-1: LGTM! Test migration to engine-centric pattern.The test correctly adopts the GLEngine-based workflow:
- Creates GLEngine with canvas and
{ glType: 'webgl2' }(line 31)- Derives renderer from
engine.renderer(lines 32-33)- Disposes via
engine.dispose()(line 39)This aligns with the engine-centric refactor across all test files.
Also applies to: 31-33, 39-39
web-packages/test/unit/src/effects-webgl/renderer.spec.ts (2)
3-4: LGTM! Test suite migrated to engine-centric pattern.All test cases correctly:
- Import GLRenderer as type-only (line 3)
- Import GLEngine as value (line 4)
- Create GLEngine instances (e.g., line 66)
- Derive renderer from
engine.renderer(e.g., line 67)- Dispose via
engine.dispose()(e.g., line 76)This ensures consistent test patterns across the suite.
Also applies to: 66-67, 76-76
182-182: LGTM! isDisposed getter usage.The test correctly accesses
renderer.isDisposed(line 182), which is now exposed as a public getter in GLRenderer (added at lines 55-57 in gl-renderer.ts). This allows tests to verify disposal state.web-packages/test/unit/src/effects-webgl/gl-state.spec.ts (1)
1-2: LGTM! Test migration to engine-centric pattern.The test correctly adopts the GLEngine-based workflow:
- GLRenderer imported as type-only (line 1)
- GLEngine imported as value (line 2)
- Creates GLEngine with
{ glType: 'webgl2' }(line 15)- Derives renderer from
engine.renderer(line 16)- Disposes via
engine.dispose()(line 20)This aligns with the engine-centric refactor.
Also applies to: 15-16, 20-20
web-packages/test/unit/src/effects-webgl/gl-render-frame.spec.ts (1)
1-96: LGTM! Clean migration to GLEngine-based workflow.The test file correctly adopts the engine-centric API:
- GLEngine is instantiated with canvas and options
- Renderer is derived from
engine.renderer- Disposal is performed via
engine.dispose()- GL context access via
renderer.glaligns with GLRenderer's getter implementationweb-packages/test/unit/src/effects-webgl/gl-shader-library.spec.ts (1)
1-220: LGTM! Consistent engine-centric test migration.The shader library tests correctly adopt the GLEngine API across all test cases:
- Dual engine setup (webgl/webgl2) properly instantiated
- ShaderLibrary accessed via
engine.shaderLibrary- All disposal flows route through
engine.dispose()- Cleanup properly nullifies both engines
web-packages/test/unit/src/effects-webgl/gl-material.spec.ts (1)
1-1712: LGTM! Material tests successfully migrated to engine-centric API.The test file correctly adopts the GLEngine workflow:
- GLEngine instantiation with proper options
- Renderer, GL context, and engine derived from GLEngine
- Disposal routed through
engine.dispose()- Engine casts to GLEngine are safe and consistent throughout
web-packages/test/unit/src/effects-webgl/gl-render-pass.spec.ts (1)
1-1350: LGTM! Render pass tests properly migrated to engine-centric API.The extensive test suite correctly adopts the GLEngine workflow:
- Engine instantiation and renderer derivation follow the new pattern
DrawObjectPassis appropriately used for resize-aware tests (subscribes to engine resize events)- Size management routed through
engine.setSize()- System framebuffer binding via
renderer.engine.bindSystemFramebuffer()- Disposal via
engine.dispose()packages/effects-webgl/src/gl-engine.ts (3)
31-110: LGTM! Well-structured engine initialization.The constructor follows a logical initialization sequence:
- Canvas-based engine setup
- Context manager creation with merged options
- Context loss/restore handler registration
- GL extraction and validation
- Capability, library, and renderer initialization
- Post-initialization resize
The initialization order ensures dependencies are satisfied before dependent components are created.
112-122: LGTM! Disposal order correctly addresses past review.The disposal sequence properly cleans up GPU consumers before disposing the GL context:
- Renderer disposal
- ShaderLibrary disposal
- Context disposal
- State reset
This ordering ensures GPU resources are released while the GL context is still valid.
58-82: Implicit composition management is working correctly; no action required.Verification confirms the composition lifecycle during restoration is properly managed:
- Old compositions are removed: The
lost()method callsdispose(), which invokesengine.removeComposition()(composition.ts:929, engine.ts:470)- New compositions are automatically added:
SceneLoader.load()returns a newCompositionwhose constructor callsengine.addComposition(this)(composition.ts:302)- The Promise.all result is intentionally unused: Compositions are tracked via the
addComposition()calls that fire in the Composition constructorsThe implicit behavior cited in the original review is verified and working as intended. No explicit documentation changes are necessary.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
packages/effects-webgl/src/gl-engine.ts (1)
70-77: Major: Add type guards and error handling for video restoration.The video restoration logic casts to
Texture2DSourceOptionsVideowithout verifying the texture'ssourceType, andvideo.play()can reject due to autoplay policies. If the texture is not a video or autoplay fails, the entire restoration flow aborts.Apply this diff to add type guards and error handling:
for (let i = 0; i < videoState.length; i++) { - if (videoState[i]) { - const video = (newComposition.textures[i].source as Texture2DSourceOptionsVideo).video; - - video.currentTime = videoState[i] ?? 0; - await video.play(); + if (videoState[i] && i < newComposition.textures.length) { + const texture = newComposition.textures[i]; + const source = texture.source as Texture2DSourceOptionsVideo; + + if (source.video && source.video instanceof HTMLVideoElement) { + try { + source.video.currentTime = videoState[i] ?? 0; + await source.video.play(); + } catch (error) { + console.warn(`Failed to restore video at texture index ${i}:`, error); + } + } } }packages/effects-core/src/render/post-process-pass.ts (1)
89-91: Critical: BloomThresholdPass still leaks resources—super.dispose not called.This was flagged in a previous review but remains unfixed. The dispose override only removes the resize listener; attachments, meshes, semantics, and framebuffer remain undisposed.
Apply this diff to fix the resource leak:
override dispose (options?: RenderPassDestroyOptions): void { this.renderer.engine.off('resize', this.onResize); + super.dispose(options); }
🧹 Nitpick comments (7)
packages/effects-webgl/src/gl-renderer.ts (7)
200-206: Add assertion or early return for GL context availability.The code checks if
glis null/undefined and logs a warning before returning, but this check happens after material setup. If the GL context is lost during material initialization, resources may be in an inconsistent state.Move the GL context check earlier:
override drawGeometry (geometry: Geometry, matrix: Matrix4, material: Material, subMeshIndex = 0): void { if (!geometry || !material) { return; } + const gl = (this.engine as GLEngine).gl; + + if (!gl) { + console.warn('GLGPURenderer has not bound a gl object, unable to render geometry.'); + return; + } material.initialize(); geometry.initialize(); geometry.flush(); const renderingData = this.renderingData; material.setMatrix('effects_ObjectToWorld', matrix); try { material.use(this, renderingData.currentFrame.globalUniforms); } catch (e) { console.error(e); return; } - const gl = (this.engine as GLEngine).gl; - - if (!gl) { - console.warn('GLGPURenderer has not bound a gl object, unable to render geometry.'); - - return; - }
426-428: Minor: Comparison operator inconsistency.Line 426 uses
===and==inconsistently when comparing dimensions. Use===for both comparisons to maintain consistency.- const filter = source.getWidth() === source.getHeight() && target.getWidth() == target.getHeight() ? gl.NEAREST : gl.LINEAR; + const filter = source.getWidth() === source.getHeight() && target.getWidth() === target.getHeight() ? gl.NEAREST : gl.LINEAR;
464-469: Guard deleteGLTexture against null GL context.The method checks
!this.disposedbut doesn't verify thatthis.glis available. If the GL context is lost before disposal,this.gl.deleteTexturecould throw.Add a null check for the GL context:
deleteGLTexture (texture: GLTexture) { - if (texture.textureBuffer && !this.disposed) { + if (texture.textureBuffer && !this.disposed && this.gl) { this.gl.deleteTexture(texture.textureBuffer); texture.textureBuffer = null; } }
471-477: Guard deleteGPUBuffer against null GL context.Similar to
deleteGLTexture, this method should verify the GL context is available before callingdeleteBuffer.deleteGPUBuffer (buffer: GLGPUBuffer | null) { - if (buffer && !this.disposed) { + if (buffer && !this.disposed && this.gl) { this.gl.deleteBuffer(buffer.glBuffer); // @ts-expect-error delete buffer.glBuffer; } }
479-484: Guard deleteGLFramebuffer against null GL context.The method lacks a GL context check before calling
deleteFramebuffer.deleteGLFramebuffer (framebuffer: GLFramebuffer) { - if (framebuffer && !this.disposed) { + if (framebuffer && !this.disposed && this.gl) { this.gl.deleteFramebuffer(framebuffer.fbo as WebGLFramebuffer); delete framebuffer.fbo; } }
486-491: Guard deleteGLRenderbuffer against null GL context.The method lacks a GL context check before calling
deleteRenderbuffer.deleteGLRenderbuffer (renderbuffer: GLRenderbuffer) { - if (renderbuffer && !this.disposed) { + if (renderbuffer && !this.disposed && this.gl) { this.gl.deleteRenderbuffer(renderbuffer.buffer); renderbuffer.buffer = null; } }
384-394: Simplify resize logic and fix condition.Line 388 has a logical error: the condition checks
gl.drawingBufferWidth !== widthbut it's outside theglnull check scope. The logic can be simplified.override resize (width: number, height: number): void { - if (this.width !== width || this.height !== height) { - const gl = this.gl; - - if (gl && gl.drawingBufferWidth !== width || gl.drawingBufferHeight !== height) { - gl.canvas.width = width; - gl.canvas.height = height; - gl.viewport(0, 0, width, height); - } + const gl = this.gl; + + if (gl && (gl.drawingBufferWidth !== width || gl.drawingBufferHeight !== height)) { + gl.canvas.width = width; + gl.canvas.height = height; + gl.viewport(0, 0, width, height); } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/effects-core/src/render/post-process-pass.ts(9 hunks)packages/effects-core/src/render/renderer.ts(3 hunks)packages/effects-webgl/src/gl-engine.ts(3 hunks)packages/effects-webgl/src/gl-renderer.ts(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-18T09:15:08.038Z
Learnt from: wumaolinmaoan
Repo: galacean/effects-runtime PR: 691
File: packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts:39-42
Timestamp: 2024-10-18T09:15:08.038Z
Learning: In `packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts`, when accumulating color components, avoid using `clone()` methods because it can cause additional garbage collection overhead.
Applied to files:
packages/effects-webgl/src/gl-engine.tspackages/effects-core/src/render/post-process-pass.ts
🧬 Code graph analysis (4)
packages/effects-webgl/src/gl-engine.ts (6)
packages/effects-webgl/src/gl-context-manager.ts (1)
GLContextManager(4-67)packages/effects-core/src/composition.ts (1)
Composition(132-1032)packages/effects-webgl/src/gl-renderer.ts (3)
canvas(50-52)gl(38-40)GLRenderer(29-501)packages/effects-core/src/scene-loader.ts (1)
SceneLoader(9-74)packages/effects-core/src/texture/types.ts (1)
Texture2DSourceOptionsVideo(144-149)packages/effects-core/src/utils/asserts.ts (1)
assertExist(1-5)
packages/effects-webgl/src/gl-renderer.ts (8)
packages/effects-core/src/render/renderer.ts (1)
Renderer(12-121)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(16-701)packages/effects-webgl/src/gl-framebuffer.ts (1)
GLFramebuffer(16-364)packages/effects-webgl/src/gl-texture.ts (1)
GLTexture(31-495)packages/effects-webgl/src/gl-renderbuffer.ts (1)
GLRenderbuffer(6-65)packages/effects-webgl/src/gl-renderer-internal.ts (1)
assignInspectorName(1-22)packages/effects-webgl/src/gl-vertex-array-object.ts (1)
GLVertexArrayObject(6-65)packages/effects-webgl/src/gl-gpu-buffer.ts (1)
GLGPUBuffer(39-152)
packages/effects-core/src/render/post-process-pass.ts (2)
packages/effects-core/src/render/renderer.ts (1)
Renderer(12-121)packages/effects-core/src/render/render-pass.ts (6)
width(173-175)height(177-179)RenderPassDestroyOptions(216-221)RenderPassOptions(271-280)RenderPass(287-702)RenderTargetHandle(104-180)
packages/effects-core/src/render/renderer.ts (3)
packages/effects-core/src/engine.ts (1)
Engine(45-528)packages/effects-core/src/render/render-frame.ts (1)
RenderingData(38-51)packages/effects-core/src/render/framebuffer.ts (1)
Framebuffer(30-86)
🔇 Additional comments (15)
packages/effects-core/src/render/renderer.ts (2)
13-13: LGTM: API simplification aligns with Engine-centric architecture.The signature change from
(canvas, framework, renderOptions?)to(engine: Engine)is a breaking change but aligns with the PR's objective to centralize around the Engine class.
118-120: Breaking change signature update verified—all callers already updated.The
dispose()signature change removing thehaltGLparameter has been completed successfully. Verification confirms that all call sites across the codebase already use the parameterless signature, including the production call inpackages/effects-webgl/src/gl-engine.ts:118and all test cases.packages/effects-webgl/src/gl-engine.ts (1)
112-121: Disposal order is now correct.The disposal sequence has been fixed from the previous review. GPU consumers (renderer and shader library) are now disposed before the GL context, which prevents potential errors when GPU resources try to access a destroyed context.
packages/effects-core/src/render/post-process-pass.ts (12)
81-87: LGTM: Resize logic correctly scales framebuffer with viewport.The resize handler properly accounts for viewportScale and uses optional chaining for safety.
58-59: LGTM: Resize event properly bound and registered.The resize handler is correctly bound and subscribed to the engine's resize event.
100-100: LGTM: Level parameter added for multi-level downsampling.The level field is properly declared as readonly and stored from the constructor parameter, enabling level-based sizing calculations.
Also applies to: 104-104, 109-109
147-148: LGTM: Resize event properly bound and registered.The resize handler is correctly bound and subscribed to the engine's resize event, consistent with other passes.
170-175: LGTM: Level-based resize logic correctly scales framebuffer.The resize calculation
Math.floor(width / Math.pow(2, this.level + 1))correctly implements multi-level downsampling.
177-181: LGTM: Proper disposal with super.dispose and event cleanup.This dispose implementation correctly calls
super.dispose(options)before removing the resize listener, ensuring all parent resources are cleaned up.
189-189: LGTM: Level parameter added for multi-level upsampling.The level field is properly declared as readonly and stored from the constructor parameter, enabling level-based sizing calculations.
Also applies to: 191-191, 194-194
225-226: LGTM: Resize event properly bound and registered.The resize handler is correctly bound and subscribed to the engine's resize event, consistent with other passes.
230-230: Verify getFramebuffer() never returns null.The code assumes
renderer.getFramebuffer()always returns a valid framebuffer. IfcurrentFramebuffercan be null/undefined at runtime, this will throw.This is the same concern as in HQGaussianDownSamplePass and BloomThresholdPass—verify once across all three passes.
246-251: LGTM: Level-based resize logic correctly scales framebuffer.The resize calculation
Math.floor(width / Math.pow(2, this.level - 1))correctly implements multi-level upsampling (note thelevel - 1vs.level + 1in downsampling).
253-256: LGTM: Proper disposal with super.dispose and event cleanup.This dispose implementation correctly calls
super.dispose(options)before removing the resize listener, ensuring all parent resources are cleaned up.
305-305: Verify getFramebuffer() never returns null.The code assumes
renderer.getFramebuffer()always returns a valid framebuffer. IfcurrentFramebuffercan be null/undefined at runtime, this will throw.This is the same concern as in the other passes—verify once across all four passes.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
web-packages/imgui-demo/src/ge.ts (1)
25-25: Clarify URL change relevance to engine refactoring.The switch from loading a local JSON file (
JSON.parse(JSON.stringify(animationScene))) to a hardcoded remote URL appears unrelated to the engine creation logic refactoring described in the PR objectives. This change reduces flexibility for local testing and development.Consider reverting this change or moving it to a separate PR if it's not essential to the engine refactoring:
- GalaceanEffects.playURL('https://mdn.alipayobjects.com/mars/afts/file/A*sifJTL9KPmwAAAAAQHAAAAgAelB4AQ'); + GalaceanEffects.playURL(JSON.parse(JSON.stringify(animationScene)));packages/effects-core/src/render/post-process-pass.ts (5)
88-90: Callsuper.dispose()to prevent resource leaks.The
dispose()method only removes the resize listener but does not callsuper.dispose(options), which means attachments, meshes, semantics, and the framebuffer are never cleaned up.Apply this diff:
override dispose (options?: RenderPassDestroyOptions): void { this.renderer.engine.off('resize', this.onResize); + super.dispose(options); }
63-63: Add null-safety check forrenderer.getFramebuffer().While
getFramebuffer()returnsFramebufferper its signature, the implementation storescurrentFramebuffer: Framebuffer | nulland other code paths (e.g.,FinalCopyRPin render-frame.ts) defensively check for null before accessing it. Directly chaining.getColorTextures()[0]will throw if the framebuffer is null.Apply this guard:
override configure (renderer: Renderer): void { - this.mainTexture = renderer.getFramebuffer().getColorTextures()[0]; + const framebuffer = renderer.getFramebuffer(); + if (!framebuffer) { + return; + } + this.mainTexture = framebuffer.getColorTextures()[0]; this.sceneTextureHandle.texture = this.mainTexture; renderer.setFramebuffer(this.framebuffer); }
151-151: Add null-safety checks forrenderer.getFramebuffer().Lines 151 and 165 both chain
.getColorTextures()[0]directly ongetFramebuffer()without checking for null. Despite the non-null return type signature, the underlying implementation allows null (seecurrentFramebuffer: Framebuffer | nullin renderer.ts), and defensive checks exist elsewhere in the codebase.Apply guards at both call sites:
override configure (renderer: Renderer): void { - this.mainTexture = renderer.getFramebuffer().getColorTextures()[0]; + const framebuffer = renderer.getFramebuffer(); + if (!framebuffer) { + return; + } + this.mainTexture = framebuffer.getColorTextures()[0]; renderer.setFramebuffer(this.framebuffer); }override execute (renderer: Renderer): void { renderer.clear({ colorAction: TextureStoreAction.clear, depthAction: TextureStoreAction.clear, stencilAction: TextureStoreAction.clear, }); this.screenMesh.material.setTexture('_MainTex', this.mainTexture); this.screenMesh.material.setVector2('_TextureSize', getTextureSize(this.mainTexture)); renderer.renderMeshes([this.screenMesh]); if (this.type === 'V') { - this.gaussianResult.texture = renderer.getFramebuffer().getColorTextures()[0]; + const framebuffer = renderer.getFramebuffer(); + if (framebuffer) { + this.gaussianResult.texture = framebuffer.getColorTextures()[0]; + } } }Also applies to: 165-165
229-229: Add null-safety check forrenderer.getFramebuffer().Line 229 chains
.getColorTextures()[0]ongetFramebuffer()without null-checking. This mirrors the same issue at lines 63, 151, and 165.Apply this guard:
override configure (renderer: Renderer): void { - this.mainTexture = renderer.getFramebuffer().getColorTextures()[0]; + const framebuffer = renderer.getFramebuffer(); + if (!framebuffer) { + return; + } + this.mainTexture = framebuffer.getColorTextures()[0]; renderer.setFramebuffer(this.framebuffer); }
304-304: Add null-safety check forrenderer.getFramebuffer().Line 304 accesses
.getColorTextures()[0]without checking whethergetFramebuffer()returns null.Apply this guard:
override configure (renderer: Renderer): void { - this.mainTexture = renderer.getFramebuffer().getColorTextures()[0]; + const framebuffer = renderer.getFramebuffer(); + if (!framebuffer) { + return; + } + this.mainTexture = framebuffer.getColorTextures()[0]; if (!this.sceneTextureHandle.texture) { this.sceneTextureHandle.texture = this.mainTexture; } renderer.setFramebuffer(null); }
🧹 Nitpick comments (1)
web-packages/imgui-demo/src/ge.ts (1)
11-11: Remove unused import.The
animationSceneimport is no longer used after the change at line 25 switched to a hardcoded URL.Apply this diff to remove the unused import:
-import * as animationScene from './ffd-demo.json';
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/effects-core/src/render/draw-object-pass.ts(1 hunks)packages/effects-core/src/render/framebuffer.ts(0 hunks)packages/effects-core/src/render/post-process-pass.ts(9 hunks)packages/effects-core/src/render/render-pass.ts(5 hunks)packages/effects-webgl/src/gl-framebuffer.ts(9 hunks)packages/effects-webgl/src/gl-renderer.ts(7 hunks)web-packages/imgui-demo/src/ge.ts(1 hunks)web-packages/test/unit/src/effects-webgl/gl-frame-buffer.spec.ts(2 hunks)web-packages/test/unit/src/effects-webgl/gl-render-pass.spec.ts(31 hunks)
💤 Files with no reviewable changes (1)
- packages/effects-core/src/render/framebuffer.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/effects-core/src/render/render-pass.ts
- packages/effects-webgl/src/gl-framebuffer.ts
- packages/effects-core/src/render/draw-object-pass.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-10-18T09:15:08.038Z
Learnt from: wumaolinmaoan
Repo: galacean/effects-runtime PR: 691
File: packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts:39-42
Timestamp: 2024-10-18T09:15:08.038Z
Learning: In `packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts`, when accumulating color components, avoid using `clone()` methods because it can cause additional garbage collection overhead.
Applied to files:
web-packages/imgui-demo/src/ge.tspackages/effects-core/src/render/post-process-pass.ts
📚 Learning: 2024-10-17T07:15:16.523Z
Learnt from: liuxi150
Repo: galacean/effects-runtime PR: 0
File: :0-0
Timestamp: 2024-10-17T07:15:16.523Z
Learning: Imports in `plugin-packages/model/src/utility/plugin-helper.ts` do not need to be merged as per project preference or coding standards.
Applied to files:
web-packages/imgui-demo/src/ge.ts
📚 Learning: 2025-08-05T07:51:18.728Z
Learnt from: ChengYi996
Repo: galacean/effects-runtime PR: 1118
File: packages/effects-core/src/components/ffd-component.ts:162-164
Timestamp: 2025-08-05T07:51:18.728Z
Learning: In packages/effects-core/src/components/ffd-component.ts, the //ts-expect-error usage for accessing siblingComponent.splits (around lines 162-164) is temporary debugging code that will be properly implemented later, as confirmed by ChengYi996.
Applied to files:
packages/effects-core/src/render/post-process-pass.ts
🧬 Code graph analysis (4)
packages/effects-webgl/src/gl-renderer.ts (9)
packages/effects-core/src/render/renderer.ts (1)
Renderer(12-121)packages/effects-webgl/src/ext-wrap.ts (1)
ExtWrap(20-161)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(16-701)packages/effects-webgl/src/gl-framebuffer.ts (1)
GLFramebuffer(16-362)packages/effects-webgl/src/gl-geometry.ts (1)
GLGeometry(29-593)packages/effects-webgl/src/gl-texture.ts (1)
GLTexture(31-495)packages/effects-webgl/src/gl-renderbuffer.ts (1)
GLRenderbuffer(6-65)packages/effects-webgl/src/gl-vertex-array-object.ts (1)
GLVertexArrayObject(6-65)packages/effects-webgl/src/gl-gpu-buffer.ts (1)
GLGPUBuffer(39-152)
web-packages/test/unit/src/effects-webgl/gl-render-pass.spec.ts (4)
packages/effects-webgl/src/gl-renderer.ts (1)
gl(38-40)web-packages/test/unit/src/effects-webgl/gl-utils.ts (1)
getGL2(13-23)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(16-701)packages/effects-core/src/render/draw-object-pass.ts (1)
DrawObjectPass(5-24)
packages/effects-core/src/render/post-process-pass.ts (2)
packages/effects-core/src/render/renderer.ts (1)
Renderer(12-121)packages/effects-core/src/render/render-pass.ts (5)
width(173-175)height(177-179)RenderPassDestroyOptions(216-221)RenderPassOptions(265-274)RenderPass(281-689)
web-packages/test/unit/src/effects-webgl/gl-frame-buffer.spec.ts (2)
packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(16-701)packages/effects-webgl/src/gl-renderer.ts (3)
canvas(50-52)GLRenderer(29-500)gl(38-40)
🔇 Additional comments (3)
web-packages/imgui-demo/src/ge.ts (1)
22-22: Verify silent failure behavior when ticker is unavailable.The optional chaining operator will silently skip registering the
updateRenderTexturecallback iftickeris unavailable. This could prevent the scene from rendering to the ImGui texture without any error indication.Please verify that this silent failure is the intended behavior for the engine refactoring. If ticker unavailability is a valid state, consider adding a fallback or logging to aid debugging.
packages/effects-core/src/render/post-process-pass.ts (2)
176-180: Good disposal order.Calling
super.dispose(options)before removing the event listener ensures proper cleanup of parent resources.
252-255: Good disposal order.Calling
super.dispose(options)before removing the event listener ensures proper cleanup of parent resources.
…/player-engine-creation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web-packages/imgui-demo/src/asset/inspire-list.ts (1)
1-622: Consider modularizing this large asset list.The asset list has grown to over 600 lines. For better maintainability, consider:
- Grouping related assets into separate modules (e.g.,
wufu-assets.ts,firework-assets.ts,popup-assets.ts)- Using a directory structure like
assets/wufu/,assets/fireworks/, etc.- Creating an index file that combines all asset modules
This would improve code organization and make it easier to locate and maintain specific asset groups.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web-packages/imgui-demo/src/asset/inspire-list.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
web-packages/imgui-demo/src/asset/inspire-list.ts (5)
2-113: LGTM - New asset entries added correctly.The new asset entries follow the established pattern and structure. All entries have valid URL and name properties.
381-385: Clarify the v2.4 mask incompatibility status.The comment indicates that the v2.4 mask refactoring is incompatible with the original mask that has no shape set. Please clarify:
- Is this a known limitation that is acceptable?
- Does the
countdownasset work correctly with the refactored engine despite this comment?- Should there be runtime handling or user warnings for this incompatibility?
422-426: Verify the JSON compatibility fix for redpackClick.Similar to the
bangerentries, this asset has been updated due to inconsistent behavior between old and new JSON. Ensure the new URL has been tested with the refactored player engine.
614-621: LGTM - New asset entries added.The
tryandredpack11entries are well-formed and follow the established pattern.
287-299: Clarify test asset inconsistency and confirm compatibility verification.The new URLs for
banger3,banger4, andbanger5are applied inimgui-demo/src/asset/inspire-list.tsmatchingtest/case/src/2d/assets/inspire.ts, buttest/memory/src/scene-list.tsstill uses the old URLs. This creates an inconsistency in the test suite. Additionally, no validation or test output confirms the new URLs resolve the compatibility issues mentioned in the comment.Please clarify:
- Whether the test/memory divergence is intentional or an oversight that needs updating
- How the new JSON compatibility has been verified with the refactored engine
packages/effects/src/types.ts
Outdated
| /** | ||
| * 每帧渲染调用后的回调，WebGL2 上下文生效 | ||
| * @param time - GPU 渲染使用的时间，秒 | ||
| * @deprecated 2.7.0 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
packages/effects-core/src/render/post-process-pass.ts (4)
62-62: Null-safety issue remains unaddressed.While the non-null assertion operator was removed, line 62 still accesses
renderer.getFramebuffer().getColorTextures()[0]without null-checking the framebuffer. As noted in previous reviews,getFramebuffer()can return null at runtime (evidenced by defensive checks in FinalCopyRP and nullable types in CopyTexturePass).Add a null guard before accessing:
override configure (renderer: Renderer): void { - this.mainTexture = renderer.getFramebuffer().getColorTextures()[0]; + const framebuffer = renderer.getFramebuffer(); + if (!framebuffer) { + return; + } + this.mainTexture = framebuffer.getColorTextures()[0]; this.sceneTextureHandle.texture = this.mainTexture; renderer.setFramebuffer(this.framebuffer); }
151-151: Null-safety issue remains unaddressed.Lines 151 and 165 access
renderer.getFramebuffer().getColorTextures()[0]without null-checking the framebuffer. As documented in previous reviews,getFramebuffer()can return null at runtime.Add null guards at both locations:
override configure (renderer: Renderer): void { - this.mainTexture = renderer.getFramebuffer().getColorTextures()[0]; + const framebuffer = renderer.getFramebuffer(); + if (!framebuffer) { + return; + } + this.mainTexture = framebuffer.getColorTextures()[0]; renderer.setFramebuffer(this.framebuffer); }override execute (renderer: Renderer): void { renderer.clear({ colorAction: TextureStoreAction.clear, depthAction: TextureStoreAction.clear, stencilAction: TextureStoreAction.clear, }); this.screenMesh.material.setTexture('_MainTex', this.mainTexture); this.screenMesh.material.setVector2('_TextureSize', getTextureSize(this.mainTexture)); renderer.renderMeshes([this.screenMesh]); if (this.type === 'V') { - this.gaussianResult.texture = renderer.getFramebuffer().getColorTextures()[0]; + const framebuffer = renderer.getFramebuffer(); + if (framebuffer) { + this.gaussianResult.texture = framebuffer.getColorTextures()[0]; + } } }Also applies to: 165-165
228-228: Null-safety issue remains unaddressed.Line 228 accesses
renderer.getFramebuffer().getColorTextures()[0]without null-checking the framebuffer. As documented in previous reviews,getFramebuffer()can return null at runtime.Add a null guard:
override configure (renderer: Renderer): void { - this.mainTexture = renderer.getFramebuffer().getColorTextures()[0]; + const framebuffer = renderer.getFramebuffer(); + if (!framebuffer) { + return; + } + this.mainTexture = framebuffer.getColorTextures()[0]; renderer.setFramebuffer(this.framebuffer); }
303-303: Null-safety issue remains unaddressed.Line 303 accesses
renderer.getFramebuffer().getColorTextures()[0]without null-checking the framebuffer. As documented in previous reviews,getFramebuffer()can return null at runtime.Add a null guard:
override configure (renderer: Renderer): void { - this.mainTexture = renderer.getFramebuffer().getColorTextures()[0]; + const framebuffer = renderer.getFramebuffer(); + if (!framebuffer) { + return; + } + this.mainTexture = framebuffer.getColorTextures()[0]; if (!this.sceneTextureHandle.texture) { this.sceneTextureHandle.texture = this.mainTexture; } renderer.setFramebuffer(null); }packages/effects/src/player.ts (2)
377-377: Invalid decorator syntax.The decorator
@handleThrowError<Player>uses generic type parameter syntax, which is not valid in TypeScript. This issue was previously flagged.Apply this diff to fix:
- @handleThrowError<Player> + @handleThrowError async loadScene (
630-630: Unsafe type assertion to GLEngine.The code assumes
this.engineis aGLEnginewithout checking. This issue was previously flagged and remains unresolved.Apply this diff to safely check for the context:
- if (this.canvas instanceof HTMLCanvasElement && (this.engine as GLEngine).context) { + if (this.canvas instanceof HTMLCanvasElement && 'context' in this.engine && this.engine.context) {packages/effects-webgl/src/gl-engine.ts (2)
70-77: Add type guards and error handling for video restoration.The video restoration logic has several robustness issues that could cause the entire restoration flow to fail:
- Line 72 casts to
Texture2DSourceOptionsVideowithout verifying the texture exists or has the correctsourceType- Line 75:
video.play()can reject due to autoplay policies or invalid video state, but errors are not caught- No bounds checking for the
texturesarrayApply this diff to add type guards and error handling:
for (let i = 0; i < videoState.length; i++) { - if (videoState[i]) { - const video = (newComposition.textures[i].source as Texture2DSourceOptionsVideo).video; - - video.currentTime = videoState[i] ?? 0; - await video.play(); + if (videoState[i] && i < newComposition.textures.length) { + const texture = newComposition.textures[i]; + const source = texture.source as Texture2DSourceOptionsVideo; + + if (source.video && source.video instanceof HTMLVideoElement) { + try { + source.video.currentTime = videoState[i] ?? 0; + await source.video.play(); + } catch (error) { + // Log but don't fail restoration if video can't play + console.warn(`Failed to restore video at texture index ${i}:`, error); + } + } } }
48-49: Disposed compositions are not removed from the engine's composition list in editor mode.When the WebGL context is lost in editor mode,
Composition.dispose()returns early (line 918) without callingremoveComposition(), leaving disposed compositions in theengine.compositionsarray. When the context restores,SceneLoader.load()adds new compositions alongside the old disposed ones, causing the array to accumulate stale entries.Fix by ensuring disposed compositions are always removed from the engine, even in editor mode. Consider moving the
removeComposition()call before the editor mode check at line 918 inpackages/effects-core/src/composition.ts, or handle cleanup of the old compositions after restoring.
🧹 Nitpick comments (1)
packages/effects/src/player.ts (1)
398-402: Consider guarding against concurrent loadScene calls.Disposing all asset managers synchronously could cause issues if
loadSceneis invoked concurrently or while compositions are still actively using assets. Consider adding a loading guard or documenting that concurrent calls are unsupported.Example guard:
+ private loading = false; + async loadScene ( scene: Scene.LoadType | Scene.LoadType[], options?: SceneLoadOptions, ): Promise<Composition | Composition[]> { + if (this.loading) { + throw new Error('Concurrent loadScene calls are not supported'); + } + this.loading = true; + try { // ... existing logic + } finally { + this.loading = false; + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/effects-core/src/plugins/interact/event-system.ts(3 hunks)packages/effects-core/src/render/draw-object-pass.ts(1 hunks)packages/effects-core/src/render/index.ts(1 hunks)packages/effects-core/src/render/post-process-pass.ts(9 hunks)packages/effects-core/src/scene-loader.ts(1 hunks)packages/effects-threejs/src/three-display-object.ts(4 hunks)packages/effects-webgl/src/gl-engine.ts(3 hunks)packages/effects/src/index.ts(2 hunks)packages/effects/src/player.ts(14 hunks)web-packages/test/unit/src/effects-webgl/gl-render-pass.spec.ts(31 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/effects-core/src/plugins/interact/event-system.ts
- packages/effects-core/src/scene-loader.ts
- packages/effects-core/src/render/draw-object-pass.ts
- packages/effects-threejs/src/three-display-object.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: yiiqii
Repo: galacean/effects-runtime PR: 214
File: packages/effects/src/player.ts:416-429
Timestamp: 2024-10-17T07:15:16.523Z
Learning: Data compatibility or processing logic related to replacing text variables in scene compositions should be handled in the AssetManager class through the updateOptions method, not in the Player class.
📚 Learning: 2024-10-18T09:15:08.038Z
Learnt from: wumaolinmaoan
Repo: galacean/effects-runtime PR: 691
File: packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts:39-42
Timestamp: 2024-10-18T09:15:08.038Z
Learning: In `packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts`, when accumulating color components, avoid using `clone()` methods because it can cause additional garbage collection overhead.
Applied to files:
packages/effects-webgl/src/gl-engine.tspackages/effects/src/player.tspackages/effects-core/src/render/post-process-pass.ts
📚 Learning: 2024-10-17T07:15:16.523Z
Learnt from: yiiqii
Repo: galacean/effects-runtime PR: 214
File: packages/effects/src/player.ts:416-429
Timestamp: 2024-10-17T07:15:16.523Z
Learning: Data compatibility or processing logic related to replacing text variables in scene compositions should be handled in the AssetManager class through the updateOptions method, not in the Player class.
Applied to files:
packages/effects/src/player.ts
📚 Learning: 2025-08-05T07:51:18.728Z
Learnt from: ChengYi996
Repo: galacean/effects-runtime PR: 1118
File: packages/effects-core/src/components/ffd-component.ts:162-164
Timestamp: 2025-08-05T07:51:18.728Z
Learning: In packages/effects-core/src/components/ffd-component.ts, the //ts-expect-error usage for accessing siblingComponent.splits (around lines 162-164) is temporary debugging code that will be properly implemented later, as confirmed by ChengYi996.
Applied to files:
packages/effects-core/src/render/post-process-pass.ts
🧬 Code graph analysis (5)
web-packages/test/unit/src/effects-webgl/gl-render-pass.spec.ts (4)
packages/effects-webgl/src/gl-renderer.ts (1)
gl(38-40)web-packages/test/unit/src/effects-webgl/gl-utils.ts (1)
getGL2(13-23)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(16-701)packages/effects-core/src/render/draw-object-pass.ts (1)
DrawObjectPass(5-24)
packages/effects-webgl/src/gl-engine.ts (6)
packages/effects-webgl/src/gl-context-manager.ts (1)
GLContextManager(4-67)packages/effects-core/src/composition.ts (1)
Composition(132-1032)packages/effects-webgl/src/gl-renderer.ts (2)
canvas(50-52)gl(38-40)packages/effects-core/src/scene-loader.ts (1)
SceneLoader(9-74)packages/effects-core/src/texture/types.ts (1)
Texture2DSourceOptionsVideo(144-149)packages/effects-core/src/utils/asserts.ts (1)
assertExist(1-5)
packages/effects/src/player.ts (3)
packages/effects-core/src/engine.ts (1)
Engine(45-528)packages/effects-core/src/scene-loader.ts (1)
SceneLoader(9-74)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(16-701)
packages/effects-core/src/render/post-process-pass.ts (2)
packages/effects-core/src/render/renderer.ts (1)
Renderer(12-121)packages/effects-core/src/render/render-pass.ts (6)
width(173-175)height(177-179)RenderPassDestroyOptions(216-221)RenderPassOptions(265-274)RenderPass(281-689)RenderTargetHandle(104-180)
packages/effects/src/index.ts (4)
packages/effects-core/src/render/renderer.ts (1)
Renderer(12-121)packages/effects-core/src/engine.ts (2)
Engine(45-528)EngineOptions(24-33)packages/effects-webgl/src/gl-renderer.ts (2)
GLRenderer(29-500)canvas(50-52)packages/effects-webgl/src/gl-engine.ts (1)
GLEngine(16-701)
🪛 Biome (2.1.2)
packages/effects/src/player.ts
[error] 404-404: Invalid decorator handleThrowError<Player>
(parse)
🔇 Additional comments (26)
packages/effects-core/src/render/index.ts (1)
12-12: LGTM!The export addition correctly exposes the new
DrawObjectPassclass and follows the existing pattern of barrel exports in this file. This aligns well with the PR's engine-centric refactoring objectives.packages/effects-core/src/render/post-process-pass.ts (9)
1-3: LGTM: Import additions support new functionality.The new imports for specification types, vector math, and render pass options are correctly added and used throughout the updated pass implementations.
Also applies to: 12-13
87-90: LGTM: Dispose now properly calls super.dispose().The dispose implementation correctly unsubscribes from the resize event and then delegates to
super.dispose(options), ensuring all resources (attachments, meshes, semantics) are properly cleaned up. This resolves the resource leak concern from the previous review.
57-59: LGTM: Resize handling correctly implemented.The resize event subscription and handler are properly implemented. The onResize method safely uses optional chaining when resizing the framebuffer, and dimensions are correctly sourced from the renderer.
Also applies to: 80-85
99-99: LGTM: Level parameter correctly added.The
levelparameter is properly added to the constructor signature, stored as a private readonly field, and used in the resize calculations for downsampling dimensions.Also applies to: 103-103, 108-108
176-179: LGTM: Dispose now properly calls super.dispose().The dispose implementation correctly unsubscribes from the resize event and delegates to
super.dispose(options), ensuring complete resource cleanup.
169-174: LGTM: Downsample resize logic is correct.The resize handler correctly calculates reduced dimensions using
Math.pow(2, this.level + 1)as the divisor, which is appropriate for multi-level Gaussian downsampling. The optional chaining onframebuffer?.resize()provides proper null-safety.
187-187: LGTM: Level parameter correctly added.The
levelparameter is properly added to the constructor signature, stored as a private readonly field, and used in the resize calculations for upsampling dimensions.Also applies to: 189-189, 192-192
251-254: LGTM: Dispose now properly calls super.dispose().The dispose implementation correctly unsubscribes from the resize event and delegates to
super.dispose(options), ensuring complete resource cleanup.
244-249: LGTM: Upsample resize logic is correct.The resize handler correctly calculates dimensions using
Math.pow(2, this.level - 1)as the divisor, which properly mirrors the downsampling level scheme (note thelevel - 1for upsampling vslevel + 1for downsampling). The optional chaining provides proper null-safety.packages/effects/src/player.ts (10)
1-13: LGTM!The updated imports correctly reflect the engine-centric refactor, bringing in
Engine,SceneLoader, andGLEnginetypes as needed.
38-113: LGTM!The introduction of the
enginefield and delegating getters cleanly centralizes rendering, ticker, and asset management responsibilities within the Engine, simplifying Player's surface.
168-180: LGTM!Engine creation and initialization correctly set up the rendering environment, pixel ratio, and offscreen mode.
183-217: LGTM!Engine event bindings (
rendererror,contextlost,contextrestored,onClick) are properly wired to delegate to Player's event handlers and lifecycle methods.
240-251: LGTM!Speed management correctly delegates to the engine.
404-429: LGTM!The refactor to use
SceneLoader.loadwith engine-based parameters correctly aligns with the new architecture, and composition setup (render order, message handlers, autoplay) is properly handled.
435-486: LGTM!The play/gotoAndPlay/gotoAndStop methods correctly handle offscreen mode, manage autoplay state, and ensure rendering occurs via
tick(0)when the ticker is unavailable (manual render mode).
553-591: LGTM!The
tick,resize, andclearCanvasmethods correctly delegate to the engine and renderer, maintaining proper lifecycle and rendering flow.
619-628: LGTM!The disposal flow correctly delegates to
engine.dispose()for cleanup of rendering resources, compositions, and asset managers.
634-638: LGTM!Canvas cleanup properly removes the element from the DOM to avoid rendering artifacts on certain platforms.
web-packages/test/unit/src/effects-webgl/gl-render-pass.spec.ts (3)
14-24: LGTM!The test correctly adopts the engine-centric approach: instantiating
GLEnginewith canvas and options, accessing the renderer viaengine.renderer, and disposing the engine (which handles internal cleanup).
1106-1106: LGTM!Using
DrawObjectPassinstead ofRenderPassin these resize tests is correct. Per the relevant code snippets,DrawObjectPasslistens to engine resize events and automatically resizes its framebuffer, which is exactly what these tests are validating.Also applies to: 1276-1276
1-1: Reconsider the necessity of removing@ts-nocheck—most test files don't use it.Only 2 out of 14 test files in the directory (gl-render-pass.spec.ts and gl-geometry.spec.ts) use
@ts-nocheck. The other 12 test files in effects-webgl successfully use proper type imports without it. Removing this directive would require understanding why these specific files need it when others don't.Before removing it:
- Verify if the missing
@ts-nocheckcauses build/test failures for gl-render-pass.spec.ts- Check if gl-geometry.spec.ts has similar underlying issues
- Ensure the test environment has proper type definitions for chai/mocha globals (describe, it, after, expect)
- Review whether these files have type incompatibilities that other test files have resolved
If the directive can be safely removed without breaking tests, align the approach with how the other 12 test files handle types.
packages/effects/src/index.ts (1)
76-82: LGTM!The factory method signatures correctly implement the engine-centric API.
Renderer.createnow takes anEngineparameter and returns aGLRenderer, whileEngine.createtakes a canvas and options to return aGLEngine. Both align with the refactored constructors.packages/effects-webgl/src/gl-engine.ts (2)
31-44: LGTM!The constructor correctly implements the engine-centric initialization flow: creating the
GLContextManager, initializing the GL context, GPU capability, shader library, and renderer, then performing an initial resize. The initialization sequence is sound.Also applies to: 98-110
112-122: LGTM!The dispose method now correctly disposes GPU consumers (renderer, shaderLibrary) before disposing the GL context. This fixes the issue flagged in previous reviews where the context was disposed before its consumers, which could break cleanup operations that require a live GL context.
2 participants
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores