perf(spx): optimize Web pen rendering

[joeykchen]

No description provided.

[fennoai]

Review summary

Solid refactor. Moving pen rasterization off the CPU Image/set_pixel path onto a persistent transparent SubViewport with CLEAR_MODE_NEVER + UPDATE_ONCE is the correct Godot idiom for an accumulating GPU canvas, and the command-batching in SpxPenCanvas keeps draw-call count low. The pending_commands.clear()-per-draw design correctly bounds CPU-side command growth (pixels persist in the render target, not the command vector), and the JS glue fix (reading globalThis.__spxDirectCallbackHandlerSlots at call time with a slots && guard) is correct and null-safe.

No blocking bugs and no security issues were found. The notable items are one perf win, one perf scaling concern, a comment-accuracy nit, and a minor lifecycle hazard — all left as inline comments. A few additional non-blocking notes below.

Additional non-blocking notes

• modules/spx/spx_pen.cpp:47 — surface = static_cast<SpxPenSurface *>(root) is an unchecked downcast. It is safe today because SpxPenMgr::on_awake is the only creator and always sets root to an SpxPenSurface, but a defensive Object::cast_to<SpxPenSurface>(root) would be more robust if the base _create_root path is ever reused.
• modules/spx/spx_pen.cpp:125 / modules/spx/spx_pen_mgr.cpp:84 — on_erase_all() now only resets per-pen state; the actual canvas clear is deferred to SpxPenSurface::clear() and only takes effect on the next flush_all(). This is fine in the normal loop (SpxEngine calls pen->flush_all() every frame), but it is a behavioral change from the old synchronous clear — worth confirming no caller relies on destroy_all_pens() clearing synchronously outside the update loop.
• modules/spx/spx_pen_mgr.cpp:88 — destroy_all_pens() calls the mutating pen->on_erase_all() under an RWLockRead. This matches the existing _update_all convention, so it is consistent, but the SpxObjectMgr docstring claims "All public methods are thread-safe," which a shared lock over per-element mutation does not strictly satisfy.

[fennoai]

The per-batch indices / vertices / colors vectors are built with push_back and no reserve(), in the hot per-flush path. Each line segment deterministically produces ~30 vertices (4 + 2*(1 + CAP_SEGMENTS)) and ~78 indices, so a batch of N segments triggers many reallocations. Reserving up front is an easy win:

const int count = p_end - p_begin;
vertices.reserve(count * 30);
colors.reserve(count * 30);
indices.reserve(count * 78);

(Upper bound — the zero-length-segment path uses fewer, which is fine for reserve.)

[fennoai]

Every line segment appends a full CAP_SEGMENTS-poly disc at both from and to. For a continuous pen stroke (the typical Scratch usage: one add_line per step/mouse move), every interior join point gets a disc from both the incoming and outgoing segment — roughly 2x redundant cap geometry at each interior vertex, and it dominates the triangle array for long strokes. Draw-call count is unaffected (good), but CPU array-build and GPU vertex/fill cost scale at ~30 verts/segment. Consider only emitting the start cap for the first segment and the end cap for the last within a contiguous stroke, and/or scaling CAP_SEGMENTS down for sub-pixel radii (radius clamps to 0.5 at line 60, yet still emits 12-segment caps). Non-blocking, but it is the largest scaling factor in this file.

[fennoai]

Comment accuracy: "the complete frame is sent as one colored triangle array" overstates it. _notification() (lines ~100-121) splits line runs into a separate _draw_line_batch() — and thus a separate canvas_item_add_triangle_array() call — whenever STAMP commands are interspersed between lines. The load-bearing point (cap segment count grows vertices, not draw calls) is correct; suggest rewording to e.g. "each contiguous run of line commands is sent as a single triangle array."

[fennoai]

_destroy_all() (in spx_object_mgr.h) already calls root->queue_free(), and root and surface point to the same object. So between _destroy_all() returning and this surface = nullptr, surface is a pointer to a node already scheduled for deferred deletion. Nothing dereferences it in this window today, but it is a latent dangling-pointer hazard — consider nulling surface before/as part of the destroy, or reordering so surface is cleared before _destroy_all().