Repository navigation
fix(studio): keep saved strip positions when presets and scenes change - #434
Merged
Merged
Conversation
Without a custom LED zone, effect applies and layout publications rewrite the primary zone's layout from the active spatial layout, so the daemon needs to know when a placement saved on that zone belongs in the layout instead. The predicate wraps the existing custom LED zone check so the scene manager and its callers share one rule. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A strip moved and saved in Studio snapped back on the next effect or preset change. The save only rewrote the scene's copy of the primary zone, but while a scene has no custom LED zone that copy follows the active layout: effect applies, layout publications (hotplug, auto layout, layout apply, scene activation) and the startup restore all rebuild it from that layout, which never learned about the move. Saving such a zone now also writes its outputs into the active layout. The layout keeps its id, name, description, canvas size, sampling defaults and version, and takes the zone's outputs in the zone's order. The store entry is persisted and the layout republished through the guarded activation path, so the spatial engine, the layout store and the zone agree. The layout update guard is held from the zone read through the republication and released before connectivity convergence, which takes the guard itself. The workflow runs as one spawned task so a dropped request cannot split the two writes, and the Save answers with the post-publication revision when no other commit landed in between. Scenes with a custom LED zone keep a scene-local primary layout, as before. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Mirroring the whole primary zone into the active layout was wrong whenever the zone was a stale snapshot. A named scene keeps the zone it was created with, and activation never re-aligned it, so a Save there dropped outputs the layout gained since (and auto-excluded their devices), reverted positions saved from another scene, and replaced a stored layout edit that had not been applied yet. A Save now copies only the placement fields of outputs whose placement it changed, by output id, onto the stored entry and onto the published layout separately, wherever each holds that output. Outputs neither layout knows (members assigned in Studio) are added to both, as the startup restore does. Nothing is removed, order and every other field stay put, exclusions are untouched, and an unapplied stored edit stays stored and unapplied. If the store write or the publication is refused, the zone is put back and the refusal is returned, so a Save is never left half applied; a failed rollback is named in the error. Scene activation, deactivation, deleting the active scene, and the configured startup scene now re-align a following primary zone to the active layout, so a scene switch never shows stale positions. Snapshot scenes keep what they captured, and a scene with its own layout is re-aligned again when that layout publishes. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Studio assumed a zone layout Save moves the scene revision by one. A Save on a primary zone that follows the active layout also publishes that layout, which is a second commit, so the next guarded write before the scene refresh landed was refused as stale. The Save now records the revision the daemon reported in ETag, falling back to the old guess only when the header is missing. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
This review includes 11 billable files and costs up to $2.75. Or wait 23 minutes for your next included review. View limit details
Comment |
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.
A user on Reddit reported that LED strip positions saved in Studio reset whenever they changed presets. Studio's Save writes the new placement only into the active scene's copy of the primary zone (
PUT /api/v1/scene/zones/{zone}/layout), and the global active layout never gets it. Applying an effect or a preset rebuilds the primary zone from that global layout whenever the scene has no custom LED zones, which is the default setup for a new rig. The strip snaps back. Layout publications (hotplug, auto-layout,POST /layouts/{id}/apply) and a daemon restart reset it the same way. Every release since Studio shipped has this bug. With this PR, a saved placement lands in the shared layout and survives all of those paths.What this changes
A Save carries its placements into the shared layout. When a Save targets a primary zone that follows the active layout (no display target, and no custom LED zones in its scene), the daemon copies the placements that Save changed into the active layout. It diffs the zone before and after the Save and takes only the placement fields that moved (position, size, rotation, scale, orientation), merged by output id. The changes go into the stored catalog entry in
layouts.jsonand into the published layout separately, wherever each already holds that output. An output that neither layout has, such as a member assigned in Studio, is added to both. Nothing is ever removed, output order and every other layout field stay as they were, and auto-layout exclusions are never touched. The store is written first, then the layout publishes under the layout guard, and a failed publish rolls the store back. If either step is refused, the zone write is undone and the Save returns the refusal, so a Save never lands halfway.Scene switches re-align a following primary zone. Activating, deactivating, or deleting the active scene, and starting into the configured scene, now re-align a following primary zone to the active layout. A named scene therefore never shows stale positions after a switch. Snapshot-locked scenes keep what they captured. A scene with its own
layout_idis re-aligned again when that layout publishes.Studio takes its revision from the response. The Save can now commit twice: the zone write plus the layout publication. Studio reads the scene revision from the response's
ETaginstead of assuming one bump, and falls back to one bump only when the header is missing.Why it is shaped this way
The first version of this fix mirrored the whole primary zone into the active layout, and independent verification failed it. A named scene keeps the primary zone it was created with, so its zone can be a stale snapshot. Mirroring that snapshot had three bad effects:
Copying only what the Save changed, by output id, keeps the Save from asserting anything about outputs it never touched. Re-aligning on activation removes the staleness that made the first version dangerous.
Verification
just verifypasses locally (Rust fmt + lint + test)just denypasses (required for dependency or license changes)just ui-testandjust ui-buildpass (required forcrates/hypercolor-ui/)just sdk-lint,just sdk-check, andjust sdk-buildpass (required forsdk/)just python-verifypasses (required forpython/)just compat-checkpasses (required fordata/drivers/vendors/*.toml)just docs-buildpasses (required for docs or README changes)cd docs && zola checkpasses (required for docs link/content changes)scripts/orpackaging/)just e2e-buildpasses with the normal Servo stack (required for daemon/UI/effect integration changes)just e2e-build-cpupasses when validating the CPU smoke fallbackjust e2epasses against the Servo stack (required for end-to-end behavior changes; starts daemon/browser)studio_layout_persistence_testshas 15 tests:On
origin/main, the original report's tests fail with the strip back at its old position. On the first version of the fix, the five verification-failure and re-align cases fail. All 15 pass now.Gate results:
--test-threads=1)cargo test -p hypercolor-daemon --libfiltered to scene and layoutcargo test -p hypercolor-core --test zone_lifecycle_testscargo clippy -p hypercolor-daemon -p hypercolor-core --all-targets -- -D warningscargo fmt --all -- --checkhypercolor-ui:cargo check --liband theapi::client,api::zones, andlayout_buildertestsAn independent verifier failed the first version and passed this one, and its three reproduction probes pass against this head.
startup_tests,daemon_integration_tests, anddiscovery_testswere not run locally, because they start real device discovery; CI runs them. The restart case is simulated from the persistedlayouts.jsonand runtime session rather than a fullDaemonState::start. The UI change has not been exercised in a browser.Notes for reviewers
Start with
mirror_primary_zone_save_under_guardindomain/layout/workflows.rs, thenset_zone_layoutandundo_zone_layout_saveindomain/scene_tree.rs. The lock order matchesactivate_scene. The layout guard is taken before the scene mutation and released before connectivity convergence.The verifier left four low-severity notes, all from static traces:
PUT /scenes/{id}now loses that layout on activation. That matches the "follows the active layout" model; snapshot scenes are skipped.Not changed here: a Save that only reorders outputs is not mirrored. An active layout with no stored entry, such as a preview, is published but not persisted.
🤖 Generated with Claude Code