Skip to content

fix: correct stale animation state, mask suppression and editor scans - #239

Merged
Naruto merged 1 commit into
developfrom
fix/player-review-findings
Jul 26, 2026
Merged

Naruto merged 1 commit into
developfrom
fix/player-review-findings

Conversation

@Naruto

@Naruto Naruto commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

A review pass over ss_player/ turned up a set of independent defects. They are unrelated to each other but all small, so they land in one PR.

Memory safety

ss_internal_player.cpp _fetchAnimation() left _currentAnimationData pointing into the previous resource's FlatBuffer on all four of its failure paths. setSSABResource() drops the last Ref to the old resource before re-fetching, so a failed borrow/bind then left update() and _emit_effect_slot() dereferencing freed memory on the next tick. Cleared up front; the effect slot's fps() read is now null-safe.
ss_internal_player.cpp The passed-event index returned by the runtime went straight into events()->Get(). A not-found (-1), or an index belonging to a previously bound animation after a mid-tick switch, read past the mapped buffer. Bounds-checked against events()->size().

Masking

  • Pure-mask suppression was per batch, not per part. It tested only draw_order[batch->start_rank()] and then skipped the whole batch. Normal/Shape/Mesh batches group parts by texture + blend, so a draw_as_mask part could either drag its entire batch out of the frame or — when it sat at rank 1..n — draw its own colour over the scene. Now tested per part inside the emit loops; the single-part kinds (Instance/Effect) keep a batch-level check.
  • Suppression was gated on _mask_coverage_valid, which is never true for instance children (the parent owns the coverage pass, _render_mask_coverage is gated on !_parent_driven) nor on the first frame after a coverage target is acquired. Mask art was visible in both cases. It now depends only on the writer list, backed by a per-part lookup table so the check stays O(1) instead of walking _mask_writers.
  • _mask_local_to_uv was one frame ahead of the texture it describes. It was rebuilt from the current frame's writer bbox, but with UPDATE_ONCE the coverage that maskable shaders sample is the previous frame's render, rasterized under the previous frame's canvas transform. A moving or resizing mask was therefore sampled through a bbox its texels were never drawn with, sliding it off the target. The transform is now held back one frame so it matches the texture being read — including on a size-class transition, where _mask_prev (also last frame's render) is what gets sampled.

Editor

  • ss_resource_inspector.cpp called EditorInterface::get_resource_filesystem() unguarded. That does not compile as a custom module against Godot 4.4/4.5, which this file otherwise supports via its own VERSION_MINOR include branches. Now uses the same #if / get_resource_file_system() fallback as ss_importer.cpp:561 and ss_import_dock.cpp:698, plus a null check.
  • SSImportControl::stop_intercepting() returned early when get_window() was null, leaving every editor files_dropped handler it had disconnected permanently disconnected and is_intercepting stuck true. The intercept state is now always cleared.
  • Dropping a folder on the dock called importer->queue_scan_and_import() with no null check, while the guard above it and the sibling _start_import path are both null-tolerant.
  • _ensure_output_dir_exists() registered a freshly created directory with scan_changes() / scan_sources(). Those are mtime-driven and do not reliably notice a brand-new folder (parent-mtime granularity on Windows), so it would not appear in the FileSystem dock until an unrelated rescan. Now a full scan(), matching what SSImporter::_poll_fs_sync already does for the same reason.
  • The generated AnimationLibrary keyed frame to total_frame at t=length — one past the last valid frame (the player exposes 0..total-1). The last valid frame is now keyed at its own start time, so the interpolated rate is exactly one frame per 1/fps and the value holds until the clip ends.

Player API

  • get_part_transform() ignored the flip/offset matrix the player applies to its internal root canvas item (_root_ci is not part of the Node2D transform), so a SpriteStudioPartAttachment2D detached the moment flip_h or offset was set. It is now composed in, and the EN/JA API docs say so. This is a behaviour change to a public bound method.

Performance

  • _emit_normal_batch's flush_default_run() trimmed only indices. The vertex/uv/color/custom0 accumulators are peak-retained (grown, never shrunk), so a 4-vertex run after a 2000-vertex batch uploaded 2000 vertices plus 8000 custom0 floats, and Godot derived the surface AABB from the stale positions — culling computed against geometry that is not drawn. They are now trimmed to the run and restored afterwards.
  • SSABResource::is_valid() ran a full flatbuffers::Verifier over the whole buffer, and nearly every accessor gates on it (get_animation_names, get_cellmap_names, find_animation_by_hash, …). Selecting a player in the inspector alone triggered several full verifications of a multi-megabyte SSAB, and each Instance child adds one per lookup. The verdict is cached and reset wherever binary is replaced (load_from_file / copy_from).

