feat(spine): build Spine in as a 2D skeletal animation solution (4.2 backend)#3057
feat(spine): build Spine in as a 2D skeletal animation solution (4.2 backend)#3057cptbtptpbcptdtptp wants to merge 8 commits into
Conversation
Move engine-spine into the engine monorepo following the physics provider pattern: a version-agnostic facade plus a pluggable version backend. Public API matches the engine-spine 4.2 branch. - @galacean/engine-spine: user-facing SpineAnimationRenderer + loaders, owns the Galacean-side buffers/material, with zero spine-core runtime dependency. ISpineRuntime and ISpineRenderTarget are the seam; the backend self-registers on import, mirroring IPhysics with physics-physx / physics-lite. - @galacean/engine-spine-core-4.2: bundles @esotericsoftware/spine-core 4.2, the mesh generator and Spine42Runtime. - 2D/Spine ShaderLab shader, precompiled and registered in ShaderPool. - e2e: spineboy case. The 3.8 backend (@galacean/engine-spine-core-3.8) follows in a separate PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
e2e: spineboy basic render, plus a tint-black two-color case using the real tank-pro asset sampled at the muzzle-smoke frame where tintBlack on/off differs ~7% (registered in config.ts with 0.05 tolerance). unit (tests/src/spine): pool reuse, runtime registry, vertex stride and blend ordinal contracts, loader url parsing, SpineMaterial blend factors, SpineAnimationRenderer tintBlack setter and material cache. fix: SpineAnimationRenderer._getMaterial read its cache via Map[key] (always undefined) instead of .get(key), so every sub-mesh allocated a fresh SpineMaterial every frame. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
# Conflicts: # packages/shader/compiledShaders/index.ts
The packages/spine-core-3.8 directory was never committed, but a leftover lockfile importer entry for it remained, which pnpm install prunes.
The merge's pre-commit hook auto-fixed Object -> object in this file per the newly-merged ESLint config, which made the implementation signature's AssetPromise.all(promises) return type no longer assignable (T is unconstrained here since it must satisfy both the single-item and collection overloads).
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR adds Spine 2D skeletal animation support to the engine: a runtime contract (ISpineRuntime/ISpineRenderTarget), a spine-core-4.2 backend package, a spine loader/renderer/material package, shader registration, E2E test cases and assets, plus minor unrelated Object-type cleanups and a monorepo-wide experimental version bump. ChangesSpine Animation Feature
Core Type Cleanups and Build Config
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant SpineAnimationRenderer
participant Spine42Runtime
participant SpineGenerator
participant Skeleton
SpineAnimationRenderer->>Spine42Runtime: updateState(skeleton, state, delta)
Spine42Runtime->>Skeleton: update, apply, Physics.update
SpineAnimationRenderer->>Spine42Runtime: buildPrimitive(skeleton, target)
Spine42Runtime->>SpineGenerator: buildPrimitive(skeleton, target)
SpineGenerator->>SpineGenerator: compute vertices/indices, group sub-primitives
SpineGenerator-->>SpineAnimationRenderer: publish sub-primitives and materials
sequenceDiagram
participant SpineLoader
participant ResourceManager
participant SpineAtlasLoader
participant LoaderUtils
participant SpineResource
SpineLoader->>ResourceManager: request skeleton data
SpineLoader->>ResourceManager: load atlas (single/multiple URLs)
ResourceManager->>SpineAtlasLoader: load(item)
SpineAtlasLoader->>LoaderUtils: loadTexturesByPaths / createTextureAtlas
LoaderUtils-->>SpineAtlasLoader: TextureAtlas
SpineAtlasLoader-->>SpineLoader: TextureAtlas
SpineLoader->>LoaderUtils: createSkeletonData(rawData, atlas, scale)
LoaderUtils-->>SpineLoader: SkeletonData
SpineLoader->>SpineResource: new SpineResource(engine, skeletonData, url)
SpineResource-->>SpineLoader: instantiate() entity template
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
把 Spine 从独立仓库收进 engine,按物理包的 provider 模式拆成版本无关门面 @galacean/engine-spine + 可替换版本内核 engine-spine-core-4.2。方向非常正确:接缝切在 ISpineRuntime(解析/步进/生成)+ ISpineRenderTarget(缓冲/材质/包围盒的窄契约),门面运行时零 spine-core 引用、内核经副作用自注册,与 IPhysics 抽象同构——比 Cocos 的"一工程锁死单版本"更灵活,又不像 Three.js/Babylon 那样把 Spine 完全甩给 userland。RFC(#3050)写得很清楚,_render 忠实对标 MeshRenderer 2.0 模型、update() 走 _overrideUpdate 自动探测正确挂帧、tint-black shader 是全工程唯一非直译处且有专门 e2e(采样 darkColor 可见帧、~7% 差异)守住。绝大多数文件是 engine-spine 4.2 的忠实移植,SpineConstant.test.ts 显式锁 SpineVertexStride/SpineBlendMode 数值防跨包漂移是亮点。build 三平台 + e2e 4/4 全绿。
先澄清一处我一度怀疑的 P0(已排除):ShaderPool.registerShaders() 调 Shader._createFromPrecompiled(SpineSource),而 tip 上既无 compiledShaders/2D/Spine.shaderc 也无 compiledShaders/index.ts 的 Spine 导出——但 packages/shader/.gitignore 含 compiledShaders/,该目录是 precompile 脚本从 src/Shaders(已含 Spine 源注册)构建期生成的产物,非提交物。build+e2e 绿即证明生成正确、2D/Spine 运行时可 Shader.find。无 P0。
整体建议合入,以下均为 P2/P3,不阻塞。
问题
-
[P2] SpineAnimationRenderer.ts — 跨包硬编码 engine 内部 enum 值
文件末尾重声明enum RendererUpdateFlags { WorldVolume = 0x1 },update()里this._dirtyUpdateFlag |= RendererUpdateFlags.WorldVolume依赖这个0x1与 engineRenderer.ts:507的RendererUpdateFlags.WorldVolume = 0x1数值一致。卫星包看不到@internalenum 只能重定义我理解,但这是无编译期护栏的跨包耦合:engine 一旦给RendererUpdateFlags重新编号,spine 会静默用错 bit(可能标错脏标记 → 包围盒/剔除异常),而不会有任何报错或测试失败。SpineConstant.test.ts已经用"锁数值"手法守住了SpineBlendMode/SpineVertexStride,建议同样加一条断言把 spine 侧的RendererUpdateFlags.WorldVolume与 engine 导出值对齐(engine 侧RendererUpdateFlags是 export 的,可直接 import 比对),让漂移在 CI 红而非线上白。 -
[P3] SpineGenerator.ts:122 / SpineTexture.ts:23 — 移植带进两处上游既有小瑕疵
①if (numFloats > _vertices.length) { tempVerts = new Array(numFloats) }:预扩的是 scratchtempVerts,判据却拿目标 GPU 缓冲_vertices.length比,应比tempVerts.length。实际无害(JS Array 自动增长,且首帧_vertices长度 0 会强制扩一次自愈),但判据对象是错的。②SpineTexture.setFilters用magFilter === MipMapLinearLinear判 trilinear——mipmap 是 minification 概念,放大过滤不会用 mipmap,常规 atlas 路径(minFilter=MipMapLinearLinear/magFilter=Linear)下 trilinear 永远选不到。两处都与上游galacean/engine-spine逐字一致(非本 PR 引入),但既然是全新落地到本仓,顺手修掉比让它长期潜伏好。 -
[P3] SpineAnimationRenderer.ts:_materialCacheMap 缓存键不含
tintBlack
静态材质缓存键${texture.instanceId}_${blendMode}_${pma}不含 tintBlack,而_setTintBlack每帧 mutate 共享材质。理论上两个共享同一 texture 但 tintBlack 不同的 renderer 会撞同一材质、后跑的 buildPrimitive 覆盖前者。实践中 tintBlack 是 per-asset 的编辑器导出标记,共享 atlas 的实例几乎必然同 tintBlack,故基本不可达;且与上游一致。仅记录,可不动。 -
[P3] 范围蔓延 — core 无关清理混进 feature PR
Engine.ts/Entity.ts/Polyfill.ts/ResourceManager.ts/CustomDataModule.ts里的Object→object、let..in→const..in、AssetPromise.all的 cast,都是与 spine 无关的类型/语法卫生清理。混进 11k 行的 feature PR 会稀释 review 注意力、粗化回滚粒度。建议这类纯 hygiene 单独一个 chore PR(不阻塞本 PR)。附带一致性:新packages/spine/src/index.ts自己却仍用for (let key in ...),与本 PR 给 core 改的const风格相反。 -
[P3] SpineAnimationRenderer._render:
if (!_subPrimitives) return是死守卫
_subPrimitives初始化为[]且从不置 null/undefined,该判永假。真正想跳过空绘制应判_subPrimitives.length === 0(虽然 for 循环 n=0 时也自然空转,当前无害)。
简化建议
架构层面无需简化——facade/kernel 的切分、副作用自注册、ISpineRenderTarget 窄契约都恰到好处,没有过度抽象。唯一可收敛的是上面 [P2]:把 spine 侧对 engine 内部 enum 的"重声明 + 隐式数值同步"用一条 CI 断言钉死,是这次跨包耦合里投入产出比最高的一处硬化(与你已经在 SpineConstant.test.ts 用的手法完全一致,顺手就能补)。
竞品对标:Cocos 内置 Spine 但锁死一工程一版本;Three.js/Babylon 不内置、交给 userland(类 pixi-spine)。Galacean 复用已有的 IPhysics provider 心智做 spine 版本后端,既内置又可换版本内核,是比二者都更自洽的选择——值得肯定。
github-actions
Bot
added
the
documentation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3057 +/- ##
===========================================
+ Coverage 79.37% 80.73% +1.35%
===========================================
Files 903 977 +74
Lines 100632 112568 +11936
Branches 11260 11330 +70
===========================================
+ Hits 79879 90878 +10999
- Misses 20569 21494 +925
- Partials 184 196 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
增量审查（tip `b53a03e98`）
自上轮审查（`369102e3e`，我 `--comment` LGTM）以来，唯一新增 commit 是 `b53a03e98 chore: release v0.0.0-experimental-2.0-spine.0`——纯版本号发布 commit，零代码改动。已逐项核实：
- 只改 18 个
package.json的version字段（每文件 +1/-1），无任何src/或 test 文件变动。 - 18 个包统一 bump 到
0.0.0-experimental-2.0-spine.0，版本一致。 - 未触碰任何
dependencies/peerDependencies版本 pin——@galacean/engine-spine用peerDependencies: { "@galacean/engine": "workspace:*" }，workspace 协议在 publish 时解析，故无需同步跨包版本引用（这是正确做法，非遗漏）。 - CI：build×3 / e2e×4 / lint / codecov(主) / release×2 全绿。
codecov/patch+codecov/projectfail = 发布 commit 覆盖率中性的既知 artifact，非测试失败。
无新问题。发布 commit 本身干净可合。
上轮的 P2/P3（跨包硬编码 RendererUpdateFlags.WorldVolume=0x1 建议加 CI 断言、SpineGenerator/SpineTexture 两处上游移植小瑕疵、_materialCacheMap key 缺 tintBlack、core hygiene 范围蔓延、_subPrimitives 死守卫）均为不阻塞项，本次无代码变更故不重复展开——详见上一条 review。整体仍建议合入。
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (8)
packages/spine/src/runtime/SpineRuntimeRegistry.ts (1)
9-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider warning on silent re-registration.
registerSpineRuntimesilently overwrites any previously registered backend. If a consumer accidentally imports two different core packages (e.g.spine-core-4.2and a futurespine-core-3.8), the second import silently wins with no indication anything went wrong, which could be confusing to debug given how side-effect-driven this registration is.♻️ Optional: warn on overwrite
export function registerSpineRuntime(runtime: ISpineRuntime): void { + if (_runtime && _runtime !== runtime) { + console.warn("`@galacean/engine-spine`: overwriting a previously registered spine runtime backend."); + } _runtime = runtime; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/spine/src/runtime/SpineRuntimeRegistry.ts` around lines 9 - 11, registerSpineRuntime currently overwrites the existing _runtime without any notice, so add a guard in registerSpineRuntime to detect when a runtime is already registered and emit a warning before replacing it. Use the existing registerSpineRuntime and _runtime symbols in SpineRuntimeRegistry to keep the change localized, and make the warning descriptive enough to indicate a second registration occurred and that the previous backend is being replaced.tests/src/spine/SpineRuntimeRegistry.test.ts (1)
6-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOrder-dependent test relying on module singleton state.
This test only passes if it executes before the other two tests in the file, since there's no way to reset the module-scoped
_runtimesingleton. The comment flags this, but it's still fragile if the test file is later refactored (e.g., newitblocks added before this one, ortest.concurrent/shuffle mode is enabled).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/spine/SpineRuntimeRegistry.test.ts` around lines 6 - 9, The test in SpineRuntimeRegistry.test.ts is order-dependent because getSpineRuntime() reads a module-scoped singleton with no reset path. Refactor the registry or test setup so each test can start from a clean state, ideally by adding a reset/helper around the _runtime singleton (or equivalent API) and using it in the test suite instead of relying on this “must run first” case. Keep the existing getSpineRuntime and registry symbols in place, but make the “throws when no runtime is registered” assertion independent of execution order.packages/spine/src/index.ts (1)
20-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnconditional console.log on every import.
Logging the package version on module load pollutes consumer console output unconditionally, especially for a library entrypoint loaded in production apps.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/spine/src/index.ts` around lines 20 - 21, The module entrypoint is logging the package version unconditionally on import, which pollutes consumer output. Remove or guard the console.log in the index.ts module initialization around the version export so it does not run on every import; keep the version constant available via version without side effects.e2e/case/spine-spineboy.ts (1)
22-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
SpineResourcetype instead ofany.
resourceis typed asany, losing type checking on.instantiate()and any other member access.SpineResourceis exported from@galacean/engine-spine.♻️ Proposed fix
-import { SpineAnimationRenderer } from "`@galacean/engine-spine`"; +import { SpineAnimationRenderer, SpineResource } from "`@galacean/engine-spine`"; ... engine.resourceManager .load({ urls: ["/spineboy.json", "/spineboy.atlas", "/spineboy.png"], type: "Spine" }) - .then((resource: any) => { + .then((resource: SpineResource) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/case/spine-spineboy.ts` around lines 22 - 27, The load callback in spine-spineboy currently types `resource` as `any`, which removes type safety for `.instantiate()` and other member access. Update the `.then(...)` handler to use the exported `SpineResource` type from `@galacean/engine-spine` for the `resource` parameter, and keep the rest of the `engine.resourceManager.load(...)` flow unchanged.packages/spine-core-4.2/src/SpineTexture.ts (1)
18-26: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFilter mapping loses information for several mipmap combinations.
spine-core'sTextureFilterhas 7 values (Nearest,Linear,MipMap,MipMapNearestNearest,MipMapLinearNearest,MipMapNearestLinear,MipMapLinearLinear), but this mapping only distinguishesNearest(checked viaminFilter) andMipMapLinearLinear(checked viamagFilter) — every other mipmap variant (MipMap,MipMapNearestNearest,MipMapLinearNearest,MipMapNearestLinear) falls through toBilinear, losing the mipmap/trilinear behavior even thoughGalacean's engine does supportTrilinear. Mixing the check field (minFilterfor one branch,magFilterfor another) is also inconsistent and may not match spine atlas authoring conventions where min/mag filters can differ.Consider checking whichever of
minFilter/magFilterindicates mipmap usage consistently (e.g., treat anyMipMap*value asTrilinear, matching Galacean's simplified 3-mode model).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/spine-core-4.2/src/SpineTexture.ts` around lines 18 - 26, The setFilters mapping in SpineTexture is dropping most mipmap cases by only checking minFilter for Nearest and magFilter for MipMapLinearLinear, so the other MipMap* TextureFilter values incorrectly fall back to Bilinear. Update setFilters to handle all mipmap-related TextureFilter variants consistently, using the same filter field for the decision and mapping any mipmap mode to TextureFilterMode.Trilinear while keeping Nearest as Point and Linear as Bilinear.packages/spine-core-4.2/src/Spine42Runtime.ts (1)
28-40: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAtlas text parsed twice per load.
parseAtlasPageNamesandcreateTextureAtlaseach construct a freshnew TextureAtlas(atlasText), so the same atlas text is parsed twice for every atlas load (per theLoaderUtils.loadTextureAtlasflow:parseAtlasPageNames→ load textures →createTextureAtlas). Consider caching the first parse result and reusing it (e.g., havecreateTextureAtlasreuse the already-parsed pages instead of re-parsing).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/spine-core-4.2/src/Spine42Runtime.ts` around lines 28 - 40, `Spine42Runtime` is parsing the same atlas text twice because `parseAtlasPageNames` and `createTextureAtlas` each create a new `TextureAtlas`. Update the atlas-loading flow so the first parsed `TextureAtlas` (or its pages) is reused by `createTextureAtlas` instead of constructing a second one, and adjust the `LoaderUtils.loadTextureAtlas` path accordingly.packages/spine-core-4.2/src/index.ts (1)
14-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnconditional
console.logon module import.Every import of this package prints a version log line to the console; if this diverges from other Galacean packages' logging convention, consider gating it (e.g., only in dev builds) to avoid noisy console output in production consumer apps. Skip if this matches an existing repo-wide pattern.
#!/bin/bash # Description: Check whether other Galacean packages follow the same version-log-on-import pattern. rg -n '__buildVersion' --type=ts -g '!**/dist/**'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/spine-core-4.2/src/index.ts` around lines 14 - 15, The module entrypoint in index.ts unconditionally logs the package version on import, which can create noisy output in consumer apps. Update the version logging around the version constant and top-level console.log so it follows the repo’s existing convention, gating it behind a dev-only or debug-only check if that is the established pattern; use the version export in index.ts as the reference point and keep the import side effects minimal.tests/src/spine/SpineAnimationRenderer.test.ts (1)
28-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a regression test for the
tintBlackcache-key gap.Current caching test only varies
blendMode; it doesn't catch the missingtintBlackdimension in the cache key (see comment onSpineAnimationRenderer.tsLines 294-324). Once that fix lands, a test asserting two renderers with the same texture/blendMode but differenttintBlackget distinct materials would guard against regression.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/spine/SpineAnimationRenderer.test.ts` around lines 28 - 36, Add a regression test around SpineAnimationRenderer._getMaterial to cover the missing tintBlack cache-key dimension. Extend the existing caching test in SpineAnimationRenderer.test.ts by creating two renderer instances with the same Texture2D and SpineBlendMode but different tintBlack settings, then assert that each call to _getMaterial returns a different material. Use the existing _getMaterial and SpineAnimationRenderer symbols so the test stays tied to the cache behavior and protects the fix from regressing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/spine-core-4.2/src/SpineGenerator.ts`:
- Line 51: The tempVerts preallocation in SpineGenerator is ineffective because
buildPrimitive compares the needed float count against _vertices.length instead
of tempVerts.length, and the destructured const tempVerts keeps pointing to the
old array after reassignment. Update the resize check to use tempVerts length,
and change the local tempVerts binding in buildPrimitive so it can be reassigned
from SpineGenerator.tempVerts when growing, ensuring later writes use the new
buffer immediately.
- Around line 108-144: The switch in SpineGenerator’s attachment handling
violates noSwitchDeclarations because `const` and `let` are declared directly in
`case` clauses. Wrap the bodies for the `RegionAttachment`, `MeshAttachment`,
and `ClippingAttachment` cases in their own blocks so `regionAttachment`,
`meshAttachment`, and `clip` are scoped per case. Keep the existing logic in
`SpineGenerator` unchanged while moving those declarations inside block braces.
In `@packages/spine/src/loader/LoaderUtils.ts`:
- Around line 46-49: Propagate failures from the loader helpers instead of
converting them into successful fallback results. In LoaderUtils, the
Promise.all(texturePromises) and the matching loader path that currently call
reject(...) and then return [] or null should either re-throw the caught error
or stop using the side-effect reject parameter so callers of these helpers can
observe the failure. Update the affected helper methods in LoaderUtils to keep
rejected loads rejected, and preserve the original error when bubbling it up.
- Around line 64-69: In loadTextureAtlas, atlas page URLs are always loaded
through AssetType.Texture, which skips compressed texture handling for .ktx and
.ktx2 pages. Update the page-loading logic to detect each page name’s extension
and choose the compressed texture asset type when appropriate, while keeping the
existing texture loading path for standard image formats. Use the
loadTextureAtlas function and the loadTexturePromises mapping in LoaderUtils as
the place to apply the fix.
In `@packages/spine/src/loader/SpineAtlasLoader.ts`:
- Around line 21-23: The texture path collection in SpineAtlasLoader is out of
sync because imagePaths is populated for ktx/ktx2 but imageExtensions is not, so
LoaderUtils.loadTexturesByPaths cannot select the compressed texture loader.
Update the SpineAtlasLoader logic that builds assetPath (and any related
atlas/image parsing in the same loader) so every pushed URL has a matching entry
in imageExtensions, preserving index alignment for loadTexturesByPaths.
- Around line 68-72: The atlasPath variable in the SpineAtlasLoader is being set
to item.url, but according to the LoadItem contract, the url property is absent
when urls is provided instead. When a load request uses urls (like { urls:
["foo.atlas"], type: "SpineAtlas" }), item.url will be undefined. Replace the
assignment of atlasPath to use the actual resolved atlas path from the urls
array (likely the first element or the one being processed in the current
iteration) instead of item.url, so that loadTextureAtlas receives a valid path
string.
In `@packages/spine/src/loader/SpineLoader.ts`:
- Around line 105-107: The JSON skeleton detection in
SpineLoader._decoder.decode currently only checks
skeletonString.startsWith("{"), so BOM-prefixed or whitespace-prefixed JSON gets
misrouted to the binary parser. Update the SpineLoader logic around
skeletonString and spineLoadContext.skeletonRawData to normalize the decoded
string first (for example, trim leading whitespace and strip any BOM) before
checking whether it is JSON, while keeping the existing decode-and-assign flow
intact.
In `@packages/spine/src/loader/SpineResource.ts`:
- Around line 83-88: The skin attachment scan in SpineResource currently only
checks one placeholder per slot via slot.name, so it can miss textures from
other attachments in the same skin. Update the attachment enumeration in
SpineResource to iterate all skin attachments using skin.getAttachments() or
getAttachmentsForSlot(slot.index), then resolve each attachment’s texture before
adding it to the tracked set.
In `@packages/spine/src/renderer/SpineAnimationRenderer.ts`:
- Around line 294-324: The shared material cache in SpineAnimationRenderer is
missing tintBlack in its cache key, so different renderers can reuse and mutate
the same SpineMaterial with conflicting tint state. Update both _getMaterial and
_clearMaterialCache to include tintBlack in the key alongside
texture.instanceId, blendMode, and premultipliedAlpha, and keep the cache
lookup/deletion logic consistent so each tintBlack variant gets its own cached
material.
---
Nitpick comments:
In `@e2e/case/spine-spineboy.ts`:
- Around line 22-27: The load callback in spine-spineboy currently types
`resource` as `any`, which removes type safety for `.instantiate()` and other
member access. Update the `.then(...)` handler to use the exported
`SpineResource` type from `@galacean/engine-spine` for the `resource` parameter,
and keep the rest of the `engine.resourceManager.load(...)` flow unchanged.
In `@packages/spine-core-4.2/src/index.ts`:
- Around line 14-15: The module entrypoint in index.ts unconditionally logs the
package version on import, which can create noisy output in consumer apps.
Update the version logging around the version constant and top-level console.log
so it follows the repo’s existing convention, gating it behind a dev-only or
debug-only check if that is the established pattern; use the version export in
index.ts as the reference point and keep the import side effects minimal.
In `@packages/spine-core-4.2/src/Spine42Runtime.ts`:
- Around line 28-40: `Spine42Runtime` is parsing the same atlas text twice
because `parseAtlasPageNames` and `createTextureAtlas` each create a new
`TextureAtlas`. Update the atlas-loading flow so the first parsed `TextureAtlas`
(or its pages) is reused by `createTextureAtlas` instead of constructing a
second one, and adjust the `LoaderUtils.loadTextureAtlas` path accordingly.
In `@packages/spine-core-4.2/src/SpineTexture.ts`:
- Around line 18-26: The setFilters mapping in SpineTexture is dropping most
mipmap cases by only checking minFilter for Nearest and magFilter for
MipMapLinearLinear, so the other MipMap* TextureFilter values incorrectly fall
back to Bilinear. Update setFilters to handle all mipmap-related TextureFilter
variants consistently, using the same filter field for the decision and mapping
any mipmap mode to TextureFilterMode.Trilinear while keeping Nearest as Point
and Linear as Bilinear.
In `@packages/spine/src/index.ts`:
- Around line 20-21: The module entrypoint is logging the package version
unconditionally on import, which pollutes consumer output. Remove or guard the
console.log in the index.ts module initialization around the version export so
it does not run on every import; keep the version constant available via version
without side effects.
In `@packages/spine/src/runtime/SpineRuntimeRegistry.ts`:
- Around line 9-11: registerSpineRuntime currently overwrites the existing
_runtime without any notice, so add a guard in registerSpineRuntime to detect
when a runtime is already registered and emit a warning before replacing it. Use
the existing registerSpineRuntime and _runtime symbols in SpineRuntimeRegistry
to keep the change localized, and make the warning descriptive enough to
indicate a second registration occurred and that the previous backend is being
replaced.
In `@tests/src/spine/SpineAnimationRenderer.test.ts`:
- Around line 28-36: Add a regression test around
SpineAnimationRenderer._getMaterial to cover the missing tintBlack cache-key
dimension. Extend the existing caching test in SpineAnimationRenderer.test.ts by
creating two renderer instances with the same Texture2D and SpineBlendMode but
different tintBlack settings, then assert that each call to _getMaterial returns
a different material. Use the existing _getMaterial and SpineAnimationRenderer
symbols so the test stays tied to the cache behavior and protects the fix from
regressing.
In `@tests/src/spine/SpineRuntimeRegistry.test.ts`:
- Around line 6-9: The test in SpineRuntimeRegistry.test.ts is order-dependent
because getSpineRuntime() reads a module-scoped singleton with no reset path.
Refactor the registry or test setup so each test can start from a clean state,
ideally by adding a reset/helper around the _runtime singleton (or equivalent
API) and using it in the test suite instead of relying on this “must run first”
case. Keep the existing getSpineRuntime and registry symbols in place, but make
the “throws when no runtime is registered” assertion independent of execution
order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3c2b84b5-f3ca-4cf1-9fb7-24b8115838dc
⛔ Files ignored due to path filters (6)
e2e/.dev/public/spineboy.pngis excluded by!**/*.pnge2e/.dev/public/tank-pro.pngis excluded by!**/*.pnge2e/fixtures/originImage/Spine_spine-spineboy.jpgis excluded by!**/*.jpge2e/fixtures/originImage/Spine_spine-tint-black.jpgis excluded by!**/*.jpgpackages/shader/src/Shaders/2D/Spine.shaderis excluded by!**/*.shaderpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (62)
e2e/.dev/public/spineboy.atlase2e/.dev/public/spineboy.jsone2e/.dev/public/tank-pro.atlase2e/.dev/public/tank-pro.jsone2e/case/spine-spineboy.tse2e/case/spine-tint-black.tse2e/config.tse2e/package.jsonexamples/package.jsonpackage.jsonpackages/core/package.jsonpackages/core/src/Engine.tspackages/core/src/Entity.tspackages/core/src/Polyfill.tspackages/core/src/asset/ResourceManager.tspackages/core/src/particle/modules/CustomDataModule.tspackages/design/package.jsonpackages/galacean/package.jsonpackages/galacean/src/ShaderPool.tspackages/loader/package.jsonpackages/math/package.jsonpackages/physics-physx/package.jsonpackages/rhi-webgl/package.jsonpackages/shader-compiler/package.jsonpackages/shader/package.jsonpackages/shader/src/Shaders/index.tspackages/spine-core-4.2/package.jsonpackages/spine-core-4.2/src/Spine42Runtime.tspackages/spine-core-4.2/src/SpineGenerator.tspackages/spine-core-4.2/src/SpineTexture.tspackages/spine-core-4.2/src/index.tspackages/spine-core-4.2/src/util/ClearablePool.tspackages/spine-core-4.2/src/util/ReturnablePool.tspackages/spine-core-4.2/tsconfig.jsonpackages/spine/package.jsonpackages/spine/src/SpineConstant.tspackages/spine/src/enums/SpineBlendMode.tspackages/spine/src/index.tspackages/spine/src/loader/LoaderUtils.tspackages/spine/src/loader/SpineAtlasLoader.tspackages/spine/src/loader/SpineLoader.tspackages/spine/src/loader/SpineResource.tspackages/spine/src/loader/index.tspackages/spine/src/renderer/SpineAnimationRenderer.tspackages/spine/src/renderer/SpineMaterial.tspackages/spine/src/renderer/index.tspackages/spine/src/runtime/ISpineRenderTarget.tspackages/spine/src/runtime/ISpineRuntime.tspackages/spine/src/runtime/SpineRuntimeRegistry.tspackages/spine/tsconfig.jsonpackages/ui/package.jsonpackages/xr-webxr/package.jsonpackages/xr/package.jsonrollup.config.jstests/package.jsontests/src/spine/LoaderUtils.test.tstests/src/spine/Pool.test.tstests/src/spine/SpineAnimationRenderer.test.tstests/src/spine/SpineConstant.test.tstests/src/spine/SpineLoader.test.tstests/src/spine/SpineMaterial.test.tstests/src/spine/SpineRuntimeRegistry.test.ts
|
|
||
| const { _clipper, _separateSlots, _subRenderItems, _separateSlotTextureMap } = this; | ||
|
|
||
| const { tempVerts, subRenderItemPool, subPrimitivePool } = SpineGenerator; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
tempVerts resize logic is broken: wrong length comparison and stale local reference.
Two issues combine to make this "grow if needed" logic ineffective:
- Line 122 compares
numFloats(size needed for a single attachment) against_vertices.length(the full render target vertex buffer, unrelated in scale) instead oftempVerts.length. In practice_vertices.lengthis almost always far larger thannumFloats, so the resize branch essentially never fires. - Even when it does fire,
SpineGenerator.tempVertsis reassigned to a new array (line 123), but the localtempVertsused at lines 113/129 was already destructured from the static field at the top of the function (line 51) and still points to the old array — the resize doesn't take effect until the nextbuildPrimitivecall.
Since tempVerts is a plain JS array it auto-grows on out-of-bounds writes, so there's no crash, but the intended pre-allocation never actually happens for large mesh attachments, causing repeated implicit array growth every frame.
🔧 Proposed fix
- case MeshAttachment:
+ case MeshAttachment: {
const meshAttachment = <MeshAttachment>attachment;
attachmentColor = meshAttachment.color;
numFloats = (meshAttachment.worldVerticesLength >> 1) * clippedVertexSize;
- if (numFloats > _vertices.length) {
- SpineGenerator.tempVerts = new Array(numFloats);
+ if (numFloats > tempVerts.length) {
+ tempVerts = SpineGenerator.tempVerts = new Array(numFloats);
}(requires tempVerts to be declared with let instead of destructured const so the local reference can be reassigned)
Also applies to: 121-132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/spine-core-4.2/src/SpineGenerator.ts` at line 51, The tempVerts
preallocation in SpineGenerator is ineffective because buildPrimitive compares
the needed float count against _vertices.length instead of tempVerts.length, and
the destructured const tempVerts keeps pointing to the old array after
reassignment. Update the resize check to use tempVerts length, and change the
local tempVerts binding in buildPrimitive so it can be reassigned from
SpineGenerator.tempVerts when growing, ensuring later writes use the new buffer
immediately.
| switch (attachment.constructor) { | ||
| case RegionAttachment: | ||
| const regionAttachment = <RegionAttachment>attachment; | ||
| attachmentColor = regionAttachment.color; | ||
| numFloats = clippedVertexSize << 2; | ||
| regionAttachment.computeWorldVertices(slot, tempVerts, 0, clippedVertexSize); | ||
| triangles = SpineGenerator.QUAD_TRIANGLES; | ||
| uvs = regionAttachment.uvs; | ||
| texture = regionAttachment.region.texture; | ||
| break; | ||
| case MeshAttachment: | ||
| const meshAttachment = <MeshAttachment>attachment; | ||
| attachmentColor = meshAttachment.color; | ||
| numFloats = (meshAttachment.worldVerticesLength >> 1) * clippedVertexSize; | ||
| if (numFloats > _vertices.length) { | ||
| SpineGenerator.tempVerts = new Array(numFloats); | ||
| } | ||
| meshAttachment.computeWorldVertices( | ||
| slot, | ||
| 0, | ||
| meshAttachment.worldVerticesLength, | ||
| tempVerts, | ||
| 0, | ||
| clippedVertexSize | ||
| ); | ||
| triangles = meshAttachment.triangles; | ||
| uvs = meshAttachment.uvs; | ||
| texture = meshAttachment.region.texture; | ||
| break; | ||
| case ClippingAttachment: | ||
| let clip = <ClippingAttachment>attachment; | ||
| _clipper.clipStart(slot, clip); | ||
| continue; | ||
| default: | ||
| _clipper.clipEndWithSlot(slot); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Wrap switch-case declarations in blocks (Biome error).
Static analysis flags noSwitchDeclarations at lines 110, 119, and 138: const/let declared directly in case clauses are visible to sibling clauses. Wrapping each case body in { } resolves the lint error without changing behavior.
🔧 Proposed fix
switch (attachment.constructor) {
- case RegionAttachment:
- const regionAttachment = <RegionAttachment>attachment;
- attachmentColor = regionAttachment.color;
- numFloats = clippedVertexSize << 2;
- regionAttachment.computeWorldVertices(slot, tempVerts, 0, clippedVertexSize);
- triangles = SpineGenerator.QUAD_TRIANGLES;
- uvs = regionAttachment.uvs;
- texture = regionAttachment.region.texture;
- break;
- case MeshAttachment:
- const meshAttachment = <MeshAttachment>attachment;
- attachmentColor = meshAttachment.color;
- numFloats = (meshAttachment.worldVerticesLength >> 1) * clippedVertexSize;
- if (numFloats > _vertices.length) {
- SpineGenerator.tempVerts = new Array(numFloats);
- }
- meshAttachment.computeWorldVertices(
- slot,
- 0,
- meshAttachment.worldVerticesLength,
- tempVerts,
- 0,
- clippedVertexSize
- );
- triangles = meshAttachment.triangles;
- uvs = meshAttachment.uvs;
- texture = meshAttachment.region.texture;
- break;
- case ClippingAttachment:
- let clip = <ClippingAttachment>attachment;
- _clipper.clipStart(slot, clip);
- continue;
+ case RegionAttachment: {
+ const regionAttachment = <RegionAttachment>attachment;
+ attachmentColor = regionAttachment.color;
+ numFloats = clippedVertexSize << 2;
+ regionAttachment.computeWorldVertices(slot, tempVerts, 0, clippedVertexSize);
+ triangles = SpineGenerator.QUAD_TRIANGLES;
+ uvs = regionAttachment.uvs;
+ texture = regionAttachment.region.texture;
+ break;
+ }
+ case MeshAttachment: {
+ const meshAttachment = <MeshAttachment>attachment;
+ attachmentColor = meshAttachment.color;
+ numFloats = (meshAttachment.worldVerticesLength >> 1) * clippedVertexSize;
+ if (numFloats > _vertices.length) {
+ SpineGenerator.tempVerts = new Array(numFloats);
+ }
+ meshAttachment.computeWorldVertices(
+ slot,
+ 0,
+ meshAttachment.worldVerticesLength,
+ tempVerts,
+ 0,
+ clippedVertexSize
+ );
+ triangles = meshAttachment.triangles;
+ uvs = meshAttachment.uvs;
+ texture = meshAttachment.region.texture;
+ break;
+ }
+ case ClippingAttachment: {
+ let clip = <ClippingAttachment>attachment;
+ _clipper.clipStart(slot, clip);
+ continue;
+ }📝 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.
| switch (attachment.constructor) { | |
| case RegionAttachment: | |
| const regionAttachment = <RegionAttachment>attachment; | |
| attachmentColor = regionAttachment.color; | |
| numFloats = clippedVertexSize << 2; | |
| regionAttachment.computeWorldVertices(slot, tempVerts, 0, clippedVertexSize); | |
| triangles = SpineGenerator.QUAD_TRIANGLES; | |
| uvs = regionAttachment.uvs; | |
| texture = regionAttachment.region.texture; | |
| break; | |
| case MeshAttachment: | |
| const meshAttachment = <MeshAttachment>attachment; | |
| attachmentColor = meshAttachment.color; | |
| numFloats = (meshAttachment.worldVerticesLength >> 1) * clippedVertexSize; | |
| if (numFloats > _vertices.length) { | |
| SpineGenerator.tempVerts = new Array(numFloats); | |
| } | |
| meshAttachment.computeWorldVertices( | |
| slot, | |
| 0, | |
| meshAttachment.worldVerticesLength, | |
| tempVerts, | |
| 0, | |
| clippedVertexSize | |
| ); | |
| triangles = meshAttachment.triangles; | |
| uvs = meshAttachment.uvs; | |
| texture = meshAttachment.region.texture; | |
| break; | |
| case ClippingAttachment: | |
| let clip = <ClippingAttachment>attachment; | |
| _clipper.clipStart(slot, clip); | |
| continue; | |
| default: | |
| _clipper.clipEndWithSlot(slot); | |
| continue; | |
| } | |
| switch (attachment.constructor) { | |
| case RegionAttachment: { | |
| const regionAttachment = <RegionAttachment>attachment; | |
| attachmentColor = regionAttachment.color; | |
| numFloats = clippedVertexSize << 2; | |
| regionAttachment.computeWorldVertices(slot, tempVerts, 0, clippedVertexSize); | |
| triangles = SpineGenerator.QUAD_TRIANGLES; | |
| uvs = regionAttachment.uvs; | |
| texture = regionAttachment.region.texture; | |
| break; | |
| } | |
| case MeshAttachment: { | |
| const meshAttachment = <MeshAttachment>attachment; | |
| attachmentColor = meshAttachment.color; | |
| numFloats = (meshAttachment.worldVerticesLength >> 1) * clippedVertexSize; | |
| if (numFloats > _vertices.length) { | |
| SpineGenerator.tempVerts = new Array(numFloats); | |
| } | |
| meshAttachment.computeWorldVertices( | |
| slot, | |
| 0, | |
| meshAttachment.worldVerticesLength, | |
| tempVerts, | |
| 0, | |
| clippedVertexSize | |
| ); | |
| triangles = meshAttachment.triangles; | |
| uvs = meshAttachment.uvs; | |
| texture = meshAttachment.region.texture; | |
| break; | |
| } | |
| case ClippingAttachment: { | |
| let clip = <ClippingAttachment>attachment; | |
| _clipper.clipStart(slot, clip); | |
| continue; | |
| } | |
| default: | |
| _clipper.clipEndWithSlot(slot); | |
| continue; | |
| } |
🧰 Tools
🪛 Biome (2.5.1)
[error] 110-110: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
[error] 119-119: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
[error] 138-138: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/spine-core-4.2/src/SpineGenerator.ts` around lines 108 - 144, The
switch in SpineGenerator’s attachment handling violates noSwitchDeclarations
because `const` and `let` are declared directly in `case` clauses. Wrap the
bodies for the `RegionAttachment`, `MeshAttachment`, and `ClippingAttachment`
cases in their own blocks so `regionAttachment`, `meshAttachment`, and `clip`
are scoped per case. Keep the existing logic in `SpineGenerator` unchanged while
moving those declarations inside block braces.
Source: Linters/SAST tools
| return Promise.all(texturePromises).catch((error) => { | ||
| reject(error); | ||
| return []; | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate loader failures instead of fulfilling fallback values.
Line 48 and Line 77 convert failed loads into fulfilled []/null promises after calling reject, so any direct caller of these helpers can observe a successful result for a failed asset load. Re-throw after wrapping/rejecting, or remove the side-effect reject parameter.
Proposed fix
return Promise.all(texturePromises).catch((error) => {
reject(error);
- return [];
+ throw error;
}); .catch((err) => {
- reject(new Error(`Spine Atlas: ${atlasPath} load error: ${err}`));
- return null;
+ const error = new Error(`Spine Atlas: ${atlasPath} load error: ${err}`);
+ reject(error);
+ throw error;
})Also applies to: 75-78
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/spine/src/loader/LoaderUtils.ts` around lines 46 - 49, Propagate
failures from the loader helpers instead of converting them into successful
fallback results. In LoaderUtils, the Promise.all(texturePromises) and the
matching loader path that currently call reject(...) and then return [] or null
should either re-throw the caught error or stop using the side-effect reject
parameter so callers of these helpers can observe the failure. Update the
affected helper methods in LoaderUtils to keep rejected loads rejected, and
preserve the original error when bubbling it up.
| const loadTexturePromises = pageNames.map((name) => { | ||
| const textureUrl = baseUrl + name; | ||
| return resourceManager.load({ | ||
| url: textureUrl, | ||
| type: AssetType.Texture | ||
| }) as unknown as Promise<Texture2D>; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use compressed texture loaders for parsed atlas pages.
loadTextureAtlas parses page names from the atlas but always loads them as AssetType.Texture, so atlas pages ending in .ktx or .ktx2 will bypass the KTX/KTX2 loaders.
Proposed fix
const loadTexturePromises = pageNames.map((name) => {
const textureUrl = baseUrl + name;
+ const ext = name.match(/\.(\w+)(\?|$)/)?.[1]?.toLowerCase();
+ const textureType = ext === "ktx" ? AssetType.KTX : ext === "ktx2" ? AssetType.KTX2 : AssetType.Texture;
return resourceManager.load({
url: textureUrl,
- type: AssetType.Texture
+ type: textureType
}) as unknown as Promise<Texture2D>;
});📝 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.
| const loadTexturePromises = pageNames.map((name) => { | |
| const textureUrl = baseUrl + name; | |
| return resourceManager.load({ | |
| url: textureUrl, | |
| type: AssetType.Texture | |
| }) as unknown as Promise<Texture2D>; | |
| const loadTexturePromises = pageNames.map((name) => { | |
| const textureUrl = baseUrl + name; | |
| const ext = name.match(/\.(\w+)(\?|$)/)?.[1]?.toLowerCase(); | |
| const textureType = ext === "ktx" ? AssetType.KTX : ext === "ktx2" ? AssetType.KTX2 : AssetType.Texture; | |
| return resourceManager.load({ | |
| url: textureUrl, | |
| type: textureType | |
| }) as unknown as Promise<Texture2D>; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/spine/src/loader/LoaderUtils.ts` around lines 64 - 69, In
loadTextureAtlas, atlas page URLs are always loaded through AssetType.Texture,
which skips compressed texture handling for .ktx and .ktx2 pages. Update the
page-loading logic to detect each page name’s extension and choose the
compressed texture asset type when appropriate, while keeping the existing
texture loading path for standard image formats. Use the loadTextureAtlas
function and the loadTexturePromises mapping in LoaderUtils as the place to
apply the fix.
| if (["png", "jpg", "webp", "jpeg", "ktx", "ktx2"].includes(ext)) { | ||
| assetPath.imagePaths.push(url); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep imageExtensions aligned with imagePaths.
LoaderUtils.loadTexturesByPaths reads imageExtensions[index] to choose KTX/KTX2 loaders, but these branches only push paths. Explicit compressed texture URLs therefore fall back to AssetType.Texture.
Proposed fix
- let ext = SpineLoader._getUrlExtension(url);
+ const ext = SpineLoader._getUrlExtension(url)?.toLowerCase();
if (!ext) return;
if (ext === "atlas") {
assetPath.atlasPath = url;
}
if (["png", "jpg", "webp", "jpeg", "ktx", "ktx2"].includes(ext)) {
assetPath.imagePaths.push(url);
+ assetPath.imageExtensions.push(ext);
} for (let key in atlasDependency) {
const imageVirtualPath = atlasDependency[key];
assetPath.imagePaths.push(imageVirtualPath);
+ assetPath.imageExtensions.push(SpineLoader._getUrlExtension(imageVirtualPath)?.toLowerCase() ?? "");
}Also applies to: 33-36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/spine/src/loader/SpineAtlasLoader.ts` around lines 21 - 23, The
texture path collection in SpineAtlasLoader is out of sync because imagePaths is
populated for ktx/ktx2 but imageExtensions is not, so
LoaderUtils.loadTexturesByPaths cannot select the compressed texture loader.
Update the SpineAtlasLoader logic that builds assetPath (and any related
atlas/image parsing in the same loader) so every pushed URL has a matching entry
in imageExtensions, preserving index alignment for loadTexturesByPaths.
| const imagePaths = spineAtlasAsset.imagePaths; | ||
| if (imagePaths.length === 0) { | ||
| const atlasPath = item.url; | ||
| LoaderUtils.loadTextureAtlas(atlasPath, engine, reject) | ||
| .then((textureAtlas) => { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use the resolved atlas path when loading from urls.
Line 70 reads item.url, but the LoadItem contract makes url absent when urls is used. A { urls: ["foo.atlas"], type: "SpineAtlas" } load will pass undefined into loadTextureAtlas.
Proposed fix
const imagePaths = spineAtlasAsset.imagePaths;
if (imagePaths.length === 0) {
- const atlasPath = item.url;
LoaderUtils.loadTextureAtlas(atlasPath, engine, reject)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/spine/src/loader/SpineAtlasLoader.ts` around lines 68 - 72, The
atlasPath variable in the SpineAtlasLoader is being set to item.url, but
according to the LoadItem contract, the url property is absent when urls is
provided instead. When a load request uses urls (like { urls: ["foo.atlas"],
type: "SpineAtlas" }), item.url will be undefined. Replace the assignment of
atlasPath to use the actual resolved atlas path from the urls array (likely the
first element or the one being processed in the current iteration) instead of
item.url, so that loadTextureAtlas receives a valid path string.
| const skeletonString = SpineLoader._decoder.decode(skeletonRawData); | ||
| if (skeletonString.startsWith("{")) { | ||
| spineLoadContext.skeletonRawData = skeletonString; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle BOM/leading whitespace when detecting JSON skeletons.
Line 106 treats JSON as text only when the decoded buffer starts exactly with {; valid JSON with a BOM or leading whitespace will be passed to the binary parser.
Proposed fix
- const skeletonString = SpineLoader._decoder.decode(skeletonRawData);
- if (skeletonString.startsWith("{")) {
+ const skeletonString = SpineLoader._decoder.decode(skeletonRawData).replace(/^\uFEFF/, "");
+ if (skeletonString.trimStart().startsWith("{")) {
spineLoadContext.skeletonRawData = skeletonString;
}📝 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.
| const skeletonString = SpineLoader._decoder.decode(skeletonRawData); | |
| if (skeletonString.startsWith("{")) { | |
| spineLoadContext.skeletonRawData = skeletonString; | |
| const skeletonString = SpineLoader._decoder.decode(skeletonRawData).replace(/^\uFEFF/, ""); | |
| if (skeletonString.trimStart().startsWith("{")) { | |
| spineLoadContext.skeletonRawData = skeletonString; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/spine/src/loader/SpineLoader.ts` around lines 105 - 107, The JSON
skeleton detection in SpineLoader._decoder.decode currently only checks
skeletonString.startsWith("{"), so BOM-prefixed or whitespace-prefixed JSON gets
misrouted to the binary parser. Update the SpineLoader logic around
skeletonString and spineLoadContext.skeletonRawData to normalize the decoded
string first (for example, trim leading whitespace and strip any BOM) before
checking whether it is JSON, while keeping the existing decode-and-assign flow
intact.
| for (let i = 0, n = skins.length; i < n; i++) { | ||
| for (let j = 0, m = slots.length; j < m; j++) { | ||
| const slot = slots[j]; | ||
| const attachment = skins[i].getAttachment(slot.index, slot.name); | ||
| const texture = <Texture2D>(<RegionAttachment | MeshAttachment>attachment)?.region?.texture.texture; | ||
| if (texture) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect available Spine Skin attachment enumeration APIs in the checked-out sources.
rg -n -C 4 'class\s+Skin|interface\s+Skin|getAttachments\(|attachments\s*:' packagesRepository: galacean/engine
Length of output: 1312
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== packages/spine/src/loader/SpineResource.ts (relevant range) ==="
cat -n packages/spine/src/loader/SpineResource.ts | sed -n '1,180p'
echo
echo "=== Spine-related API definitions in repository ==="
rg -n -C 3 'getAttachments\(|class\s+Skin|interface\s+Skin|attachments\s*:' packages/spine packages -g '!**/dist/**' -g '!**/build/**'Repository: galacean/engine
Length of output: 5560
🌐 Web query:
Spine 4.2 TypeScript Skin API getAttachments getAttachment enumerate skin entries attachments
💡 Result:
In Spine 4.2 TypeScript (spine-ts), the Skin API provides methods to access and enumerate attachments stored within a skin. The Skin class stores attachments using slot indices and names as keys [1][2]. Key methods in the Skin class include: getAttachment(slotIndex: number, name: string): Attachment | null This method returns the attachment for a specific slot index and attachment name within the skin [1]. getAttachments: Array This method returns an array containing all attachments currently stored in the skin [1]. Each SkinEntry contains the slotIndex, the name (skin placeholder name), and the Attachment object itself [1]. getAttachmentsForSlot(slotIndex: number, attachments: Array): void This is a utility method to populate a provided array with all attachments specifically for a given slot index [1]. For skeletons, the Skeleton class provides high-level access to attachments, which automatically checks the current skin and falls back to the default skin if necessary [3]: getAttachment(slotIndex: number, attachmentName: string): Attachment | null This method searches the current skin for the attachment; if not found, it subsequently checks the SkeletonData's default skin [3][2]. getAttachmentByName(slotName: string, attachmentName: string): Attachment | null A convenience method that looks up an attachment using string-based slot and attachment names by first resolving the slot index [3]. These APIs allow you to programmatically inspect, modify, or create skins at runtime, decoupling animations and code from specific attachments [4]. To enumerate all attachments in a skin, you typically call skin.getAttachments and iterate over the resulting array of SkinEntry objects [1].
Citations:
- 1: https://github.com/EsotericSoftware/spine-runtimes/blob/393a6c2b/spine-ts/spine-core/src/Skin.ts
- 2: https://tr.esotericsoftware.com/spine-runtime-skins
- 3: https://github.com/EsotericSoftware/spine-runtimes/blob/refs/heads/4.2/spine-ts/spine-core/src/Skeleton.ts
- 4: https://en.esotericsoftware.com/spine-runtime-skins
Enumerate all skin attachments here. slot.name only checks a single placeholder per slot, so textures referenced by other entries in the same skin can be missed and end up untracked. Iterate skin.getAttachments() or getAttachmentsForSlot(slot.index) instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/spine/src/loader/SpineResource.ts` around lines 83 - 88, The skin
attachment scan in SpineResource currently only checks one placeholder per slot
via slot.name, so it can miss textures from other attachments in the same skin.
Update the attachment enumeration in SpineResource to iterate all skin
attachments using skin.getAttachments() or getAttachmentsForSlot(slot.index),
then resolve each attachment’s texture before adding it to the tracked set.
| _getMaterial(texture: Texture2D, blendMode: SpineBlendMode): Material { | ||
| const engine = this.engine; | ||
| const premultipliedAlpha = this.premultipliedAlpha; | ||
| const tintBlack = this.tintBlack; | ||
|
|
||
| const key = `${texture.instanceId}_${blendMode}_${premultipliedAlpha ? 1 : 0}`; | ||
| let cached = SpineAnimationRenderer._materialCacheMap.get(key); | ||
| if (!cached) { | ||
| cached = new SpineMaterial(engine); | ||
| cached.isGCIgnored = true; | ||
| SpineAnimationRenderer._materialCacheMap.set(key, cached); | ||
| } | ||
| cached._setBlendMode(blendMode, premultipliedAlpha); | ||
| cached._setTexture(texture); | ||
| cached._setTintBlack(tintBlack); | ||
| cached._setPremultipliedAlpha(premultipliedAlpha); | ||
| return cached; | ||
| } | ||
|
|
||
| private _clearMaterialCache(): void { | ||
| const materialCache = SpineAnimationRenderer._materialCacheMap; | ||
| const premultipliedAlpha = this.premultipliedAlpha; | ||
| const materials = this._materials; | ||
| for (let i = 0, len = materials.length; i < len; i += 1) { | ||
| const material = materials[i] as SpineMaterial; | ||
| const texture = material.shaderData.getTexture("material_SpineTexture"); | ||
| const blendMode = material._getBlendMode(); | ||
| const key = `${texture.instanceId}_${blendMode}_${premultipliedAlpha ? 1 : 0}`; | ||
| materialCache.delete(key); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Material cache key omits tintBlack, causing shared-material corruption across renderers.
_materialCacheMap is a static map shared by every SpineAnimationRenderer instance, but the cache key at Line 299 (_getMaterial) and Line 321 (_clearMaterialCache) is built from texture.instanceId, blendMode, and premultipliedAlpha only — tintBlack is never part of it. Since tintBlack is a per-material macro (RENDERER_TINT_BLACK), two renderers that share the same texture/blendMode/premultipliedAlpha combo but differ in tintBlack will resolve to the same cached SpineMaterial instance, and each _getMaterial() call mutates it in place (Lines 306-309). Whichever renderer updates last in a frame silently overrides the tint state for every other renderer sharing that key — a very likely scenario since multiple instances of the same skeleton/atlas are common in Spine scenes.
🐛 Proposed fix
- const key = `${texture.instanceId}_${blendMode}_${premultipliedAlpha ? 1 : 0}`;
+ const key = `${texture.instanceId}_${blendMode}_${premultipliedAlpha ? 1 : 0}_${tintBlack ? 1 : 0}`;
let cached = SpineAnimationRenderer._materialCacheMap.get(key);- const key = `${texture.instanceId}_${blendMode}_${premultipliedAlpha ? 1 : 0}`;
+ const key = `${texture.instanceId}_${blendMode}_${premultipliedAlpha ? 1 : 0}_${this.tintBlack ? 1 : 0}`;
materialCache.delete(key);📝 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.
| _getMaterial(texture: Texture2D, blendMode: SpineBlendMode): Material { | |
| const engine = this.engine; | |
| const premultipliedAlpha = this.premultipliedAlpha; | |
| const tintBlack = this.tintBlack; | |
| const key = `${texture.instanceId}_${blendMode}_${premultipliedAlpha ? 1 : 0}`; | |
| let cached = SpineAnimationRenderer._materialCacheMap.get(key); | |
| if (!cached) { | |
| cached = new SpineMaterial(engine); | |
| cached.isGCIgnored = true; | |
| SpineAnimationRenderer._materialCacheMap.set(key, cached); | |
| } | |
| cached._setBlendMode(blendMode, premultipliedAlpha); | |
| cached._setTexture(texture); | |
| cached._setTintBlack(tintBlack); | |
| cached._setPremultipliedAlpha(premultipliedAlpha); | |
| return cached; | |
| } | |
| private _clearMaterialCache(): void { | |
| const materialCache = SpineAnimationRenderer._materialCacheMap; | |
| const premultipliedAlpha = this.premultipliedAlpha; | |
| const materials = this._materials; | |
| for (let i = 0, len = materials.length; i < len; i += 1) { | |
| const material = materials[i] as SpineMaterial; | |
| const texture = material.shaderData.getTexture("material_SpineTexture"); | |
| const blendMode = material._getBlendMode(); | |
| const key = `${texture.instanceId}_${blendMode}_${premultipliedAlpha ? 1 : 0}`; | |
| materialCache.delete(key); | |
| } | |
| } | |
| _getMaterial(texture: Texture2D, blendMode: SpineBlendMode): Material { | |
| const engine = this.engine; | |
| const premultipliedAlpha = this.premultipliedAlpha; | |
| const tintBlack = this.tintBlack; | |
| const key = `${texture.instanceId}_${blendMode}_${premultipliedAlpha ? 1 : 0}_${tintBlack ? 1 : 0}`; | |
| let cached = SpineAnimationRenderer._materialCacheMap.get(key); | |
| if (!cached) { | |
| cached = new SpineMaterial(engine); | |
| cached.isGCIgnored = true; | |
| SpineAnimationRenderer._materialCacheMap.set(key, cached); | |
| } | |
| cached._setBlendMode(blendMode, premultipliedAlpha); | |
| cached._setTexture(texture); | |
| cached._setTintBlack(tintBlack); | |
| cached._setPremultipliedAlpha(premultipliedAlpha); | |
| return cached; | |
| } | |
| private _clearMaterialCache(): void { | |
| const materialCache = SpineAnimationRenderer._materialCacheMap; | |
| const premultipliedAlpha = this.premultipliedAlpha; | |
| const materials = this._materials; | |
| for (let i = 0, len = materials.length; i < len; i += 1) { | |
| const material = materials[i] as SpineMaterial; | |
| const texture = material.shaderData.getTexture("material_SpineTexture"); | |
| const blendMode = material._getBlendMode(); | |
| const key = `${texture.instanceId}_${blendMode}_${premultipliedAlpha ? 1 : 0}_${this.tintBlack ? 1 : 0}`; | |
| materialCache.delete(key); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/spine/src/renderer/SpineAnimationRenderer.ts` around lines 294 -
324, The shared material cache in SpineAnimationRenderer is missing tintBlack in
its cache key, so different renderers can reuse and mutate the same
SpineMaterial with conflicting tint state. Update both _getMaterial and
_clearMaterialCache to include tintBlack in the key alongside
texture.instanceId, blendMode, and premultipliedAlpha, and keep the cache
lookup/deletion logic consistent so each tintBlack variant gets its own cached
material.
Vendors the spine 3.8 runtime (no npm package exists for that line; @esotericsoftware/spine-core starts at 4.0.1) and wires it into the existing ISpineRuntime/SpineRuntimeRegistry architecture alongside spine-core-4.2, so `import "@galacean/engine-spine-core-3.8"` registers a working 3.8 backend. Adapts the 4.2-derived SpineGenerator/Spine38Runtime for real 3.8 API differences: no-arg updateWorldTransform (no Physics enum), Bone- instead of Slot-based RegionAttachment.computeWorldVertices, a synchronous TextureAtlas(text, textureLoader) constructor, and an extra unused parameter on SkeletonClipping.clipTriangles. Spine38Runtime intentionally doesn't `implements ISpineRuntime`, since the 4.2 npm package has drifted from 3.8's shape (added TextureAtlas.setTextures, dropped Skeleton/SkeletonData's updateCacheReset/findBoneIndex/findSlotIndex/findPathConstraintIndex) enough that TypeScript's structural check fails despite every member the interface actually calls lining up; it's registered via a single cast instead. Adds two examples (spine-keli-4.2.ts, spine-otakugirl-3.8.ts) that verify both backends end-to-end against real assets, including a genuine spine 3.8.99 binary skeleton exercising the SkeletonBinary parse path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ture property SpineResource._associationTextureInSkeletonData read `region.texture.texture`, but `region.texture` is already the SpineTexture instance - getting its backing Texture2D needs `.getImage()`, same as every other texture read site in SpineGenerator. Because TextureRegion.texture is typed `any` in the npm 4.2 package, the extra `.texture` compiled fine but always resolved to undefined at runtime, so textures were never associated as a super-resource of their SpineResource and lost that GC protection. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2 participants
Supersedes #3050 (same branch, pushed directly to this repo instead of a fork so CI/collaborators have direct access).
See #3050 for the full RFC and discussion.
Summary by CodeRabbit