Repository navigation
Move the color state into the ObjectsManager - #389
Merged
Merged
Conversation
The manager owned the materials and did every recolor, but SceneManager held the source of truth: the extrusion color(s), the travel color, and the per-tool fallback with its warn-once bookkeeping. renderPaths had to compute each tool's color before handing it over. The manager now owns the colors. setExtrusionColor takes the full value (one color or an array indexed by tool), stores it, and repaints in place; colorForTool resolves what a tool draws with, including the fallback. renderExtrusions takes just a tool index, and renderTravelLines defaults to the stored travel color, so renderPaths passes no colors at all. SceneManager's accessors delegate, and clear() carries both colors onto the replacement manager. Repainting the tools beyond a shrunken color array now derives the tool list from what is actually drawn (materials and line userData) instead of job.toolPaths, so the setter no longer needs the job at all.
sophiedeziel
force-pushed
the
objects_manager_colors
branch
from
August 23, 2026 20:56
8b71d55 to
3c38b3b
Compare
|
Visit the preview URL for this PR (updated for commit 8491d6b): https://gcode-preview--pr389-objects-manager-colo-vthxbky3.web.app (expires Tue, 22 Sep 2026 21:10:57 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 59bd114ae4847b32c2bba0b68620b9069a3e3531 |
sophiedeziel
added
3.0
Two renderExtrusions call sites still passed a Color where the new signature takes a tool index. They ran anyway — vitest strips types and typeCheck only sees the entry file — and their instanceof-only assertions kept passing, so nothing caught it. New assertions close the review's gaps: - set-then-draw resolves each tool's array entry, in both line and tube mode; the whole suite previously survived colorForTool returning the array's last entry for every in-bounds tool - shrinking the color array repaints line-drawn tools, pinning the LineSegments2 branch of renderedToolIndices - renderTravelLines really uses the stored travel color as its default, asserted on the drawn material at both unit and scene level - a missing tool color warns again after clear(), pinning the documented decision that the warn-once bookkeeping belongs to the swapped manager
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
1 participant
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.
Stacked on #380 (which stacks on #311) — the next slice of moving job-derived scene state into the manager.
The problem
The manager owned the materials and performed every recolor, but
SceneManagerheld the source of truth:_extrusionColor,_travelColor, and the per-tool fallback with its warn-once bookkeeping. EveryrenderPathscall computed each tool's color before handing it over, andclear()only worked because the colors happened to live outside the manager being swapped.What moves
SceneManager._extrusionColor+ fallback logic inrenderPathsObjectsManager.extrusionColor+ privatecolorForTool()SceneManager._travelColor, passed on every render callObjectsManager.travelColor;renderTravelLinesdefaults to itsetExtrusionColor(color, toolIndex?)(Color | Color[]), stores it, repaints in placeSceneManager.defaultExtrusionColorrenderExtrusions(paths, toolIndex)now resolves its own color, sorenderPathspasses no colors at all, andclear()carries both colors onto the replacement manager like the other settings.A small robustness win
Repainting tools beyond a shrunken color array now derives the tool list from what is actually drawn (the per-tool materials and the lines'
userData.toolIndex) instead ofjob.toolPaths— the setter no longer needs the job, so it is inherently safe afterclear().Behavior notes (deliberate, minor)
clear()(it lives on the swapped manager), so a newly loaded file can warn again — arguably more correct, since it may have different tools.SceneManager's public color accessors are unchanged; the demo and DevGUI need no edits.Tests
The existing color suites were rewritten against the new API — mostly turning spy assertions into state assertions on the actual materials (the fallback tests now check
uColoruniforms instead of call arguments). Aclear()carry-over test is added. Both files hold their 100% coverage thresholds; 394 tests green.Assisted by Claude Code - Fable 5