Skip to content

refactor(spx): centralize coordinate conversion - #327

Merged
joeykchen merged 1 commit into
goplus:spx4.4.1from
joeykchen:refactor/spx-coordinate-system
Jul 31, 2026
Merged

refactor(spx): centralize coordinate conversion#327
joeykchen merged 1 commit into
goplus:spx4.4.1from
joeykchen:refactor/spx-coordinate-system

Conversation

@joeykchen

@joeykchen joeykchen commented Jul 30, 2026

Copy link
Copy Markdown

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review: refactor(spx): centralize coordinate conversion

Solid, well-scoped refactor. Centralizing the Y-axis flip into spx_to_godot_vec2 / godot_to_spx_vec2 and applying it at the manager API boundary removes a lot of error-prone scattered y = -y / flip_y() code, and the get/set pairs are now symmetric. The RenderRoot restructuring and the two new unit-test headers are good additions. The security pass found no memory-safety issues (null-guards, batch-buffer bounds checks, and reparent lifecycle all hold up).

One finding is worth confirming before merge because it is a semantic change hiding inside a "refactor", plus a few smaller items — see inline comments.

Summary of findings

  • Collider/trigger center & polygon-vertex Y-flip changed (see inline on spx_sprite_collision.cpp). This is the main item: several shape setters now flip the center Y (and collider-polygon vertices) where the base code did not. The old code was internally inconsistent, so the new unified behavior may well be the intended fix — but it is not a no-op and can vertically mirror collider/trigger offsets in existing SPX content. Please confirm it is intentional, document it, and ideally add a test asserting center placement per shape type.
  • set_render_offset re-positions RenderRoot on every batch update (inline on spx_sprite_render.cpp) — a change-guard would avoid redundant transform-dirty propagation (and the dynamic-frame-offset recompute) on the per-sprite batch hot path.
  • godot_to_spx_vec2 is an undocumented alias of spx_to_godot_vec2 (inline on spx_coordinate.h).
  • get_rect() null-render_root fallback silently returns a differently-origined rect (inline on spx_sprite_texture.cpp).

Additional (no inline location):

  • Naming inconsistency: SpxSprite renamed pivot_offset/set_pivotrender_offset/set_render_offset, but the sibling classes SpxRenderSprite/SpxStaticSprite (and spx_scene_mgr.cpp:225, the gdextension_spx_ext bindings) still use pivot/pivot_offset for the same concept. Functionally fine (separate code path), but a follow-up to unify the vocabulary — or a short comment noting the intentional divergence — would help readability.

Comment thread modules/spx/spx_sprite_collision.cpp
Comment thread modules/spx/spx_sprite_collision.cpp
Comment thread modules/spx/spx_sprite_render.cpp
Comment thread modules/spx/spx_coordinate.h
Comment thread modules/spx/spx_sprite_texture.cpp Outdated
@joeykchen
joeykchen force-pushed the refactor/spx-coordinate-system branch from c38baec to 51b6f38 Compare July 31, 2026 09:38
@joeykchen
joeykchen merged commit a9b0600 into goplus:spx4.4.1 Jul 31, 2026
18 checks passed
@joeykchen
joeykchen deleted the refactor/spx-coordinate-system branch August 6, 2026 09:27
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.

2 participants