Skip to content

feat(spx-gui): infer generated sprite defaults for character collision and pivot#3161

Open
cn0809 wants to merge 5 commits into
goplus:devfrom
cn0809:issue-2785
Open

feat(spx-gui): infer generated sprite defaults for character collision and pivot#3161
cn0809 wants to merge 5 commits into
goplus:devfrom
cn0809:issue-2785

Conversation

@cn0809
Copy link
Copy Markdown
Collaborator

@cn0809 cn0809 commented May 15, 2026

update: #2785

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces automatic collision shape detection and pivot inference for generated sprites, specifically targeting character sprites in side-scrolling or angled-top-down perspectives to set the pivot at the feet. Key changes include the implementation of inferPivotDelta using image analysis and the addition of logic to determine collision types based on sprite categories. Review feedback suggests enhancing the image analysis process with timeouts or abort signals to prevent UI blocking and recommends cloning costume objects before applying pivot changes to avoid unintended mutations of the preview state.

Comment thread spx-gui/src/models/spx/gen/sprite-gen.ts
Comment thread spx-gui/src/models/spx/gen/sprite-gen.ts
// infer it after the generated default costume image is available.
private async inferPivotDelta(costume: Costume): Promise<CostumePivot | null> {
if (!shouldUseFeetPivot(this.settings.category, this.settings.perspective)) return null
const rect = await getContentBoundingRect(await toNativeFile(costume.img))
Copy link
Copy Markdown
Collaborator

@nighca nighca May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我看这里通过 getContentBoundingRect 来检查内容,那应该能很大程度上解决我早上说的图片有边距的问题,因为生成的时候边距对应的就是透明像素

另外我在想要不要简化下用户对 pivot 的设置,它可能包括:

  1. 允许在生成的过程中对我们自动推断出来的 pivot 进行调整

  2. 在生成完成添加到项目中后,pivot 的设置入口往外放一点

    现在 pivot 只能跟着 collision 一起设置,而 collision 设置的入口需要把项目和 sprite 的物理效果打开才是可见的;但 pivot 对没开物理效果的项目/sprite 也是有意义的

这样即使对于特殊的 sprite 生成,我们取像素内容得到的 pivot 不够准确,也不是问题了

当然用图像理解模型帮我们推断 pivot 也是可行的,那个会比我们通过 getContentBoundingRect 更准确

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pivot 单独设置 & 入口往外放 先在功能上实现了一版,入口具体怎么放有点不确定,跟设计师同步了下,等他们考虑完一起确认下交互方案吧

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我赞同在生成过程中自动推断 pivot。

但我建议先不要在 sprite generator 里提供手动调整 pivot 的入口,而是把这个入口放到「地图编辑」模式下。因为 pivot 是否合理,往往要结合 sprite 在地图中的站位、移动、碰撞和运行效果来看,生成阶段更适合给出一个默认值。

我的主要顾虑是:如果在 generator 中让用户基于当前 costume 调整 pivot,但这个调整实际会影响整个 sprite,那么对于 costume 差异很大的 sprite,比如站立、跳跃、攻击等,共用同一个 pivot 未必合理,用户也容易误解调整范围。

除非后续支持不同 costume 各自设置 pivot,否则我倾向于:

  1. 生成时自动推断 pivot;
  2. 手动调整入口放到地图编辑里;
  3. 对素材库已有 sprite 的 pivot 推断规则先明确,尤其是是否以默认 costume 为准;
  4. 如果依赖默认 costume,那么现在「sprites/costumes 编辑时,选中的 costume 会立即保存为默认 costume」的机制可能也需要一起调整。

Copy link
Copy Markdown
Contributor

@fennoai fennoai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR successfully addresses #2785 by inferring collision shape and feet-pivot defaults for generated character sprites, renaming the editor to PivotCollisionEditor, and rendering the pivot marker on the sprite preview node. The core logic is sound. The following findings are worth addressing before merge.

if (!props.enableCollisionEditing) {
dirty.value = false
return
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale collision state when enableCollisionEditing flips back to true

resetValues returns early here without resetting colliderSize and colliderPos. If the user edits the collider while editing is enabled, then enableCollisionEditing flips to false and later back to true, the collision UI will re-initialize from stale in-memory state rather than fresh costume data.

Consider always resetting colliderSize/colliderPos to costume-derived defaults unconditionally, and moving dirty.value = false to the very end of the function.

if (!props.enableCollisionEditing) {
// Even when collision editing is hidden, keep any existing collider aligned with the
// artwork after the pivot moves. Otherwise re-enabling physics later would reveal a
// collider that has drifted relative to the sprite image.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment may be misleading for CollisionShapeType.Auto

The comment says "any existing collider", but Auto shapes are also included in this branch. For Auto sprites the collisionPivot adjustment may be a no-op depending on how the engine resolves it. Consider documenting that Auto is intentionally included, or add a guard restricting this adjustment to explicit (non-Auto) shapes.

pivotMarkerNode.zIndex(props.project.zorder.length)
})

function updateLocalConfigByShape(node: Konva.Node) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

watchEffect may trigger excessive Konva zIndex calls

This effect re-runs on every change to props.project.zorder.length — i.e. whenever any sprite is added/removed. Every currently-selected SpriteNode then calls Konva's zIndex setter, which triggers a full layer sort. Consider switching to watch([() => props.selected, () => props.project.zorder.length], ...) with an explicit check to skip the setter when the zIndex is already correct.

{ x: 1, y: 11, width: 4, height: 2, cornerRadius: 1, fill: '#CBD2D8', listening: false },
{ x: 19, y: 11, width: 4, height: 2, cornerRadius: 1, fill: '#CBD2D8', listening: false },
{ x: 11, y: 1, width: 2, height: 4, cornerRadius: 1, fill: '#CBD2D8', listening: false },
{ x: 11, y: 19, width: 2, height: 4, cornerRadius: 1, fill: '#CBD2D8', listening: false },
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pivot marker shape logic and constants are duplicated verbatim in PivotCollisionEditor.vue

pivotMarkerSize, pivotMarkerViewBoxSize, pivotMarkerDrawingGroupConfig, pivotMarkerCircleConfig, pivotMarkerOuterTabConfigs, pivotMarkerInnerShapeConfigs, and pivotMarkerRingConfig are copy-pasted between these two files. Any geometry change must be applied in both places. Consider extracting a composable (e.g. usePivotMarkerConfigs(color: string)) shared by both components.

const pivotMarkerInnerShapeConfigs = computed<RectConfig[]>(
() =>
[
{ x: 1, y: 11, width: 4, height: 2, cornerRadius: 1, fill: '#CBD2D8', listening: false },
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent marker color vs PivotCollisionEditor.vue

SpriteNode uses #CBD2D8 for the inner shapes and ring, while PivotCollisionEditor uses #36C2CF for the same elements. If this distinction is intentional (passive map overlay vs. interactive editor), the values should be named constants — e.g. PIVOT_MARKER_COLOR_PASSIVE / PIVOT_MARKER_COLOR_ACTIVE — so the intent is visible to reviewers and designers.

{ x: 19, y: 11, width: 4, height: 2, cornerRadius: 1, fill: '#CBD2D8', listening: false },
{ x: 11, y: 1, width: 2, height: 4, cornerRadius: 1, fill: '#CBD2D8', listening: false },
{ x: 11, y: 19, width: 2, height: 4, cornerRadius: 1, fill: '#CBD2D8', listening: false },
{ x: 9, y: 11, width: 6, height: 2, cornerRadius: 1, fill: '#CBD2D8', listening: false },
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pivotMarkerOuterTabConfigs / pivotMarkerInnerShapeConfigs should be module-level constants

These computed() wrappers read no reactive dependencies — they return arrays of hardcoded literals. Using computed() creates unnecessary reactive overhead. Declaring them as plain const arrays at module level (or inside the shared composable) would be cleaner.

const defaultCostume = await defaultCostumeGen.finish()
this.costumes.push(defaultCostumeGen)
// Cache the inferred delta here so `finish()` can stay synchronous while still using image analysis.
// We also persist this value to survive export/load before the sprite is finally adopted.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: "Cache" is imprecise wording

This is the first (and only) computation of inferredPivotDelta, not a retrieval from a cache. "Pre-compute" or "eagerly compute" would be more accurate.

if (imageIndex != null) inits.imageIndex = imageIndex
if (selectedItem != null) inits.selectedItem = selectedItem
if (animationGenIdBindings != null) inits.animationGenIdBindings = animationGenIdBindings
if (inferredPivotDelta != null) inits.inferredPivotDelta = inferredPivotDelta
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inferredPivotDelta loaded from disk without runtime type validation

The config is cast with as RawSpriteGenConfig — no runtime check verifies that inferredPivotDelta is an object with finite numeric x and y. A crafted or corrupted project file could supply { x: NaN, y: Infinity }, which would then be applied via sprite.applyCostumesPivotChange() and silently corrupt all costume pivots. Add a guard at the load site: verify it is a plain object with finite-number x and y before assigning to inits.inferredPivotDelta.

case SpriteCategory.Unspecified:
default:
return CollisionShapeType.None
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default fallthrough hides future unhandled SpriteCategory members

The default branch makes this switch non-exhaustive at the type level. If a new SpriteCategory is added, TypeScript will silently return CollisionShapeType.None without a compiler warning. Consider removing default and listing all existing members explicitly so the compiler enforces exhaustiveness.

x: (rect.x + rect.width / 2) / costume.bitmapResolution - costume.pivot.x,
y: (rect.y + rect.height) / costume.bitmapResolution - costume.pivot.y
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inferPivotDelta: guard against zero bitmapResolution

If costume.bitmapResolution is 0 or undefined, the division rect.x / costume.bitmapResolution produces Infinity or NaN, which would then be applied to all costume pivots. A guard if (costume.bitmapResolution <= 0) return null before the arithmetic would close this gap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants