fix: correct stale animation state, mask suppression and editor scans - #239
Conversation
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.
|
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 descriptionTraced this through the runtime's batch builder ( So the old batch-level 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 Also worth noting for whoever reads this later: the checks added in 2. Dropping the
|
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_currentAnimationDatapointing into the previous resource's FlatBuffer on all four of its failure paths.setSSABResource()drops the lastRefto the old resource before re-fetching, so a failed borrow/bind then leftupdate()and_emit_effect_slot()dereferencing freed memory on the next tick. Cleared up front; the effect slot'sfps()read is now null-safe.ss_internal_player.cppevents()->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 againstevents()->size().Masking
draw_order[batch->start_rank()]and then skipped the whole batch. Normal/Shape/Mesh batches group parts by texture + blend, so adraw_as_maskpart 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._mask_coverage_valid, which is never true for instance children (the parent owns the coverage pass,_render_mask_coverageis 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_uvwas one frame ahead of the texture it describes. It was rebuilt from the current frame's writer bbox, but withUPDATE_ONCEthe 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.cppcalledEditorInterface::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 ownVERSION_MINORinclude branches. Now uses the same#if/get_resource_file_system()fallback asss_importer.cpp:561andss_import_dock.cpp:698, plus a null check.SSImportControl::stop_intercepting()returned early whenget_window()was null, leaving every editorfiles_droppedhandler it had disconnected permanently disconnected andis_interceptingstucktrue. The intercept state is now always cleared.importer->queue_scan_and_import()with no null check, while the guard above it and the sibling_start_importpath are both null-tolerant._ensure_output_dir_exists()registered a freshly created directory withscan_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 fullscan(), matching whatSSImporter::_poll_fs_syncalready does for the same reason.AnimationLibrarykeyedframetototal_frameatt=length— one past the last valid frame (the player exposes0..total-1). The last valid frame is now keyed at its own start time, so the interpolated rate is exactly one frame per1/fpsand 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_ciis not part of the Node2D transform), so aSpriteStudioPartAttachment2Ddetached the momentflip_horoffsetwas 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'sflush_default_run()trimmed onlyindices. 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 fullflatbuffers::Verifierover 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 whereverbinaryis replaced (load_from_file/copy_from).Not fixed
ss_mask_write.gdshaderadditive blending.blend_addonly 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); thess_resource_inspector.cppguard mirrors the two existing call sites exactly. Runtime/visual behaviour of the three mask changes has not been verified on a device.