fix(capi): render cube node shape with 3D depth (issue #49) - #50
Conversation
Testing feedback — cubes not visually distinguishable from flatTested locally in Dasher-Apple (macOS + iOS). After a clean build with this PR, Cube mode shows a faint lighter bar at the top of some nodes but does not read as 3D/cube. Here's why and how to fix it. Root cause: rendering order buries the 3D shadingDasher renders nodes depth-first (parent → children). Each child's flat front-face rectangle () paints directly over the parent's shaded top/right faces. By the time the frame completes, only the outermost leaf nodes retain any visible shading — everything else is buried under child rectangles. The extrusion strips also fall in the wrong place:
Dasher's node layout is simply too dense and overlapping for 2D extrusion shading to be visible. The original cube mode worked on GTK/Windows because those frontends had real 3D rendering pipelines (depth buffers, perspective projection, z-ordering). The C API's flat command buffer (painter's algorithm only) cannot replicate this. Proposed fix: new opcode + two-pass frontend renderingAdd one new opcode so the depth data survives the command buffer intact:
The cube overlay is a visual enhancement layered over the flat layout, not a replacement. Each frontend implements pass 2 with its own graphics API (CGContext, Metal, Skia, Canvas, etc.). Work estimate
AlternativeIf the opcode approach is too much for now, revert to the #48 single-rect version (functional but flat) and relabel Cube in settings as requiring a 3D-capable frontend. The flat command buffer fundamentally cannot produce visible 3D cubes with painter's algorithm alone. |
200d969 to
b7e518d
Compare
|
Thanks for the test report — you're right, the shaded-rectangle emulation can't work under painter's algorithm here. I confirmed the root cause you identified: children paint over parent shading, and the extrusion strips fall past the zoom opening / under the next node up, so only leaves retain any shading. Revised the PR to Option B (your proposed fix): a dedicated opcode that carries the raw cube data through the buffer for a frontend two-pass overlay. What changed
Format impactOpcode 7 is the only variable-length command (9 ints vs 6). Parsers must read the opcode first and advance 9 for opcodes 7, 6 otherwise. This is documented in TestsThe cube suite now parses with the 9-int stride and checks:
One thing to confirmI went with the 9-int layout exactly as you sketched, which breaks strict 6-int divisibility (a parser that doesn't know opcode 7 will desync). The alternative is a 12-int (two-slot) encoding so unaware frontends stay in sync and just skip unknown records. I stuck with 9 because (a) it's your spec and (b) any frontend opting into cube mode already needs pass-2 work, so handling the 9-int read is part of the same update. Happy to switch to the 12-int two-slot form if you'd prefer graceful degradation for frontends that haven't updated yet. Build/test/format are green locally; CI re-running on the force-push. |
#49) Cube mode (LP_SHAPE_TYPE == 6) reached the C API in #48 but looked identical to flat rectangles: DrawCube discarded the CubeDepthLevel and emitted one opcode-4 rect per node. A shaded-rectangle emulation does not work either — Dasher renders depth-first, so each child's flat front face paints over the parent's shaded faces, leaving only leaf nodes with any visible 3D. The flat command buffer (painter's algorithm only, no depth buffer) cannot replicate GTK/Windows cube mode. Fixes issue #49 (revised per review). Fix (Option B from the issue): DrawCube emits a dedicated opcode-7 record carrying the raw cube data so the depth info survives the buffer intact. Frontends render cube mode in two passes: (1) opcodes 0-6 as the flat layout, (2) opcode-7 cubes composited as a 3D overlay on top via the frontend's own graphics API. Encoding: opcode 7 occupies TWO 6-int slots (12 ints), both prefixed with 7 — slot 1: [7, x1, y1, x2, y2, extrusionLevel], slot 2: [7, fillARGB, outlineARGB, thickness, 0, 0]. This keeps the buffer uniformly 6-int divisible: a parser advancing 6 ints per slot never desyncs, and frontends that don't know opcode 7 just see two unknown slots per cube and skip them (graceful degradation). Cost is 3 unused ints per cube — negligible (~50-200 visible nodes in a >=10K-int buffer). Docs: dasher.h and docs/C_API.md document opcode 7, the two-slot layout, the two-pass rendering model, and a parse example. Draw3DLabel still emits opcode 5 and DrawProjectedRectangle opcode 4 (crosshair bar), so the flat-layout pass already has labels + crosshair. Tests: cube nodes arrive as opcode 7 (not opcode 4); labels as opcode 5. Added cases verifying (a) the buffer stays 6-int divisible (cc % 6 == 0) and (b) the per-node extrusionLevel varies across the tree (>=2 distinct values) — i.e. the depth data is no longer discarded. The parse helper advances 12 ints for opcode 7. Signed-off-by: will wade <willwade@gmail.com>
b7e518d to
0096a5c
Compare
|
Done — switched to the 12-int two-slot encoding. Good call. Layout (opcode 7 = two 6-int slots, 12 ints): Both slots prefixed with The parse example in Tests updated:
All cube/draw-snapshot/draw-command tests green locally; format clean; CI re-running on the force-push. |
|
Withdrawing this PR (opcode 7) per the direction in governance RFC 0013 (dasher-project/governance#20). The two-pass shaded-rectangle overlay is visually unconvincing — the flat command buffer's painter's algorithm can't composite depth correctly, so Dasher's depth-first rendering buries parent shading under child rectangles. Rather than carry a variable-length opcode for a limited result, cube mode over the C API stays at the #48 baseline (flat rectangles — functional, not black) while the proper fix — exposing the node tree via a Strand 2 `dasher_get_visible_nodes` API — is implemented separately so frontends can render real 3D cubes with their own graphics pipelines. No code change needed on `main`: opcode 7 never landed (this PR was unmerged), so cube mode remains at the #48 state. Closing without merging; branch deleted. |
* feat(capi): Strand 2 custom-rendering API (RFC 0013) Adds the second rendering strand from governance RFC 0013: a frontend can query the visible node tree each frame and render it itself (3D cubes, VR/spatial, custom visualisations, accessibility) instead of consuming the flat int[] command buffer. Strands coexist — a frontend can use the command buffer for most rendering and drop into Strand 2 for specific features. New C API: dasher_set_visible_nodes_enabled (opt-in; off by default so Strand 1 frontends pay zero overhead), dasher_get_visible_nodes (depth-first visible node tree with pre-computed screen bounds, colours, labels, depth, game flag), dasher_get_viewport (crosshair + visible Y range + canvas size). dasher_node_info / dasher_viewport carry a caller-set struct_size field for ABI evolution. Capture happens inside CDasherViewSquare::NewRender/DisjointRender at each node actually drawn, so the captured set matches exactly what the command buffer draws for the same frame (Strand 1/Strand 2 parity). This is the proper fix for cube mode (issue #49) — frontends now render real 3D cubes from node data + their own graphics pipeline, replacing the withdrawn opcode-7 experiment (PR #50). Minor core additions: CDasherNode::IsSymbolNode() virtual (GetAlphSymbol() throws on the base class, so the C boundary gates on IsSymbolNode rather than using RTTI); CDasherView gains VisibleNode struct + SetVisibleNodeCapture/IsVisibleNodeCaptureEnabled/GetVisibleNodes virtuals. Tests (test_strand2_nodes.cpp): disabled-by-default + enable; depth-first preorder invariant; Strand 1/Strand 2 parity (every captured node bound matches an opcode-4 rect from the same frame); labels/symbols populated; viewport matches constants + canvas size; struct_size ABI guard rejects undersized caller structs; bounds are monotone. Signed-off-by: will wade <willwade@gmail.com> * docs(capi): document Strand 2 custom-rendering API (RFC 0013) Adds a Custom Rendering section to docs/C_API.md covering dasher_set_visible_nodes_enabled / dasher_get_visible_nodes / dasher_get_viewport, the dasher_node_info / dasher_viewport structs, the opt-in capture model, Strand 1/2 parity, and a render example. Signed-off-by: will wade <willwade@gmail.com> * fix(capi): guard dasher_get_visible_nodes against thrown exceptions (Rule 4) GetAlphSymbol() throws on the base CDasherNode class and the IsSymbolNode() fast-path guard is not a substitute for the C-boundary exception guard — a thrown exception crossing extern "C" aborts the process (review feedback on #51). Wrap the whole body of dasher_get_visible_nodes (and, for consistency, dasher_get_viewport and dasher_set_visible_nodes_enabled) in try { ... } catch (...) { return -1; } per CONTRIBUTING Rule 4. IsSymbolNode() is kept as the common no-throw fast path; the catch is the safety net. Test: add a long-driven-session regression that asserts dasher_get_visible_nodes always returns a sane value and never lets an exception escape — would SIGABRT without the guard. Signed-off-by: will wade <willwade@gmail.com> * fix(capi): resolve Strand 2 node data during Render (no dangling CDasherNode*) Root cause of the recurring crash (review feedback on #51): VisibleNode stored a raw CDasherNode* captured during Render() and dasher_get_visible_nodes dereferenced it after the frame returned. The model can free/mutate nodes between the capture and the query, so those pointers dangled — the IsSymbolNode/GetAlphSymbol try/catch only moved the crash one accessor along (getLabel/GetFlag on a freed node). Fix: make VisibleNode a fully self-contained snapshot. CaptureNode now resolves every value — symbol (via IsSymbolNode + GetAlphSymbol), has_children, is_game_node, fill/outline colours, label text (copied), depth, clipped screen bounds — while the node is guaranteed alive, during the render pass. The C API query just copies pre-resolved fields; no CDasherNode* is stored or dereferenced, so node lifetime between frame and query no longer matters. Verified under ASan + UBSan (all 9 strand2 tests clean, no heap-use-after-free). Added regression test: drive, then dasher_reset() (frees the node tree the snapshot came from), then query — must not crash. The boundary try/catch is retained as defense-in-depth. Signed-off-by: will wade <willwade@gmail.com> --------- Signed-off-by: will wade <willwade@gmail.com>
Problem
After #48, cube mode (
LP_SHAPE_TYPE == CUBE) rendered via the C API but looked identical to flat rectangles.CommandScreen::DrawCubereceivedCubeDepthLevel nodeDepth/parentDepth(the extrusion depth) and discarded both, emitting a single opcode-4 rect per node. The 3D extruded-cube appearance the mode is supposed to provide was missing.Fix (Option A from the issue)
Decompose each cube into three shaded faces using existing opcodes (no command-buffer format change):
Painter's order (right → top → front) makes each face's bounding rect resolve to the correct visible colour.
Depth-aware extrusion
The
CubeDepthLevelis now used instead of discarded:0.15 × min(w,h), clamped[1,16]), so large cubes pop more than tiny ones.FinishRender3Dnow stores the crosshair'sextrusionLevel(one-frame lag —DrawCuberuns beforeFinishRender3Dwithin a frame), and eachDrawCubedivides its offset by1 + 0.35·|nodeDepth.extrusionLevel − crosshairLevel|. Result: the focused node extrudes most, distant ones recede.Added a
shadeARGBhelper (lighten/darken a normalised ARGB int) next tocolorToARGB.FinishRender3Dstores the crosshair level; no other interface changes, no new opcodes.Test
New case in
test_cube_geometry.cpp: cube mode now emits ~3× the filled rects of flat rectangle mode for the same node tree (top+front+right faces). Measured: flat=210, cube=670 (~3.2×). Assertscube ≥ 2× flat, which fails on the single-rect code from #48 (~1×). Existing cube tests still pass.Verification
--Werrordry-run)dasher_draw_snapshot_tests(golden) +dasher_draw_command_testspass — default-shape rendering unchangedNotes / trade-offs