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 注释
Merge origin/dev into origin/main
docs: changelog 2.4.7
WalkthroughThis update increments package versions to 2.4.7 and updates changelogs with two main fixes: resolving pre composition mask migration errors and preventing duplicate video downloads via the assetManager. Several core files are refactored to simplify asset and texture handling, adjust scene interface structure, and correct migration logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Player
participant AssetManager
participant AssetService
participant Scene
Player->>AssetManager: loadScene()
AssetManager->>Scene: Construct scene object (with textures, images, assets)
Player->>AssetService: prepareAssets(scene.assets)
AssetService->>Scene: Initialize textures and assets
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: 3
🔭 Outside diff range comments (3)
packages/effects-core/src/composition.ts (1)
386-389: 🛠️ Refactor suggestionExpose a defensive copy from the
texturesgetter
get textures()returns the private_texturesarray by reference, so external callers can mutate the internal collection and break invariants (e.g.undefinedholes leading to NPEs inupdateVideo,offloadTexture, etc.).Two quick-win fixes:
-get textures () { - return this._textures; -} +get textures () { + // return a read-only view + return [...this._textures]; +}or
get textures () { return Object.freeze(this._textures.slice()); }packages/effects/src/player.ts (1)
343-349: 💡 Verification agent🧩 Analysis chain
Double-check all call-sites after
prepareAssetssignature change
prepareAssets(scene, scene.assets)looks correct with the newScene.assetsfield.
Make sure no stale invocations still pass anAssetManager#getAssets()result; otherwise those sites will now silently miss assets.
🏁 Script executed:
#!/bin/bash # Find other call-sites that may still pass two arguments where the second # one is NOT 'scene.assets'. rg --line-number --context 2 $'prepareAssets\\([^,]+,' | grep -v 'scene\\.assets'Length of output: 1145
Update stale
prepareAssetsinvocation in three-display-object.tsA grep run shows one remaining call-site still passing
assetManager.getAssets()instead ofscene.assets:• packages/effects-threejs/src/three-display-object.ts:101
Suggested diff:
- this.assetService.prepareAssets(scene, assetManager.getAssets()); + this.assetService.prepareAssets(scene, scene.assets);Please make this change (and verify no other call-sites remain) to align with the updated
prepareAssets(scene, scene.assets)signature.packages/effects-core/src/asset-manager.ts (1)
158-170:⚠️ Potential issue
Sceneobject built without mandatory data in the JSON-object branchYou correctly add
textures,images, andassetsin the non-JSON branch, but whenrawJSONis already aScene-like object (Scene.isJSONObject(rawJSON)path at line 141) you still return:scene = { ...rawJSON };That object may not contain the newly-required properties
texturesandassets, making later calls (scene.assetsin player,assetService.initializeTexture(scene)) crash or misbehave.Minimal patch: extend both branches consistently.
- ...rawJSON, + ...rawJSON, + textures: rawJSON.textures ?? [], + assets: this.assets,(If
rawJSONalready has those keys they will be preserved.)Without this, the type system is satisfied (thanks to
anyin external JSON), but runtime fails.Please add unit tests that load a bare JSON scene to catch this regression.
🧹 Nitpick comments (3)
packages/effects-core/src/plugins/particle/particle-system.ts (2)
441-441: Explicit vec3 assertion is clear; hoist default vector.
Casting[0, 0, 0]asvec3improves type safety. Consider moving theoriginVecdeclaration above the outer burst loop to prevent recreating this constant on every iteration.
462-462: Simplify translate call with spread; validate fallback.
Usingp.transform.translate(...burstOffset)removes redundant type assertions. EnsureburstOffsetis always a three-element array to avoid runtime errors—consider an explicit check or default guard.packages/effects-core/src/composition.ts (1)
300-308: Avoid sharingscene.texturesby reference
this._texturesnow directly points to the same array held by theSceneinstance.
If downstream code mutatesthis._textures(splice,push,= [], …), the originalSceneobject – potentially shared with other consumers – will be affected, which is error-prone and hard to debug.Consider cloning or freezing the array at construction time:
-this._textures = scene.textures; +this._textures = [...scene.textures]; // shallow copy – keeps element identitiesAt the very least, add a short comment clarifying the intentional aliasing.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
CHANGELOG-zh_CN.md(1 hunks)CHANGELOG.md(1 hunks)packages/effects-core/package.json(1 hunks)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-helper/package.json(1 hunks)packages/effects-threejs/package.json(1 hunks)packages/effects-webgl/package.json(1 hunks)packages/effects/package.json(1 hunks)packages/effects/src/player.ts(1 hunks)plugin-packages/alipay-downgrade/package.json(1 hunks)plugin-packages/downgrade/package.json(1 hunks)plugin-packages/editor-gizmo/package.json(1 hunks)plugin-packages/model/package.json(1 hunks)plugin-packages/multimedia/package.json(1 hunks)plugin-packages/orientation-transformer/package.json(1 hunks)plugin-packages/rich-text/package.json(1 hunks)plugin-packages/spine/package.json(1 hunks)plugin-packages/stats/package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (17)
plugin-packages/rich-text/package.json (1)
3-3: Approve version bump to 2.4.7. This aligns with the overall 2.4.7 release and associated fixes.plugin-packages/model/package.json (1)
3-3: Confirm version bump consistency.
Theversionfield was updated to2.4.7. Please ensure this aligns with the CHANGELOG entries, release scripts, and all other related package.json files in this release train. Also verify that any lockfiles have been regenerated to reflect this change.packages/effects-webgl/package.json (1)
3-3: Version bump to 2.4.7 is correct
This aligns with the coordinated patch release across all Effects packages and no other metadata was altered.packages/effects-threejs/package.json (1)
3-3: Version bump to 2.4.7 is correct
Consistent with the overall release, and no unintended changes to scripts or dependencies.plugin-packages/multimedia/package.json (1)
3-3: Version bump to 2.4.7 is correct
Matches the coordinated update for the multimedia plugin; no additional edits detected.packages/effects-helper/package.json (1)
3-3: Version bump to 2.4.7 is correct
Aligned with the patch release and retains all prior configuration.plugin-packages/stats/package.json (1)
3-3: Version bump to 2.4.7 is correct
Consistent with the release and no other changes present.plugin-packages/orientation-transformer/package.json (1)
3-3: Approve version bump to 2.4.7
The version update aligns with the coordinated 2.4.7 release across plugins.plugin-packages/editor-gizmo/package.json (1)
3-3: Approve version bump to 2.4.7
Consistent with the overall release version for editor-gizmo.plugin-packages/downgrade/package.json (1)
3-3: Approve version bump to 2.4.7
Matches the release version used across downgrade plugins.plugin-packages/alipay-downgrade/package.json (1)
3-3: Approve version bump to 2.4.7
Version increment is in line with the global 2.4.7 release.plugin-packages/spine/package.json (1)
3-3: Approve version bump to 2.4.7
Spine plugin now matches the 2.4.7 release across the ecosystem.packages/effects-core/package.json (1)
3-3: Package version bumped to 2.4.7.
The version update aligns with the new patch release. Confirm that build artifacts, tags, and published packages reflect this version.packages/effects/package.json (1)
3-3: Package version updated to 2.4.7.
This bump matches the core package and CHANGELOG. Ensure inter-package dependencies and workspace references align with this version.CHANGELOG.md (1)
10-15: Patch release 2.4.7 entry is accurate.
The new header, date, and fix items correspond to PRs #1030 and #1029. Formatting and ordering are consistent with previous releases.packages/effects-core/src/plugins/text/text-item.ts (1)
225-228: Doc tweak LGTMThe JSDoc wording and
@defaulttag improve clarity without changing behaviour.CHANGELOG-zh_CN.md (1)
11-17: Changelog entry for 2.4.7 is well formatted and consistent.
The version header, release date, and fix descriptions align with existing entries and reference the correct PRs.
| textureOptions: Record<string, any>[], | ||
| textures: Texture[], | ||
| images: ImageLike[], | ||
| assets: Record<string, ImageLike>, | ||
| consumed?: boolean, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
textureOptions vs. textures – stale field?
With textures promoted to a mandatory array, the purpose of the still-present textureOptions: Record<string, any>[] is unclear and now overlaps semantically.
Keeping both fields invites confusion and accidental misuse.
If legacy code still needs textureOptions, mark it as @deprecated and plan removal; otherwise delete it altogether.
🤖 Prompt for AI Agents
In packages/effects-core/src/scene.ts around lines 20 to 24, the textureOptions
field overlaps with the mandatory textures array and causes confusion. Determine
if textureOptions is still used by legacy code; if yes, mark it as @deprecated
with a comment indicating planned removal. If not used, remove the
textureOptions field entirely to avoid redundancy and potential misuse.
| initializeTexture (scene: Scene) { | ||
| for (let i = 0; i < scene.textureOptions.length; i++) { | ||
| let textureOptions = scene.textureOptions[i]; | ||
|
|
||
| if (textureOptions instanceof Texture) { | ||
| this.engine.addInstance(textureOptions); | ||
| } else { | ||
| textureOptions = this.engine.findObject<Texture>({ id:scene.textureOptions[i].id }); | ||
| scene.textureOptions[i] = textureOptions; | ||
| } | ||
| textureOptions.initialize(); | ||
| scene.textures[i] = this.engine.findObject<Texture>({ id: scene.textureOptions[i].id }); | ||
| scene.textures[i].initialize(); | ||
| } |
There was a problem hiding this comment.
Null-safety missing when resolving textures
engine.findObject<Texture> can legitimately return undefined (e.g., corrupted JSON or missing preload).
Dereferencing the result immediately (scene.textures[i].initialize()) will throw and halt 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) {
+ // Keep the index in sync to avoid later mis-alignment.
+ logger.warn(`Texture ${scene.textureOptions[i].id} not found – skipped initialisation`);
+ continue;
+}
+scene.textures[i] = tex;
+tex.initialize();Also consider switching to a for…of loop to avoid repeated index look-ups.
📝 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.
| initializeTexture (scene: Scene) { | |
| for (let i = 0; i < scene.textureOptions.length; i++) { | |
| let textureOptions = scene.textureOptions[i]; | |
| if (textureOptions instanceof Texture) { | |
| this.engine.addInstance(textureOptions); | |
| } else { | |
| textureOptions = this.engine.findObject<Texture>({ id:scene.textureOptions[i].id }); | |
| scene.textureOptions[i] = textureOptions; | |
| } | |
| textureOptions.initialize(); | |
| scene.textures[i] = this.engine.findObject<Texture>({ id: scene.textureOptions[i].id }); | |
| scene.textures[i].initialize(); | |
| } | |
| initializeTexture (scene: Scene) { | |
| for (let i = 0; i < scene.textureOptions.length; i++) { | |
| const tex = this.engine.findObject<Texture>({ id: scene.textureOptions[i].id }); | |
| if (!tex) { | |
| // Keep the index in sync to avoid later mis-alignment. | |
| logger.warn(`Texture ${scene.textureOptions[i].id} not found – skipped initialisation`); | |
| continue; | |
| } | |
| scene.textures[i] = tex; | |
| tex.initialize(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/effects-core/src/asset-service.ts around lines 86 to 90, the code
calls initialize() on the result of engine.findObject<Texture> without checking
if it is undefined, which can cause runtime errors. To fix this, add a null
check after calling findObject to ensure the texture exists before calling
initialize. Also, refactor the loop to use a for…of loop over
scene.textureOptions to avoid repeated index look-ups and improve readability.
| for (const comp of compositions) { | ||
| processContent(comp); | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Mask migration may bleed across compositions
processContent relies on the module-level variable currentMaskComponent.
Iterating over all compositions in a flat loop without resetting that variable means a MASK defined in composition A could erroneously influence OBSCURED items in composition B.
+ for (const comp of compositions) {
+ currentMaskComponent = undefined as unknown as string; // reset per comp
processContent(comp);
}Alternatively, move currentMaskComponent inside processContent.
Please verify with a multi-composition test scene containing independent mask groups.
🏁 Script executed:
#!/bin/bash
# Find declaration and usage of currentMaskComponent
rg -n "currentMaskComponent" packages/effects-core/src/fallback/migration.ts
# Locate processContent definition and calls
rg -n "function processContent" packages/effects-core/src/fallback/migration.ts
rg -n "processContent(" packages/effects-core/src/fallback/migration.tsLength of output: 512
🏁 Script executed:
#!/bin/bash
# Display context around currentMaskComponent declaration
sed -n '1,80p' packages/effects-core/src/fallback/migration.ts
# Display processContent implementation
sed -n '180,300p' packages/effects-core/src/fallback/migration.ts
# Locate the loop invoking processContent
rg -nF "for (const comp" -n packages/effects-core/src/fallback/migration.tsLength of output: 6297
Reset mask state per composition
The module‐level currentMaskComponent persists across calls to processContent, so a MASK in one composition can leak into OBSCURED items of the next.
Locations to update:
packages/effects-core/src/fallback/migration.ts(around lines 119–122)for (const comp of compositions) { + currentMaskComponent = undefined as unknown as string; // reset mask state per comp processContent(comp); }
Alternatively, declare currentMaskComponent inside processContent to scope it per composition.
📝 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.
| for (const comp of compositions) { | |
| processContent(comp); | |
| } | |
| for (const comp of compositions) { | |
| currentMaskComponent = undefined as unknown as string; // reset mask state per comp | |
| processContent(comp); | |
| } |
🤖 Prompt for AI Agents
In packages/effects-core/src/fallback/migration.ts around lines 119 to 122, the
module-level variable currentMaskComponent is not reset between iterations over
compositions, causing mask state to bleed across compositions. To fix this,
either reset currentMaskComponent to null or its initial state at the start of
each loop iteration before calling processContent, or move the declaration of
currentMaskComponent inside the processContent function so it is scoped per
composition and does not persist across calls.
2 participants
Summary by CodeRabbit
Bug Fixes
Documentation
Chores