feat: add Fireplace node with animated procedural fire - #488
Conversation
New architectural fireplace node for the Pascal 3D building editor: - 4 styles: wall, freestanding, corner, double-sided - Parametric mantel, hearth, surround, firebox with inspector controls - Animated instanced fire particle system with flickering point light - 5 fire intensity levels (none → roaring), 4 fire colors - Placement tool with grid snap, alignment guides, SFX - Translucent ghost preview during placement - Resize/rotate/move handles in 3D viewport Registered through the builtin plugin via the same NodeDefinition contract as all other node kinds. Follows the three-checkbox model: geometry (pure builder) + renderer (custom React with useFrame) + tool (lazy placement component).
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 11 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a8d3b37. Configure here.
| <group | ||
| ref={ref} | ||
| position={node.position} | ||
| rotation={[0, (node.rotation * Math.PI) / 180, 0]} |
There was a problem hiding this comment.
Rotation treated as degrees
High Severity
The FireplaceRenderer expects node.rotation to be in degrees and converts it to radians, but node.rotation is stored in radians elsewhere. This unit mismatch causes the fireplace to rotate by a much smaller amount than intended when adjusted via handles or other controls.
Reviewed by Cursor Bugbot for commit a8d3b37. Configure here.
| visible={node.visible} | ||
| {...handlers} | ||
| {...liveTransform} | ||
| > |
There was a problem hiding this comment.
Live transform breaks rotation prop
High Severity
Spreading liveTransform onto the <group> passes a scalar rotation value. However, the <group>'s rotation prop expects an Euler tuple, causing incorrect rotation behavior and visual jumps during interactive drags.
Reviewed by Cursor Bugbot for commit a8d3b37. Configure here.
| handles: fireplaceHandles, | ||
| parametrics: fireplaceParametrics, | ||
| geometry: buildFireplaceGeometry, | ||
| renderer: { kind: 'parametric', module: () => import('./renderer') }, |
There was a problem hiding this comment.
Duplicate fireplace geometry mounted
High Severity
The fireplace geometry is rendered twice. It's defined in NodeDefinition.geometry for GeometrySystem to handle, but the custom FireplaceRenderer also explicitly renders the same geometry. This results in doubled visuals and can cause transformation issues, like dropped corner yaw, especially since markDirty is called on every render.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a8d3b37. Configure here.
| mantel.name = 'fireplace-mantel' | ||
| const mantelY = height - mantelThickness / 2 | ||
| mantel.position.set(0, mantelY, mantelOverhang / 2) | ||
| group.add(mantel) |
There was a problem hiding this comment.
Mantel height control ignored
Medium Severity
The mantelHeight property is exposed and triggers rebuilds, but its value only determines if a mantel is rendered. The mantel's actual dimensions and vertical placement are controlled by mantelThickness and the overall height, so adjusting mantelHeight has no visual impact.
Reviewed by Cursor Bugbot for commit a8d3b37. Configure here.
| intensity={fireConfig.scale * 2} | ||
| color={fireColor} | ||
| /> | ||
| </> |
There was a problem hiding this comment.
Double-sided fire only on front
Medium Severity
For double-sided fireplaces, the geometry includes a rear firebox opening, but fire particles and light are only rendered at the front. This leaves the back opening dark and static, preventing the fireplace from appearing genuinely two-sided.
Reviewed by Cursor Bugbot for commit a8d3b37. Configure here.
| // Corner style — rotate the whole structure by cornerAngle. | ||
| if (style === 'corner') { | ||
| group.rotation.y = (cornerAngle * Math.PI) / 180 | ||
| } |
There was a problem hiding this comment.
Corner style desyncs handles
Medium Severity
For style: 'corner', yaw is applied on the builder Group via cornerAngle, while resize/rotate/move handles are placed in the outer node frame. The visible mesh rotates under the handles, so gizmos no longer line up with the fireplace body. GeometrySystem also drops that group rotation when reparenting children.
Reviewed by Cursor Bugbot for commit a8d3b37. Configure here.
| node.fireboxMaterialPreset, | ||
| shading, | ||
| ], | ||
| ) |
There was a problem hiding this comment.
Geometry resources never disposed
Low Severity
buildFireplaceGeometry results are swapped via useMemo with no dispose of child BoxGeometry/materials, and FireParticles creates a SphereGeometry plus MeshBasicMaterial without teardown. Parametric edits and fire toggles leak GPU resources over a session.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a8d3b37. Configure here.
| /// <reference types="next" /> | ||
| /// <reference types="next/image-types/global" /> | ||
| import "./.next/dev/types/routes.d.ts"; | ||
| import "./.next/types/routes.d.ts"; |
There was a problem hiding this comment.
Unrelated Next env change
Low Severity
This commit changes apps/ifc-converter/next-env.d.ts (an auto-generated Next.js types stub marked “should not be edited”) with no connection to the Fireplace feature. It looks like an accidental local regen included in the PR.
Reviewed by Cursor Bugbot for commit a8d3b37. Configure here.
| } | ||
|
|
||
| emitter.on('grid:move', onGridMove) | ||
| const unsubscribePlacementClicks = subscribeFloorPlacementClicks(commitAtCursor) |
There was a problem hiding this comment.
Placement ignores fireplace clicks
Medium Severity
subscribeFloorPlacementClicks only listens for clicks on kinds listed in FLOOR_PLACEMENT_CLICK_TRIGGER_KINDS, and that list was never updated for fireplace. Clicks on an existing fireplace do not commit floor placement for the fireplace tool or other floor tools that share this helper.
Reviewed by Cursor Bugbot for commit a8d3b37. Configure here.
| selectable: { hitVolume: 'bbox' }, | ||
| duplicable: true, | ||
| deletable: true, | ||
| }, |
There was a problem hiding this comment.
Missing slab elevation support
Medium Severity
Unlike shelf, column, spawn, and item, the fireplace definition never declares capabilities.floorPlaced. FloorElevationSystem therefore never lifts it onto overlapping slabs, so a fireplace on a raised slab stays at level Y and sinks through the slab. The placement tool still calls getFloorStackPreviewPosition, which cannot elevate correctly without a footprint.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a8d3b37. Configure here.
|
This is a distinct and useful node proposal, so I’m keeping it open, but it needs a current-main revision before architectural review:
A focused follow-up commit satisfying those points would make this reviewable as a real feature rather than a prototype snapshot. |
Aymericr
left a comment
Aymericr
left a comment
There was a problem hiding this comment.
Thanks for this — a lot of it is in the right shape. Specifically good: the node is registered in both required places (AnyNode union in packages/core/src/schema/types.ts and builtinPlugin.nodes in packages/nodes/src/index.ts), buildFireplaceGeometry is a genuinely pure (node) => Group builder, the tool reuses the shared helpers in packages/nodes/src/shared/floor-placement.ts instead of re-rolling raycasts, and the HandleDescriptor set looks right. bun run check does exit 0 on this branch.
That said I can't merge it as-is — there are runtime breakages, and they mostly trace back to one architectural decision.
The root issue: this definition declares both geometry and renderer. packages/nodes/src/fireplace/definition.ts:154-155 is the only definition in the repo (of 46) doing that. <NodeRenderer> returns early when def.renderer is present, so <ParametricNodeRenderer> never runs — but your renderer also calls useRegistry (so <GeometrySystem> reparents the built children onto the group) and mounts <primitive object={geometry} /> from its own useMemo build at renderer.tsx:200. That's two copies of every mesh. wiki/architecture/node-definitions.md:189 treats geometry and renderer as alternatives. Simplest fix: drop renderer, delete fireplace/renderer.tsx, and let the shared parametric path own the mount. Almost everything below disappears if you do.
Blocking, in order:
{...liveTransform}NaNs the transform on drag —renderer.tsx:198.LiveTransform.rotationis anumber, and R3F'sapplyPropson a number withoutsetScalardoestarget.set(value).THREE.Eulerhas nosetScalar, sorotation.set(0.75)leaves y/zundefinedandupdateMatrix()yields an all-NaN matrix — I ran this to confirm. The fireplace vanishes mid-drag.parametric-node-renderer.tsx:67-82normalizes rotation to a tuple exactly to avoid this.- Degrees/radians mismatch —
renderer.tsx:195does(node.rotation * Math.PI) / 180, butpackages/core/src/schema/nodes/fireplace.ts:29rotationis radians like every other node. Should berotation={[0, node.rotation, 0]}. (cornerAngleis degrees and does need the conversion — that one's fine.) markDirtyin the render body —renderer.tsx:184writes to the store during render, re-triggering a rebuild each commit. Move it into auseLayoutEffectkeyed on the geometry params. Related: #556 is fixing the same class of thing for walls.- Corner style's yaw is silently dropped —
geometry.ts:176setsgroup.rotation.y, but<GeometrySystem>reparents onlybuilt.childrenand never applies the built group's transform. Wrap the 8 meshes in an inner rotatedGroup(returned as the root's single child), or bake the rotation into the children. mantelHeightis a dead control —geometry.ts:164only uses it as a> 0gate; it never affects the mantel's size or Y. I diffed 0.05 vs 0.50 and got byte-identicalBoxGeometryparams and the same position, yet it's exposed inparametrics.ts:26. Either wire it to the thickness or remove the control.capabilities.floorPlacedis missing —definition.ts:141-150. Without itgetFloorPlacedElevationearly-returns 0 (packages/core/src/hooks/spatial-grid/floor-placed-elevation.ts:67-75), so thegetFloorStackPreviewPositioncall intool.tsx:77-82is inert and a fireplace on a raised slab sinks through it. Also add'fireplace'toFLOOR_PLACEMENT_CLICK_TRIGGER_KINDS(shared/floor-placement.ts:16-28) so the placement click actually fires.
Should-fix:
- No disposal anywhere — the
useMemo'd group, the emberSphereGeometry/MeshBasicMaterial, and the built geometry inpreview.tsxall leak.cupola/renderer.tsxhas the pattern (useEffect(() => () => geometry.dispose(), [geometry])). - Raw per-node
<pointLight>+ twouseFramecallbacks (renderer.tsx:127, 75, 120) bypass the pooled light system (packages/viewer/src/systems/item-light/item-light-system.tsx, POOL_SIZE = 12, with camera scoring and hysteresis). Register aLightEffectviauseItemLightPool— seepackages/nodes/src/item/renderer.tsx:805. Twenty fireplaces currently means 20 lights and 40 frame callbacks. - No tests. A
fireplace/geometry.test.tscovering child count per style,mantelHeightaffecting dimensions, corner yaw landing on children, and the lintel staying insidenode.heightat the schema minimum (0.8 currently puts the lintel top at 1.17 m) would lock all of the above in. - CI has never run on
a8d3b37— please push so the required checks execute.
Nits: renderer.tsx:152 uses // eslint-disable-next-line react-hooks/exhaustive-deps, which is inert here — this repo uses // biome-ignore lint/correctness/useExhaustiveDependencies:. The apps/ifc-converter/next-env.d.ts change is unrelated to the feature and reverts a generated path main still carries; please drop it. The four near-identical material resolvers in geometry.ts could collapse into one resolveMaterial(node, role), which would also give the exported FireplaceMaterialRole a use. And there's no floorplan builder, no mcp block (this would be the only definition of 46 without one), and no bake policy — worth deciding deliberately whether the fireplace should show in 2D plans, be MCP-addressable, and survive GLB export.
Two things that are not on you: the packages/core/src/events/bus.ts conflict and the removed packages/plugin-trees are just base drift (82 commits since merge base 7d6879c4) — a rebase clears both. And the local bun-test failures reproduce identically on main, so they're environmental.
One process question before you invest in the fixes: I couldn't find an issue requesting a fireplace node. Could you say a bit about the use case? If it's wanted, the geometry builder here is a good starting point and the fix list gets much shorter once the custom renderer is dropped.
2 participants
What does this PR do?
Adds a new architectural Fireplace node kind to the editor — the first node with an animated procedural effect (instanced fire particles + flickering point light via
useFrame).wall,freestanding,corner,double-sidednone→roaring), 4 fire colors (orange,amber,blue,white)Registered through the builtin plugin via the same
NodeDefinitioncontract as all other node kinds. Follows the three-checkbox model fromwiki/architecture/node-definitions.md:geometry(pure builder) +renderer(custom React withuseFrame) +tool(lazy placement component).How to test
git clone https://github.com/toolucid/editor.git && cd editor && bun install && bun devhttp://localhost:3002in a browsernone→roaring), fire color, and fireplace stylebun check-typesandbun check— both pass with 0 errorsScreenshots / screen recording
Will add a screen recording before review.
Checklist
bun devbun checkto verify)mainbranchNote
Medium Risk
Adds a new discriminated union member and event surface area, plus per-instance
useFrameanimation and extra lights that could affect viewer performance with many fireplaces; otherwise mirrors existing furnish nodes with no auth or persistence changes.Overview
Introduces a new
fireplacefurnish node end-to-end: Zod schema in core,FireplaceEventon the editor bus, and registration in the builtin plugin.The node supports parametric dimensions (mantel, hearth, surround, firebox), four styles (
wall,freestanding,corner,double-sided), and inspector fire settings (intensity and color). Implementation follows the standardNodeDefinitionsplit: box-based geometry builder, a custom renderer with instanced particles and a flickering point light driven byuseFrame, floor placement tool (grid snap, alignment guides, ghost preview), and viewport resize/rotate/move handles.Also updates
apps/ifc-converter/next-env.d.tsto import Next route types from./.next/types/routes.d.tsinstead of the dev path.Reviewed by Cursor Bugbot for commit a8d3b37. Bugbot is set up for automated code reviews on this repo. Configure here.