From 2f683ad6f012e52e2ae790573aff56d42467fedd Mon Sep 17 00:00:00 2001 From: Aymeric Rabot Date: Tue, 22 Sep 2026 13:37:35 -0400 Subject: [PATCH] fix(viewer): honour node.visible on container kinds in 3D The sidebar eye on a building, level, zone or ceiling wrote a flag that only half the app read. Selection candidates, first-person collision, the 2D plan and every export already prune a hidden node; the 3D viewport did not, because these kinds ship custom renderers and none of them applied the flag to its root. Hiding a building left it drawn but unclickable; hiding a level left a floor you fell through in walkthrough; hiding a zone or ceiling left it on screen but out of the plan and the export. Each of those renderers now sets `visible={node.visible !== false}` the way ParametricNodeRenderer does. Two per-frame systems used to overwrite that prop and now fold the flag in instead: LevelSystem's solo decision moves to a pure `resolveLevelVisibility` in level-utils (a level the author hid is hidden outright and never becomes a shadow-caster), and both zone systems, which force the group visible to keep the label alive, exempt a hidden zone and drop its label with it. snapLevelsToTruePositions no longer un-hides those levels for thumbnail and bake captures, so the capture matches the export. The Site keeps the rule settled on the export side: its flag hides its own ground fill, sculpted terrain and boundary line, nothing beneath it, and the horizon disc is a world backdrop that renders either way. Co-Authored-By: Claude Fable 5.1 --- .../components/systems/zone/zone-system.tsx | 12 ++-- .../src/components/viewer-zone-system.tsx | 9 ++- packages/nodes/src/building/renderer.tsx | 1 + packages/nodes/src/ceiling/renderer.tsx | 1 + packages/nodes/src/level/renderer.tsx | 2 +- packages/nodes/src/site/renderer.tsx | 21 ++++-- packages/nodes/src/zone/renderer.tsx | 7 +- .../src/systems/level/level-system.test.ts | 42 +++++++++++ .../viewer/src/systems/level/level-system.tsx | 28 ++++---- .../src/systems/level/level-utils.test.ts | 69 +++++++++++++++++++ .../viewer/src/systems/level/level-utils.ts | 45 ++++++++++-- wiki/architecture/renderers.md | 8 +++ 12 files changed, 213 insertions(+), 32 deletions(-) create mode 100644 packages/viewer/src/systems/level/level-utils.test.ts diff --git a/packages/editor/src/components/systems/zone/zone-system.tsx b/packages/editor/src/components/systems/zone/zone-system.tsx index 8ee276dae5..f24eb12c1e 100644 --- a/packages/editor/src/components/systems/zone/zone-system.tsx +++ b/packages/editor/src/components/systems/zone/zone-system.tsx @@ -58,9 +58,13 @@ export const ZoneSystem = () => { const isDeleteHovered = editorMode === 'delete' && hoveredId === zoneId // Keep group visible (so labels stay active), hide/show meshes only. - // Show meshes when: in zone mode, selected, or delete-hovered. - if (!obj.visible) obj.visible = true - const meshVisible = !isCaptureMode && (zoneGeometryVisible || isSelected || isDeleteHovered) + // Show meshes when: in zone mode, selected, or delete-hovered. A zone the + // author hid (sidebar eye) is the one case where the group itself goes: + // otherwise this per-frame write would undo the renderer's `visible` prop. + const nodeVisible = zone?.visible !== false + if (obj.visible !== nodeVisible) obj.visible = nodeVisible + const meshVisible = + nodeVisible && !isCaptureMode && (zoneGeometryVisible || isSelected || isDeleteHovered) const targetOpacity = isCaptureMode ? 0 : isSelected || isDeleteHovered @@ -103,7 +107,7 @@ export const ZoneSystem = () => { // Labels: visible on the current level (regardless of mode), but never // during snapshot capture. const showLabel = - !isCaptureMode && !zoneLabelsHidden && !!selectedLevelId && isOnSelectedLevel + nodeVisible && !isCaptureMode && !zoneLabelsHidden && !!selectedLevelId && isOnSelectedLevel const labelOpacity = showLabel ? '1' : '0' const labelEl = document.getElementById(`${zoneId}-label`) if (labelEl && labelEl.style.opacity !== labelOpacity) { diff --git a/packages/editor/src/components/viewer-zone-system.tsx b/packages/editor/src/components/viewer-zone-system.tsx index fee75e7738..af5d4cf933 100644 --- a/packages/editor/src/components/viewer-zone-system.tsx +++ b/packages/editor/src/components/viewer-zone-system.tsx @@ -33,10 +33,14 @@ export const ViewerZoneSystem = () => { // Zone geometry: visible in zone mode on the right level, OR when this zone is selected. // The editor ZoneSystem handles the selected zone's opacity animation. const isSelected = id === zoneId + // A zone the author hid (sidebar eye) takes the group with it — this + // per-frame write would otherwise undo the renderer's `visible` prop. + const nodeVisible = zone.visible !== false const shouldShowGeometry = + nodeVisible && !isCaptureMode && ((structureLayer === 'zones' && !!levelId && isOnSelectedLevel) || isSelected) - if (!obj.visible) obj.visible = true + if (obj.visible !== nodeVisible) obj.visible = nodeVisible obj.traverse((child) => { if ((child as Mesh).isMesh) { child.visible = shouldShowGeometry @@ -44,7 +48,8 @@ export const ViewerZoneSystem = () => { }) // Labels: always visible on the current level (regardless of mode or zone selection) - const showLabel = !isCaptureMode && !zoneLabelsHidden && !!levelId && isOnSelectedLevel + const showLabel = + nodeVisible && !isCaptureMode && !zoneLabelsHidden && !!levelId && isOnSelectedLevel const targetOpacity = showLabel ? '1' : '0' const labelEl = document.getElementById(`${id}-label`) if (labelEl && labelEl.style.opacity !== targetOpacity) { diff --git a/packages/nodes/src/building/renderer.tsx b/packages/nodes/src/building/renderer.tsx index 6bc40a9049..17497bd553 100644 --- a/packages/nodes/src/building/renderer.tsx +++ b/packages/nodes/src/building/renderer.tsx @@ -16,6 +16,7 @@ export const BuildingRenderer = ({ node }: { node: BuildingNode }) => { position={node.position} ref={ref} rotation={[node.rotation[0], node.rotation[1], node.rotation[2]]} + visible={node.visible !== false} {...handlers} > {(node.children ?? []).map((childId) => ( diff --git a/packages/nodes/src/ceiling/renderer.tsx b/packages/nodes/src/ceiling/renderer.tsx index d82a1c03f3..97c0372e16 100644 --- a/packages/nodes/src/ceiling/renderer.tsx +++ b/packages/nodes/src/ceiling/renderer.tsx @@ -118,6 +118,7 @@ export const CeilingRenderer = ({ node }: { node: CeilingNode }) => { material={materials.bottomMaterial} position={position} ref={ref} + visible={node.visible !== false} {...handlers} > { const handlers = useNodeEvents(node, 'level') return ( - + {node.children.map((childId) => ( ))} diff --git a/packages/nodes/src/site/renderer.tsx b/packages/nodes/src/site/renderer.tsx index bc58850e4d..78e2f928ab 100644 --- a/packages/nodes/src/site/renderer.tsx +++ b/packages/nodes/src/site/renderer.tsx @@ -383,6 +383,13 @@ export const SiteRenderer = ({ node }: { node: SiteNode }) => { // terrain still mounts the mesh. const showTerrain = terrainGrid !== null + // The Site is the one kind whose `visible` flag stops at itself: hiding it + // drops the parcel's own presentation — ground fill, sculpted ground, lot + // line — while everything standing on the site keeps its own flag (the + // exporter and the 2D plan draw the same line). The horizon disc is a world + // backdrop rather than part of the parcel, so it stays either way. + const showSiteSurfaces = node.visible !== false + if (!(node && lineGeometry)) { return null } @@ -395,10 +402,10 @@ export const SiteRenderer = ({ node }: { node: SiteNode }) => { ))} {/* Sculpted ground, when the site has terrain */} - {showTerrain && } + {showSiteSurfaces && showTerrain && } {/* Ground fill: site polygon with slab holes, occludes below-grade geometry */} - {groundGeometry && !showTerrain && ( + {showSiteSurfaces && groundGeometry && !showTerrain && ( { )} {/* Simple boundary line */} - {/* @ts-ignore */} - - - + {showSiteSurfaces && ( + // @ts-expect-error + + + + )} ) } diff --git a/packages/nodes/src/zone/renderer.tsx b/packages/nodes/src/zone/renderer.tsx index 5d239eadde..33c695aa98 100644 --- a/packages/nodes/src/zone/renderer.tsx +++ b/packages/nodes/src/zone/renderer.tsx @@ -226,7 +226,12 @@ export const ZoneRenderer = ({ node }: { node: ZoneNode }) => { } return ( - + {showZones && ( <> { expect(objects[0]!.visible).toBe(false) expect(objects[1]!.visible).toBe(true) }) + + test('hides a level the author hid, outside solo too', async () => { + const { levels, objects } = setupLevels([0, 1]) + hideLevel(levels[0]!.id) + setLevelMode('stacked') + + await updateLevelPresentation(1 / 12) + + expect(objects[0]!.visible).toBe(false) + expect(objects[1]!.visible).toBe(true) + }) + + test('keeps a level the author hid out of the solo shadow-caster branch', async () => { + const { levels, objects } = setupLevels([0, 1]) + hideLevel(levels[1]!.id) + setLevelMode('solo', levels[0]!.id) + + await updateLevelPresentation(1 / 12) + + expect(objects[0]!.visible).toBe(true) + expect(objects[1]!.visible).toBe(false) + }) }) describe('snapLevelsToTruePositions', () => { @@ -127,4 +155,18 @@ describe('snapLevelsToTruePositions', () => { expect(objects.map((object) => object.position.y)).toEqual([10, 20]) expect(objects.map((object) => object.visible)).toEqual([false, true]) }) + + test('leaves a level the author hid hidden, matching the export', () => { + const { levels, objects } = setupLevels([0.5, 1.25]) + hideLevel(levels[0]!.id) + objects[0]!.visible = true + + const restore = snapLevelsToTruePositions() + + expect(objects.map((object) => object.visible)).toEqual([false, true]) + + restore() + + expect(objects.map((object) => object.visible)).toEqual([true, true]) + }) }) diff --git a/packages/viewer/src/systems/level/level-system.tsx b/packages/viewer/src/systems/level/level-system.tsx index 285217cafd..1bbe95f328 100644 --- a/packages/viewer/src/systems/level/level-system.tsx +++ b/packages/viewer/src/systems/level/level-system.tsx @@ -4,7 +4,7 @@ import type { Object3D } from 'three' import { lerp } from 'three/src/math/MathUtils.js' import { applyShadowOnly, clearShadowOnly } from '../../lib/shadow-only' import useViewer from '../../store/use-viewer' -import { EXPLODED_GAP } from './level-utils' +import { EXPLODED_GAP, resolveLevelVisibility } from './level-utils' // Levels currently in shadow-caster-only mode (solo hides them from the color // passes but keeps their sun shadows). Tracked so we can restore layer masks @@ -54,22 +54,22 @@ export const LevelSystem = () => { // feel at 60 fps, exact snap instead of overshoot on slow frames. obj.position.y = lerp(obj.position.y, targetY, Math.min(1, delta * 12)) - // Solo: hidden levels ABOVE the soloed one stay in the shadow map - // (shadow-caster-only) so the sun still shadows the soloed floor through - // them; levels below can't block the sun, so they plain-hide. - const hidden = levelMode === 'solo' && Boolean(selectedLevel) && level?.id !== selectedLevel - const castsWhileHidden = hidden && selectedIndex !== undefined && index > selectedIndex - if (castsWhileHidden) { + const { visible, shadowOnly } = resolveLevelVisibility({ + levelMode, + hasSelectedLevel: Boolean(selectedLevel), + isSelected: level?.id === selectedLevel, + index, + selectedIndex, + nodeVisible: level?.visible !== false, + }) + if (shadowOnly) { applyShadowOnly(obj) shadowOnlyLevels.add(obj) - obj.visible = true - } else { - if (shadowOnlyLevels.has(obj)) { - clearShadowOnly(obj) - shadowOnlyLevels.delete(obj) - } - obj.visible = !hidden + } else if (shadowOnlyLevels.has(obj)) { + clearShadowOnly(obj) + shadowOnlyLevels.delete(obj) } + obj.visible = visible } }, 5) // Using a lower priority so it runs after transforms from other systems have settled return null diff --git a/packages/viewer/src/systems/level/level-utils.test.ts b/packages/viewer/src/systems/level/level-utils.test.ts new file mode 100644 index 0000000000..4248a82390 --- /dev/null +++ b/packages/viewer/src/systems/level/level-utils.test.ts @@ -0,0 +1,69 @@ +import { describe, expect, test } from 'bun:test' +import { resolveLevelVisibility } from './level-utils' + +const decide = ( + overrides: Partial[0]> = {}, +): ReturnType => + resolveLevelVisibility({ + levelMode: 'stacked', + hasSelectedLevel: false, + isSelected: false, + index: 0, + selectedIndex: undefined, + nodeVisible: true, + ...overrides, + }) + +describe('resolveLevelVisibility', () => { + test('shows every level outside solo mode', () => { + expect(decide()).toEqual({ visible: true, shadowOnly: false }) + expect(decide({ levelMode: 'exploded', index: 2 })).toEqual({ + visible: true, + shadowOnly: false, + }) + }) + + test('solo keeps the soloed level, plain-hides the ones below it', () => { + expect( + decide({ levelMode: 'solo', hasSelectedLevel: true, isSelected: true, index: 1 }), + ).toEqual({ visible: true, shadowOnly: false }) + expect( + decide({ levelMode: 'solo', hasSelectedLevel: true, index: 0, selectedIndex: 1 }), + ).toEqual({ visible: false, shadowOnly: false }) + }) + + test('solo keeps the levels above the soloed one as shadow casters', () => { + expect( + decide({ levelMode: 'solo', hasSelectedLevel: true, index: 2, selectedIndex: 1 }), + ).toEqual({ visible: true, shadowOnly: true }) + }) + + test('solo plain-hides everything when the selected level is not registered', () => { + expect( + decide({ levelMode: 'solo', hasSelectedLevel: true, index: 2, selectedIndex: undefined }), + ).toEqual({ visible: false, shadowOnly: false }) + }) + + test('a level the author hid is hidden outright, never a shadow caster', () => { + expect(decide({ nodeVisible: false })).toEqual({ visible: false, shadowOnly: false }) + expect( + decide({ + levelMode: 'solo', + hasSelectedLevel: true, + index: 2, + selectedIndex: 1, + nodeVisible: false, + }), + ).toEqual({ visible: false, shadowOnly: false }) + expect( + decide({ + levelMode: 'solo', + hasSelectedLevel: true, + isSelected: true, + index: 1, + selectedIndex: 1, + nodeVisible: false, + }), + ).toEqual({ visible: false, shadowOnly: false }) + }) +}) diff --git a/packages/viewer/src/systems/level/level-utils.ts b/packages/viewer/src/systems/level/level-utils.ts index 090d0858e0..bfc40a0a6f 100644 --- a/packages/viewer/src/systems/level/level-utils.ts +++ b/packages/viewer/src/systems/level/level-utils.ts @@ -20,9 +20,44 @@ export function getLevelPresentationY( return baseY + explodedExtra } +/** + * Whether a level renders this frame, and whether it renders as a + * shadow-caster only. + * + * Two unrelated things hide a level and they do not compose: the author's own + * `visible` flag (the sidebar eye), which hides the floor outright, and solo + * mode, which hides every level but the soloed one — keeping the levels ABOVE + * it in the shadow map so the sun still shadows the soloed floor through them. + * A level the author hid never enters that shadow-caster branch: its shadows + * on the floor below would be exactly what hiding it was meant to remove. + */ +export function resolveLevelVisibility({ + levelMode, + hasSelectedLevel, + isSelected, + index, + selectedIndex, + nodeVisible, +}: { + levelMode: 'stacked' | 'exploded' | 'solo' | 'manual' + hasSelectedLevel: boolean + isSelected: boolean + index: number + selectedIndex: number | undefined + nodeVisible: boolean +}): { visible: boolean; shadowOnly: boolean } { + if (!nodeVisible) return { visible: false, shadowOnly: false } + + const hidden = levelMode === 'solo' && hasSelectedLevel && !isSelected + const shadowOnly = hidden && selectedIndex !== undefined && index > selectedIndex + return { visible: shadowOnly || !hidden, shadowOnly } +} + /** * Instantly snaps all level Objects3D to their true stacked Y positions - * (ignores levelMode — always uses stacked, no exploded gap). + * (ignores levelMode — always uses stacked, no exploded gap). Presentation + * hiding is undone with it, but a level the author hid stays hidden: the + * capture has to match the export, which prunes it. * * Returns a restore function that reverts each level's Y to what it was * before the snap, so lerp animations in LevelSystem can continue undisturbed. @@ -38,6 +73,7 @@ export function snapLevelsToTruePositions(): () => void { type LevelEntry = { obj: NonNullable> levelId: string + nodeVisible: boolean } const entries: LevelEntry[] = [] @@ -48,6 +84,7 @@ export function snapLevelsToTruePositions(): () => void { entries.push({ levelId, obj, + nodeVisible: level.visible !== false, }) } }) @@ -58,10 +95,10 @@ export function snapLevelsToTruePositions(): () => void { entries.map(({ levelId, obj }) => [levelId, { y: obj.position.y, visible: obj.visible }]), ) - // Snap to true stacked positions and make all levels visible - for (const { levelId, obj } of entries) { + // Snap to true stacked positions and undo presentation hiding + for (const { levelId, obj, nodeVisible } of entries) { obj.position.y = levelElevations.get(levelId)?.baseY ?? 0 - obj.visible = true + obj.visible = nodeVisible } return () => { diff --git a/wiki/architecture/renderers.md b/wiki/architecture/renderers.md index a4a6a52843..f3ddf9f84c 100644 --- a/wiki/architecture/renderers.md +++ b/wiki/architecture/renderers.md @@ -32,6 +32,14 @@ A renderer **must not**: - Manage selection state directly (use `useViewer` for read, emit events for write) - Perform expensive per-frame calculations in the component body +## `node.visible` Is the Renderer's Job + +A custom renderer **must** apply `visible={node.visible !== false}` to its root group (or outer renderable). Registry-driven kinds get this for free — `ParametricNodeRenderer` already sets it — but a kind that ships its own `renderer.tsx` and forgets it stays drawn in the 3D viewport while it is already gone everywhere else: selection candidates, first-person collision, the 2D plan and every export honour the flag. The result is a node that is on screen but unclickable. + +If a system writes `.visible` on the kind's registry object every frame (solo mode does this for levels, the zone systems do it to keep `` labels alive), that write has to fold the node flag in as well, or it silently undoes the prop on the next frame. + +The **Site is the one exception**: its flag governs only its own presentation — ground fill, sculpted terrain, boundary line — and stops there. Buildings and items standing on a hidden Site keep their own flag and still render, and the horizon disc is a world backdrop rather than part of the parcel, so it renders regardless. + ## Example — Minimal Renderer ```tsx