Conversation
Merge origin/main into origin/dev
#1029) * fix: video will be downloaded twice when loaded using the assetmanager * chore: modify type --------- Co-authored-by: yiiqii <yfj5tzl2005@sina.com>
fix: pre composition mask migration error
docs: 修改 setFontStyle 注释
WalkthroughThis change refactors asset and texture handling across several modules, making the Changes
Sequence Diagram(s)sequenceDiagram
participant Player
participant AssetManager
participant AssetService
participant Scene
Player->>AssetManager: loadScene(json)
AssetManager->>Scene: Assign textures, assets directly
Player->>AssetService: prepareAssets(scene.assets)
AssetService->>Scene: Initialize textures from scene.textures
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
🧹 Nitpick comments (1)
packages/effects-core/src/plugins/particle/particle-system.ts (1)
441-444: Use type annotation instead of assertion fororiginVecand hoist constant to avoid per-frame allocationCreating a new array each time the burst loop runs adds unnecessary GC pressure.
Also, a type annotation is cleaner than anasassertion.- const originVec = [0, 0, 0] as vec3; + // constant defined once at module scope avoids re-allocating +const ORIGIN_VEC: vec3 = [0, 0, 0]; +… + const burstOffset = (offsets && offsets[opts.cycleIndex]) || ORIGIN_VEC;If you decide to keep it local, at least switch to
const originVec: vec3 = [0, 0, 0];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/effects-core/src/asset-manager.ts(1 hunks)packages/effects-core/src/asset-service.ts(2 hunks)packages/effects-core/src/composition.ts(1 hunks)packages/effects-core/src/fallback/migration.ts(1 hunks)packages/effects-core/src/plugins/particle/particle-system.ts(2 hunks)packages/effects-core/src/plugins/text/text-item.ts(1 hunks)packages/effects-core/src/scene.ts(1 hunks)packages/effects/src/player.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
packages/effects-core/src/plugins/particle/particle-system.ts (1)
462-463: 👍 Removal of redundantas vec3castThe spread call already enforces
numberarguments, so the explicit cast was noise.
No further action required.packages/effects/src/player.ts (1)
346-351:Details
❓ Verification inconclusive
Ensure
scene.assetsis populated before callingprepareAssets
AssetManager.getAssets()previously guaranteed the map was filled.
Now we rely onscene.assets, which is assigned insideAssetManager.loadScene.
If any plugin mutates or clears this property after load, asset preparation will silently skip required assets.Please verify at least one key exists for typical scenes:
If empties are possible, consider falling back to
assetManager.getAssets()or throwing early.
🏁 Script executed:
#!/bin/bash # Count scenes whose assets map is empty after loadScene rg --no-heading -t ts -N $'\\.assets\\s*=\\s*\\{\\}' | head -n 20Length of output: 266
🏁 Script executed:
#!/bin/bash # Show loadScene implementation context rg -n -A50 "loadScene" packages/effects-core/src/asset-manager.tsLength of output: 1940
🏁 Script executed:
#!/bin/bash # Find all occurrences of `.assets` in asset-manager.ts rg -n "\.assets" packages/effects-core/src/asset-manager.tsLength of output: 485
🏁 Script executed:
#!/bin/bash # Locate getAssets implementation in AssetManager rg -n -A5 "getAssets" packages/effects-core/src/asset-manager.tsLength of output: 227
Verify non-empty
scene.assetsafterloadScene
File: packages/effects/src/player.ts (lines 346–351)We used to rely on
assetManager.getAssets()to always return a populated map. With the new API,scene.assetscomes from insideloadSceneand could be empty if no assets were found or if later code tampered with that object.– After calling
loadScene, add a quick check:const scene = await assetManager.loadScene(source, this.renderer, { env: this.env }); // fallback if no assets were loaded if (Object.keys(scene.assets).length === 0) { // either reuse the manager’s copy… scene.assets = assetManager.getAssets(); // …or throw early: // throw new Error('Loaded scene contains no assets.'); } this.assetService.prepareAssets(scene, scene.assets); this.assetService.updateTextVariables(scene, assetManager.options.variables); this.assetService.initializeTexture(scene);Please confirm that your typical scenes always populate
scene.assets. If not, implement this fallback or fail fast.packages/effects-core/src/scene.ts (1)
21-24: Requiredtextures/ newassetsfield LGTMMaking
texturesmandatory simplifies downstream code, and adding anassets
dictionary aligns with the new load flow.
No issues spotted.packages/effects-core/src/plugins/text/text-item.ts (1)
225-228: Docstring improvement acknowledgedThe JSDoc now correctly describes “font style” with a default of
"normal".
No code impact.
|
|
||
| this.renderer = renderer; | ||
| this._textures = scene.textureOptions as Texture[]; | ||
| this._textures = scene.textures; | ||
| this.postProcessingEnabled = scene.jsonScene.renderSettings?.postProcessingEnabled ?? false; | ||
| this.getEngine().renderLevel = scene.renderLevel; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sharing scene.textures between compositions can lead to double-dispose or premature unload
this._textures = scene.textures; means all compositions built from the same
Scene instance hold the same array reference.
dispose() iterates and tex.dispose() each entry.
If two compositions share the array, the first dispose call frees the GPU
resource and the second dispose call will hit an already-destroyed texture.
Clone the array or track ownership:
- this._textures = scene.textures;
+ // clone to avoid double-dispose when multiple compositions share a scene
+ this._textures = scene.textures.slice();Or introduce a reference-count in Texture/Scene.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.renderer = renderer; | |
| this._textures = scene.textureOptions as Texture[]; | |
| this._textures = scene.textures; | |
| this.postProcessingEnabled = scene.jsonScene.renderSettings?.postProcessingEnabled ?? false; | |
| this.getEngine().renderLevel = scene.renderLevel; | |
| this.renderer = renderer; | |
| // clone to avoid double-dispose when multiple compositions share a scene | |
| this._textures = scene.textures.slice(); | |
| this.postProcessingEnabled = scene.jsonScene.renderSettings?.postProcessingEnabled ?? false; | |
| this.getEngine().renderLevel = scene.renderLevel; |
🤖 Prompt for AI Agents
In packages/effects-core/src/composition.ts around lines 304 to 308, the
assignment this._textures = scene.textures shares the same texture array
reference across compositions, causing double-dispose issues. To fix this, clone
the textures array when assigning it to this._textures so each composition has
its own copy, preventing multiple disposes of the same texture. Alternatively,
implement reference counting in Texture or Scene to manage shared ownership
safely.
| textures: [], | ||
| images: loadedImages, | ||
| assets: this.assets, |
There was a problem hiding this comment.
Avoid shared-reference leaks between AssetManager and Scene.
scene.assets = this.assets hands out the same object.
When AssetManager.dispose() executes, it only reassigns its own reference (this.assets = {}), leaving scene.assets pointing to the old map – GPU / memory never freed.
Either:
- Give the scene a shallow copy so the manager can safely clear its own map, or
- Make
dispose()iterate the map and release resources, then also clearscene.assets.
- assets: this.assets,
+ assets: { ...this.assets }, // decouple the two owners📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| textures: [], | |
| images: loadedImages, | |
| assets: this.assets, | |
| textures: [], | |
| images: loadedImages, | |
| assets: { ...this.assets }, // decouple the two owners |
🤖 Prompt for AI Agents
In packages/effects-core/src/asset-manager.ts around lines 167 to 169, the
assignment scene.assets = this.assets shares the same object reference, causing
dispose() to clear only the manager's assets while the scene retains the old
reference, leading to resource leaks. Fix this by assigning a shallow copy of
this.assets to scene.assets to avoid shared references, or modify dispose() to
explicitly release resources in the map and clear scene.assets as well.
| scene.textures[i] = this.engine.findObject<Texture>({ id: scene.textureOptions[i].id }); | ||
| scene.textures[i].initialize(); | ||
| } |
There was a problem hiding this comment.
Null-check findObject() result to prevent runtime crash.
this.engine.findObject can return undefined; calling .initialize() unguarded will throw and abort scene loading.
- scene.textures[i] = this.engine.findObject<Texture>({ id: scene.textureOptions[i].id });
- scene.textures[i].initialize();
+ const tex = this.engine.findObject<Texture>({ id: scene.textureOptions[i].id });
+ if (!tex) {
+ console.warn(`Texture ${scene.textureOptions[i].id} not found`);
+ continue; // or throw, depending on expectations
+ }
+ tex.initialize();
+ scene.textures[i] = tex;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| scene.textures[i] = this.engine.findObject<Texture>({ id: scene.textureOptions[i].id }); | |
| scene.textures[i].initialize(); | |
| } | |
| const tex = this.engine.findObject<Texture>({ id: scene.textureOptions[i].id }); | |
| if (!tex) { | |
| console.warn(`Texture ${scene.textureOptions[i].id} not found`); | |
| continue; // or throw, depending on expectations | |
| } | |
| tex.initialize(); | |
| scene.textures[i] = tex; | |
| } |
🤖 Prompt for AI Agents
In packages/effects-core/src/asset-service.ts around lines 88 to 90, the result
of this.engine.findObject may be undefined, causing a runtime crash when calling
initialize() on it. Add a null-check to ensure the object exists before calling
initialize(), and handle the case where findObject returns undefined to prevent
aborting scene loading.
| for (const comp of compositions) { | ||
| processContent(comp); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reset mask state per composition to avoid cross-composition bleed-through.
currentMaskComponent is global. Iterating every composition consecutively means a mask set in one composition may wrongly influence the next.
+ for (const comp of compositions) {
+ currentMaskComponent = undefined; // clear previous composition context
processContent(comp);
}Without this, obscured items in later compositions may reference a mask from an unrelated composition.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/effects-core/src/fallback/migration.ts around lines 119 to 121, the
variable currentMaskComponent is global and not reset between processing each
composition, causing mask state to bleed across compositions. To fix this, reset
or reinitialize currentMaskComponent at the start of each iteration inside the
for loop so that each composition processes with a fresh mask state and does not
inherit masks from previous compositions.
2 participants
Summary by CodeRabbit
Refactor
Documentation
Style