Fix world state going stale after a machine reconfigure - #862
Open
Nick Hehr (HipsterBrown) wants to merge 9 commits into
Open
Fix world state going stale after a machine reconfigure#862Nick Hehr (HipsterBrown) wants to merge 9 commits into
Nick Hehr (HipsterBrown) wants to merge 9 commits into
Conversation
A new /pipeline route that auto-derives the perception pipeline from the machine config, renders it as a DAG, and spotlights the selected stage (or stages) in the existing 3D scene with per-stage tinting. Reuses the existing Threlte canvas, ECS world, hooks, and PCD worker; the only modification to shared code is an optional resource filter on providePointclouds / providePointcloudObjects.
- Scope tint effect to Points-trait entities only - Clarify clear-on-empty semantics (graph panel only, not canvas) - Add preview2D worked-examples table doubling as test fixtures - Add noise-rejection case to derive.ts test list - Call out the shared-refresh-rate UX wart for images
Covers spec phases 1-3 (skeleton, spotlight scene, edges). Phases 4 (2D preview) and 5 (overrides) deferred to follow-up plans.
- Add Task 6.5: widen PartConfig + thread filter props through SceneProviders (fixes the double-provide that defeated the filter and the missing services field) - Rewrite Task 7 route shell to use SceneProviders with filter props and to mount Settings / RefreshRate inside the Canvas snippet via domPortal (fixes the runtime crash from Settings outside Threlte context) - Drop OriginalColors from the new traits (Task 13's renderer-side override doesn't need it) - Gate the tint effect on entity.has(traits.Points) per spec scope - Note Struct.toJson() shape assumption for the attribute-value walker - Replace the now-obsolete Task 18 (real-config wiring) with a manual e2e gate - Rename e2e file to *.test.ts to match local convention
- Provide id/label/onManualRefetch to RefreshRate (pointclouds + vision) - Note that Settings has no gear button on /pipeline because Dashboard is not mounted; the panel itself works once opened
An AlwaysRebuild world_state_store reconfigure swaps the backing resource instance under the same name, so client.current stays stable and neither the transform stream nor the cached snapshot query would otherwise notice. Read machine-status config revision at the provider level and thread it into each createWorldState instance to re-open the stream and reconcile the entity set against a fresh snapshot on every revision change, mirroring useFrames's existing revision-driven refetch pattern. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BkDg3q9GxPnv4FUoA6rQ2K
The reconcile effect was reading revision() tracked, so on a reconfigure it fired synchronously in the same flush as the refetch effect - before listUUIDs.refetch() (a background refetch) had landed new data. It diffed against the still-stale snapshot (a no-op) and latched reconciledRevision to the new revision, permanently blocking the real diff once the fresh snapshot arrived. Read revision untracked in the reconcile effect so it fires on snapshot-data settling instead of the revision flip; the refetch effect continues to own the revision edge. Also: hoist the UNRECONCILED sentinel to module scope (it doesn't need per-instance allocation), and sharpen comments on the accepted mount-ordering redundant-refetch case and the snapshot-vs-stream tombstone override. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BkDg3q9GxPnv4FUoA6rQ2K
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BkDg3q9GxPnv4FUoA6rQ2K
🦋 Changeset detectedLatest commit: 1d24088 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
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.
Recovers live world-state entities after a machine reconfigure without a page reload. Previously, the first config-persisting reconfigure after page load left
<Visualizer>permanently deaf to all world-state transform deltas (ADDED / REMOVED / UPDATED): deleted entities never disappeared and new ones never appeared until a full reload.Why?
Why did the stream go dead after a reconfigure?
A
world_state_storeservice is an RDKAlwaysRebuildresource, so a config-persisting reconfigure replaces it with a new instance under the same resource name. Nothing on the client changes identity:client.currentstays a stable reference, so thestreamTransformChanges$effectnever re-fires, and the old instance'sClose()ends the gRPC stream cleanly (thefor awaitinconsumeChangesjust completes, with no error and nothing to reconnect to). The initial snapshot never refreshes either: itscreateResourceQuerykey is[partID, name, method]with no revision, so a same-name rebuild is served stale cache forever.Why re-snapshot instead of only reconnecting the stream?
StreamTransformChangesseeds its diff baseline from the rebuilt resource's current poses and deliberately does not emit that seed. A bare reconnect would therefore emit nothing for an entity deleted during the rebuild. Reconciling the rendered set against a freshListUUIDs/GetTransformsnapshot on the client is what compensates for that, so this fix is frontend-only with no backend change.Why read
revision()untracked in the reconcile effect?listUUIDs.refetch()is an async background refetch that keepsisLoadingfalse and the olddatain place. If the reconcile effect depended onrevision()tracked, it would fire on the revision edge, diff against the stale snapshot, and latchreconciledRevisionto the new value, blocking the fresh snapshot when it lands. Reading the revision untracked makes reconcile fire on snapshot data settling instead, so it always diffs against fresh data. The refetch effect keepsrevision()tracked because it owns the revision edge, mirroring the existinguseFramesprecedent.Frontend
reconcileWorldState(newsrc/lib/hooks/reconcileWorldState.ts): pure, order-preserving set-diff returning{ toAdd, toRemove }from a fresh snapshot versus the rendered entity set.provideWorldStatesreadsrevisionfromuseMachineStatus(...).current?.config?.revisionat init and threads a() => revisionaccessor intocreateWorldState. The revision is intentionally not read inside the client-loop$effect, to avoid tearing down and rebuilding every world state on each reconfigure.createWorldStatestream$effectnow readsrevision()so it re-subscribesstreamTransformChangesagainst the rebuilt instance on a revision change.$effectcallslistUUIDs.refetch()on revision change (the query key omits revision, so an explicit refetch is required to bust the cache).$effectnow reconciles instead of spawn-only: it destroys UUIDs absent from the fresh snapshot, spawns newly present ones (clearing theirremovedUUIDstombstone first), and tracks a per-revisionreconciledRevisionmarker (replacing the old one-shotinitializedboolean) so it runs once per revision.Out of scope (deferred)
GetTransformfor still-present UUIDs to catch pose/metadata changes that land during a rebuild (left to the re-opened stream to correct).@viamrobotics/svelte-sdkthat would fix this class of bug for all same-name rebuilds.Testing
Added
src/lib/hooks/__tests__/reconcileWorldState.spec.ts(6 table-driven cases: add/remove/intersection, initial mount, no-op, empty snapshot, iterable de-dup, order preservation). Ran the full vitest suite (pnpm test, 345 passing) andpnpm checkclean.The reactivity path (revision to re-subscribe to reconcile) has no automated coverage and should be verified manually against a live machine: after the first reconfigure post-load, confirm a server-side remove disappears, an add appears, and an update moves, all without a page reload.
🤖 Generated with Claude Code
https://claude.ai/code/session_01BkDg3q9GxPnv4FUoA6rQ2K