Replace the rerender event bus with direct ObjectsManager calls - #311
Merged
Conversation
sophiedeziel
force-pushed
the
objects_manager
branch
from
September 8, 2025 06:22
2a97b8c to
de7cfa8
Compare
sophiedeziel
changed the base branch from
manage_renders_and_events
to
develop
August 21, 2026 01:12
|
Visit the preview URL for this PR (updated for commit 383531c): https://gcode-preview--pr311-objects-manager-5lxp4zs4.web.app (expires Tue, 22 Sep 2026 19:10:53 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 59bd114ae4847b32c2bba0b68620b9069a3e3531 |
This was referenced Aug 21, 2026
Tests cover event registration/emission, visibility toggles, rendering methods, disposal, and clipping plane updates.
Property changes were routed through a string-keyed dispatcher into one debounced full rebuild. Nothing outside the library listened to any of them, so 21 of the 23 SceneManagerEvent members existed only to say "call a method on my own collaborator". Setters now call the ObjectsManager, which decides how to react: visibility flips group.visible, colors and lighting mutate material uniforms, layer ranges move clipping planes, and only the dimension properties discard geometry and ask for a redraw. SceneManagerEvent keeps the two real notifications, animationComplete and frameRendered. Fixes found along the way: - clipping planes were built from lineWidth/lineHeight instead of the layer range, and never applied to materials created later - every rerender added an empty LineSegments2 and leaked its material - tube materials were pushed once per progressive frame rather than cached per tool - renderPathIndex indexes job.paths but was slicing job.travels, so re-enabling travels after a completed render drew nothing - clear() left the old ObjectsManager in disposables, double-disposing
Coverage was not measurable: no provider was installed. Adds @vitest/coverage-v8, an npm run coverage script, and 100% thresholds scoped to objects-manager, scene-manager and events-dispatcher. scene-manager sat at 23% because a real instance needs WebGL. Stubbing only WebGLRenderer and OrbitControls lets the tests build a genuine SceneManager and assert on actual scene state -- object identity across toggles, material uniforms, group visibility -- rather than on spies.
It was swept in by a git add -A during the develop merge. It holds agent scratch files, not project files.
The string-keyed bus is gone along with its last four events. Each consumer now gets a typed callback instead: - SceneManager.onFrameRendered replaces frameRendered - GCodePreview.onJobUpdated replaces jobUpdated - GCodePreview.onStreamEnd replaces streamReadEnd - animationComplete is dropped; renderAnimated already returns a promise that resolves at the same moment Interpreter no longer takes a dispatcher at all. GCodePreview owns the execute() calls, so it announces job updates itself. This removes the public addEventListener API along with EventName and EventNameType. The demo assigns the callbacks directly. Also drops a stray console.log(this.eventsDispatcher) from Interpreter.
renderPathsAsLines grew a plain array one push at a time and handed it to setPositions, which then copied the whole thing into a Float32Array. Counting segments first and filling the typed array in place skips both the repeated growth and the conversion. On a 7023 path model that is 652818 floats: 3.1ms -> 0.5ms, and about 5 MB of transient boxed doubles never allocated. Output is byte for byte identical.
Adds tests for rendering without a build volume, a rebuild request firing after clear, and construction without a build volume, keeping the scene-manager.ts coverage gate at 100%.
Rebasing onto develop dropped the two merge commits, and with them the manual resolutions they carried. This restores that work: - scene-manager-properties gets back the orthographic camera coverage added while merging develop's camera feature - renderer-smoke looks up the Extrusions group again - the scene-manager suite stays branch-side, as the old-architecture suite tests render paths that now live in ObjectsManager - the demo drops the unused drawBoundingBox ref It also adopts what develop gained since the last sync: the fallbackExtrusionColor guard stays in renderPaths (a tool index past the configured colors warns once and falls back instead of drawing with undefined), and its tests are ported to the new-architecture suite in scene-manager-properties.
sophiedeziel
force-pushed
the
objects_manager
branch
from
August 23, 2026 16:59
610a441 to
941cde9
Compare
- createColorMaterial returns a fresh ShaderMaterial instead of a module-global color-keyed cache. The cache made tools with the same starting color share one instance, so recoloring tool 0 repainted every alias, and a disposed material could be resurrected by the next manager with stale uniforms. ObjectsManager already caches per tool, which is the granularity the mutate-in-place design needs. - clear() carries renderTubes, ambientLight, directionalLight and brightness into the replacement manager. It only carried the dimensions, so loading a second file silently reverted tube rendering and lighting to defaults. - updateClippingPlanes computes an unset start layer as undefined instead of NaN. startLayer?.z - startLayer?.height is NaN when unset, which passed the !== undefined guards and baked NaN plane constants and clip uniforms into every material by default — NaN comparisons are undefined behavior in GLSL, so strict mobile drivers could discard everything. An open bound now stays -Infinity/Infinity.
- A scalar extrusionColor now recolors every tool: setExtrusionColor without a tool index repaints all tube materials and all extrusion lines. Previously the setter defaulted to tool 0, so tools >= 1 kept their old color until an unrelated geometry rebuild. A color array shorter than the tool count likewise repaints the extra tools with the fallback color instead of leaving them stale. - The constructor no longer calls initGui() directly. The devMode setter two lines above already creates the GUI, so devMode previews got two DevGUI instances and the first was never destroyed. - processGCode returns the renderAnimated() promise and processGCodeStream awaits it, so callers can observe the moment the model is fully drawn. That promise is the documented replacement for the old animationComplete event, but neither entry point propagated it. - Job and SceneManagerOptions are exported from the entry point (the onJobUpdated callback takes a Job, which consumers could not name), and the three callback properties document their single-listener contract.
It is type-only, so re-exporting it as a value fails rollup's build.
The page had been fully rewrapped by an editor, drowning the PR's three real changes in whitespace noise. This rebuilds the file from develop's version with only those changes: the bounding box toggle binds to settings.drawBoundingBox, the dev mode widget is added, and the canvas drops its binding to the removed update handler.
Adds the file to the vitest thresholds now that its last four defensive branches are exercised: uniform-less materials in the all-tools recolor and lighting paths, non-line children in the travel group, and an open-ended layer range resetting the clip uniforms to infinities.
sophiedeziel
marked this pull request as ready for review
August 23, 2026 18:13
A clipping bound that sits at the very end of the layer stack is no restriction at all, yet it still planted a plane exactly at the top extrusion layer. Travel moves routinely rise above that layer — a final park move, a wipe — so a preview with endLayer set to the layer count (what the demo does by default) silently hid them (#278). updateClippingPlanes now only bounds a side whose layer is strictly inside the stack: startLayer 1 and endLayer == countLayers produce no plane on their side. A genuinely restricted range still clips travel and extrusion lines alike, which is why the selective-clipping TODO in updateLineClipping is retired rather than implemented: hiding travels outside a restricted range is intended, and with no planes in the unrestricted case there is nothing left to exempt travels from. Fixes #278
Member
|
In terms of scope:
|
Member
|
@sophiedeziel
|
Collaborator
Author
I measured yesterday, we actually have a performance gain with fewer re-renders, and some re-renders that start at some point of the flow instead of doing a whole round trip |
This was referenced Aug 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #282, fixes #375 and fixes #278. Alternative to #310
#310 routes property changes through an event bus. This POC takes the opposite direction and removes
EventsDispatcheraltogether — every notification is a typed callback.It also delivers what #236 asks for: consumers no longer call
render()after changing a property — the library decides per property what kind of update is needed.The two approaches
flowchart TB subgraph B["PR 311 — the manager decides, no bus"] direction TB b1["setter"] --> b2["ObjectsManager"] b2 --> b3["group.visible = false"] b2 --> b4["uniforms.uColor.value = c"] b2 --> b5["material.clippingPlanes = p"] b2 --> b6["debounce -> reset -> redraw"] b7["notifications"] --> b8["onFrameRendered / onJobUpdated<br/>typed callbacks"] end subgraph A["PR 310 — every change is an event"] direction TB a1["setter"] -->|"emit('extrusionColorChange')"| a2(["EventsDispatcher"]) a2 --> a3["debounce 100ms"] a3 --> a4["render()"] a4 --> a5["resetScene()"] a5 --> a6["rebuild every geometry"] a6 --> a7["7023 paths re-walked"] endOf
SceneManagerEvent's 23 members, 21 were…Changeevents with no listener outside the library — they existed only to trigger a full rebuild. The remaining two were real notifications, and they became callbacks, so the enum and the dispatcher are both gone:frameRenderedSceneManager.onFrameRenderedjobUpdatedGCodePreview.onJobUpdatedstreamReadEndGCodePreview.onStreamEndanimationCompleterenderAnimated()already resolves then(The
addEventListener/EventNameAPI only ever existed inside this branch — it was introduced and removed here, so nothing shipped fromdeveloploses it.)Breaking changes vs
develop:GCodePreview.render()is removed — rendering is reactive; usepreview.sceneManager.render()/renderAnimated()for a manual drawSceneManager's constructor narrows to(opts, job); theonFrameargument becomes theonFrameRenderedpropertySceneManagerOptions.nonTravelMovesand thenonTravelmovesfield are removedSceneManager.inchesandinitialCameraPositionare no longer publicprocessGCodenow returns therenderAnimated()promise, andprocessGCodeStreamresolves only once the animated render completesNew callbacks, each holding a single assignable listener:
Where each property goes now
flowchart LR subgraph REBUILD["needs new buffers"] d1["lineWidth"] --> m4["reset + redraw"] d2["lineHeight"] --> m4 d3["extrusionWidth"] --> m4 d4["renderTubes"] --> m4 end subgraph CHEAP["in place"] c1["extrusionColor"] --> m2["material uniform"] c2["travelColor"] --> m2 c3["ambientLight"] --> m2 c4["directionalLight"] --> m2 c5["brightness"] --> m2 c6["startLayer"] --> m3["clipping planes"] c7["endLayer"] --> m3 c8["singleLayerMode"] --> m3 end subgraph FREE["free"] r1["renderTravel"] --> m1["group.visible"] r2["renderExtrusion"] --> m1 end style FREE fill:#1a5c2e,color:#fff style CHEAP fill:#7a5c00,color:#fff style REBUILD fill:#8b1a1a,color:#fffToggling travels
Verified in the browser
3DBenchy, 7023 paths, driven through the real API:
extrusionColor00ff00travelColorff00ffendLayer = 80clipMaxY = 24, model visibly clippedrenderTubesBatchedMesh⇄LineSegments2, rebuilt — as intendedBugs this surfaced
createClippingPlanes(lineWidth, lineHeight)was passing dimensions where a layer range belongedLineSegments2and leaked its materialrenderPathsAsTubespushed a new material per progressive frame instead of caching one per toolrenderPathIndexindexesjob.pathsbut was slicingjob.travels, so re-enabling travels after a completed render drew nothingclear()left the oldObjectsManagerindisposables, double-disposing itAnd found in review, fixed on this branch:
clear()resetrenderTubesand the lighting to defaults for the next jobNaNinto every clipping plane and clip uniform (undefined behavior in GLSL)extrusionColoronly recolored tool 0DevGUIs: the constructor duplicated the setter'sinitGui()renderAnimated()'s completion promise was not propagated byprocessGCode/processGCodeStreamNumbers
SceneManagerEventmembersemit()callsEventsDispatcherscene-manager.ts(vsdevelop)develop)Still open
renderPathIndexstill conflates paths / travels / toolPaths inrenderAnimatedtopLayerColor,lastSegmentColoranddisableGradientare still stored but never read (topLayerColor, lastSegmentColor, toolColors and disableGradient are accepted but never used by the renderer #356)Travel moves above the top layer are still clipped (Travel lines are clipped #278)— fixed here: a layer bound at the very end of the stack no longer produces a clipping planeAssisted by Claude Code - Opus 5