fix(spx): resize pen canvas with stage - #329
Conversation
Allow the SPX pen surface to follow logical stage dimensions, discarding pending draws and clearing pixels when the native size changes. Expose the resize operation through native and Web bridges.
There was a problem hiding this comment.
Review: runtime pen canvas resize (set_canvas_size)
Overall the change is clean and consistent with the sibling pen functions (destroy_all_pens, create_pen) across all five files — the C++ manager/surface methods, the gdextension wrapper, the web wrapper, and the JS binding all follow existing conventions, and the resize flush/dirty/clear path is correct. A few points worth addressing:
1. (High) No upper bound on canvas dimensions from untrusted JS input — OOM/DoS risk.
SpxPenSurface::set_canvas_size clamps only the lower bound (MAX(1, ...)) and passes the size straight to render_target->set_size(). SubViewport::set_size -> Viewport::_set_size (scene/main/viewport.cpp:1017) only clamps to a minimum of 2 and then calls viewport_set_size(width, height) — there is no upper clamp anywhere in the chain. width/height originate at the JS→wasm ABI (gdspx_pen_set_canvas_size) with no sanitization, so a request such as 65535 x 65535 triggers a multi-GB render-target allocation and can crash the tab/process. Recommend clamping both dimensions to a sane maximum, ideally via a shared helper reused by initialize().
2. (Medium) int64 → int32 truncation happens before clamping.
SpxPenMgr::set_canvas_size takes GdInt (int64) but constructs Size2i (int32), so the value is narrowed before MAX(1, ...) runs on the surface side. A large int64 can wrap into a valid-looking or negative int32 (e.g. 0x80000000 truncates negative, then MAX(1,...) rescues it to 1 — silently wrong). Validate/clamp on the int64 values in SpxPenMgr::set_canvas_size before narrowing. Fixing this together with finding #1 resolves both.
3. (Notable) gdspx.js is a generated file — edit will be lost on regeneration.
platform/web/js/engine/gdspx.js (header lines 1-8) states it is generated from gdspx.js.tmpl and "changes ... will be lost if the code is regenerated." The new gdspx_pen_set_canvas_size binding was added directly here. Unless the upstream template/spec is also updated, the next regeneration will silently drop this half of the PR. Please confirm the generator source is synced.
4. (Minor) Stored canvas_size can diverge from the real viewport size.
MAX(1, ...) permits a stored canvas_size of 1, but the viewport clamps to a minimum of 2, so a 1px request leaves canvas_size (1) out of sync with the actual render target (2). Since draw_line/draw_stamp derive the centering origin from canvas_size, this produces a half-pixel offset. Pre-existing in initialize() as well; clamping to MAX(2, ...) in both keeps them in sync (low impact).
Also worth a one-line comment: a resize intentionally clears all existing pen content (clear_requested = true + discard_pending()), which is non-obvious given initialize()/clear() are the only other clearing paths.
|
|
||
| canvas->discard_pending(); | ||
| canvas_size = next_size; | ||
| render_target->set_size(canvas_size); |
There was a problem hiding this comment.
Missing upper-bound clamp (High). next_size clamps only the lower bound; the value flows unchecked into render_target->set_size(). SubViewport::set_size/Viewport::_set_size (scene/main/viewport.cpp:1017) only enforce a minimum of 2 — no upper limit — so an unsanitized JS-boundary request (e.g. 65535 x 65535) triggers a multi-GB GPU allocation and can crash the tab (OOM/DoS). Clamp both dimensions to a sane maximum, ideally via a helper shared with initialize().
Minor: MAX(1, ...) lets the stored canvas_size reach 1 while the viewport clamps to 2, so canvas_size diverges from the real render-target size and the draw_line/draw_stamp centering origin is off by half a pixel. Clamping to MAX(2, ...) here and in initialize() keeps them in sync.
|
|
||
| void SpxPenMgr::set_canvas_size(GdInt width, GdInt height) { | ||
| if (surface != nullptr) { | ||
| surface->set_canvas_size(Size2i(width, height)); |
There was a problem hiding this comment.
int64 → int32 truncation before clamping (Medium). GdInt is int64 but Size2i is int32, so the value is narrowed here before the MAX(1, ...) guard runs in SpxPenSurface::set_canvas_size. A large int64 can wrap to a negative or misleading int32 (e.g. 0x80000000). Validate/clamp on the int64 width/height here before constructing Size2i — combine with the upper-bound clamp to resolve both this and the OOM risk.
| _gdFuncPtr(); | ||
|
|
||
| } | ||
| gdspx_pen_set_canvas_size(width_low,width_high,height_low,height_high) { |
There was a problem hiding this comment.
This file is generated from gdspx.js.tmpl (header, lines 1-8): "changes ... will be lost if the code is regenerated." This new binding was added directly to the generated output. Please update the upstream template/spec too, or the next regeneration will silently drop gdspx_pen_set_canvas_size.
Summary
SpxPenSurface.set_canvas_sizethrough the native GDExtension and Web bridges.Motivation
The pen surface was initialized from the viewport size, so stamps could be clipped when the logical SPX world size differed from the window size.
Validation
Dependency
This API is consumed by the corresponding SPX runtime change:
fix/pen-stage-canvas-size.