Not fixed

  • ss_mask_write.gdshader additive blending. blend_add only implements OR while no texel receives the same bit twice. That holds across writers (their bits are disjoint) and for the quad/pentagon and triangulated-shape geometry we emit, but a single writer whose own triangles overlap (a bone-deformed mesh folded onto itself) adds its bit twice, which carries into the neighbouring bit — those texels then read as a different writer's mask. Godot's canvas render modes offer no saturating (max) blend, so a correct fix needs a stencil-style write, not a shader change. Documented as a limitation in the shader header rather than re-architected here.

Verification

./scripts/build-extension.sh (macOS arm64, target=editor) builds clean — no errors or warnings from these files. The custom-module build was not compiled here (no engine checkout); the ss_resource_inspector.cpp guard mirrors the two existing call sites exactly. Runtime/visual behaviour of the three mask changes has not been verified on a device.

A review pass over ss_player/ turned up a set of independent defects. They
are unrelated to each other but all small, so they land together.

Memory safety

- _fetchAnimation() left _currentAnimationData pointing into the previous
  resource's FlatBuffer on all four of its failure paths. setSSABResource()
  drops the last Ref to the old resource before re-fetching, so a failed
  borrow/bind then left update() and _emit_effect_slot() dereferencing freed
  memory on the next tick. Clear it up front instead, and make the effect
  slot's fps read null-safe.
- The passed-event index returned by the runtime was fed straight into
  events()->Get(). A not-found (-1) or an index belonging to a previously
  bound animation read past the mapped buffer; bounds-check it.

Masking

- Pure-mask suppression tested only the first part of a batch and then
  skipped the whole batch. Normal/Shape/Mesh batches group parts by texture
  and blend, so a draw_as_mask part could either drag its whole batch out of
  the frame or, at rank 1..n, draw its own colour over the scene. Test per
  part inside the emit loops instead; the single-part kinds keep a
  batch-level check.
- That test was also gated on _mask_coverage_valid, which is never true for
  instance children (the parent owns the coverage pass) nor on the first
  frame after a coverage target is acquired. Mask art was therefore visible
  in both cases. Suppression now depends only on the writer list, backed by
  a per-part lookup table so it stays O(1).
- _mask_local_to_uv was rebuilt from the current frame's writer bbox, but
  the coverage texture maskable shaders sample is the previous frame's
  render (UPDATE_ONCE latency). A moving or resizing mask was therefore
  sampled through a bbox its texels were never rasterized with. Hold the
  transform back one frame so it matches the texture being read.

Editor

- ss_resource_inspector.cpp called EditorInterface::get_resource_filesystem()
  unguarded, which does not compile as a custom module against Godot 4.4/4.5.
  Use the same #if / get_resource_file_system() fallback as the other two
  call sites.
- SSImportControl::stop_intercepting() returned early when get_window() was
  null, leaving the editor's own files_dropped handlers disconnected and
  is_intercepting stuck true. Always clear the intercept state.
- Dropping a folder called importer->queue_scan_and_import() without a null
  check, unlike every sibling path.
- _ensure_output_dir_exists() registered a freshly created directory with
  scan_changes()/scan_sources(), which are mtime-driven and miss new folders
  on Windows. Use a full scan(), as the importer's own sync phase does.
- The generated AnimationLibrary keyed `frame` to total_frame at t=length,
  one past the last valid frame. Key the last valid frame at its own start
  time so the interpolated rate is exactly one frame per 1/fps.

Player API

- get_part_transform() ignored the flip/offset matrix that the player
  applies to its internal root canvas item, so a SpriteStudioPartAttachment2D
  detached as soon as flip_h or offset was set. Compose it, and say so in
  the API docs.

Performance

- _emit_normal_batch's flush_default_run() trimmed only the index array; the
  vertex/uv/color/custom0 accumulators are peak-retained, so every small run
  uploaded the largest batch's vertex count and derived its surface AABB
  from stale positions. Trim them to the run and restore afterwards.
- SSABResource::is_valid() ran a full FlatBuffers verification of the whole
  buffer, and nearly every accessor gates on it. Cache the verdict and reset
  it wherever the buffer is replaced.

Also documents, in ss_mask_write.gdshader, that additive blending only
implements OR while no texel receives the same bit twice — a single writer
whose own triangles overlap corrupts the neighbouring bit. Godot's canvas
render modes offer no saturating blend, so fixing that needs a
stencil-style write rather than a shader change.
@MasamiYitsuse
MasamiYitsuse self-requested a review July 26, 2026 09:29
@MasamiYitsuse

Copy link
Copy Markdown
Collaborator

Reviewed the three masking changes, and ran them on a purpose-built harness (custom-module build, Godot 4.7, Windows x86_64). Short version: items 1 and 2 are confirmed real fixes, and item 3 does exactly what it says — with one side effect we will pick up separately. Nothing here blocks a merge.

1. Per-part pure-mask suppression — confirmed, and the bug is worse than the description

Traced this through the runtime's batch builder (build_draw_order_and_batches in libs/ssruntime/src/core/framedata.rs). A draw_as_mask part takes the needs_mask_twin path, which finalizes the in-progress batch, emits the 1-part Mask twin, and then falls through and starts a new batch of its own. Normal / Shape / Mesh are all batchable, so the following parts with the same texture + blend + flags join the batch that the mask part started.

So the old batch-level continue did not just drop the mask part — it dropped every innocent part that had been batched behind it. Concretely: a shape flagged "use as mask", followed in z-order by ordinary shapes of the same blend, made those shapes disappear.

One correction to the PR description: the "or — when it sat at rank 1..n — draw its own colour" case is not reachable, because a mask contributor always ends up at start_rank of its batch. The fix is correct either way.

Also worth noting for whoever reads this later: the checks added in _emit_normal_batch and _emit_mesh_batch are dead code today. The converter only sets draw_as_mask for Shape / Text / Nines (animedata.rs), and Text / Nines have no draw branch yet, so only the Shape path is live. Fine as future-proofing.

2. Dropping the _mask_coverage_valid gate — agreed, with a behaviour change worth calling out

_build_mask_writers() does run for instance children — only _render_mask_coverage() is gated on !_parent_driven — so the writer list is populated there and the per-part suppression now applies to them.

The visible consequence: an instance child's own mask parts become invisible, while still not clipping anything (the parent owns the coverage pass, and its writer list only covers the parent's parts). That is closer to the intent than drawing the mask art, but it does change how existing content looks, so it is a behaviour change and not purely a fix.

3. Coverage-UV hold-back — correct for the current architecture, and it has a measurable cost

To judge this I built a harness that pairs each mask with a translucent child part sharing its cell and transform, so "where the mask actually is this frame" and "where the mask is being sampled from" are both visible in the same image.

Measured on this branch:

  • E_TwoMasks (one static mask, one moving mask) — the static mask's hole stays registered with it. This is the case the change fixes.
  • D_Translate (single mask, 20 px/frame) — the punched hole trails the mask by ~20 px, i.e. exactly one frame.

The lag is the expected consequence rather than a defect: sampling becomes geometrically exact, but the coverage being sampled is still one frame old. The current behaviour happens to cancel that lag whenever the bbox moves together with the mask — single writer, rigid translation — which is why it looks right in the most common case and goes wrong as soon as a second writer moves, or the mask rotates or scales non-uniformly.

To be explicit about what is and is not evidence here: the pre-PR build was not measured. The distortion attributed to the current behaviour above is reasoned from the code, not observed.

Root cause of the latency itself

For the follow-up, the one-frame latency looks addressable at its source. The coverage viewport is created with viewport_create() + viewport_set_active(true) and never gets viewport_set_parent_viewport(). Godot orders active viewports children-first through viewport->parent (RendererViewport::_sort_active_viewports), so a parent-less coverage viewport is drawn after the main viewport — which is the delay. Parenting it to the player's viewport should make it render in the same frame, removing both the misregistration and the lag, and making the fresh / _mask_prev size-class machinery unnecessary along with it.

Two implementation notes for that work: viewport_set_parent_viewport() does not set sorted_active_viewports_dirty (only viewport_set_active() does), so the parent has to be assigned before activating; and since SsMaskCoveragePool shares targets between players, the parent has to be re-assigned on each acquire.

This has not been prototyped yet — it is read off the engine source, not tested.

Why this does not hold up the merge

The follow-up and item 3 are mutually exclusive: once the coverage is same-frame, the UV has to be published immediately again and the _mask_local_to_uv_pending hold-back has to come back out. Leaving it in at that point would produce a one-frame error in the opposite direction, which would be harder to spot than the current one.

Given the architecture as it stands today, item 3 is the correct behaviour, so it should go in as written. We will track the latency work and the removal of the hold-back on our side.

Not blocking

The additive-OR limitation note is accurate. One more source of a double-write is worth adding to that comment: a draw_as_mask part appears in both its Mask twin batch and its Normal / Shape / Mesh batch, and _render_mask_coverage() walks both kinds and matches the part against _mask_writers each time, so its bit can be rasterized twice. It appears harmless today — I did not confirm what the twin batch actually rasterizes for a Shape part — but it would become a genuine same-bit double-add once Text / Nines get a draw path.

@Naruto
Naruto merged commit a1c9eb0 into develop Jul 26, 2026
2 of 4 checks passed
@Naruto
Naruto deleted the fix/player-review-findings branch July 26, 2026 13:04
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