feat(scene): correctable, disappearing objects on upstream ConceptGraphs - #234
Conversation
List/relabel/geometry/delete/flush for derived Scene objects. Every correction carries the map epoch it was read under (map_id + generation echoed by ListObjects), so an edit aimed at a stale map is rejected instead of applied to whichever map is live now. Runtime-only by default; persist_to_snapshot makes a correction survive reload. Contracts ported unchanged from the #199 review; the implementation follows separately on top of upstream ConceptGraphs. Assisted-by: Claude Code:claude-opus-5
Six correction entry points on ObjectRegistry: sticky operator labels with explicit undo (the pre-edit model state is saved for restore), provenance-marked non-navigation-grade geometry overrides, and derived object delete/flush that never touch the robot self-record. Operator-corrected objects are exempt from both soft mark-missing and TTL hard deletion: a relocated object's stale perception cloud reads as clear absence at its old footprint, and that evidence must not erase or hide a human's edit. Assisted-by: Claude Code:claude-opus-5
ObjectMutationCoordinator keeps registry, perception, graph cache, and saved snapshot coherent for the correction contracts. Every mutation revalidates the map epoch under the ops lock and rolls the runtime back if the snapshot write fails. The perception layer stays upstream ConceptGraphs; corrections hook in from outside the map: the projection tick refreshes liveness but never overwrites an operator-corrected pose, eviction releases a culled uuid without hiding the human's edit, and deletion drops the bound CG entries under the inference lock so the next tick cannot resurrect a deleted record. Differences from the reviewed #199 draft, per review findings: - operator labels are stored verbatim instead of canonicalized ("desk" no longer silently becomes "table") - a single edit invalidates only its own caption/relation cache entries; the full clear_derived_state is reserved for flush Assisted-by: Claude Code:claude-opus-5
Ten focused tests: stale-epoch rejection, robot-record guard, verbatim operator labels, provenance-marked geometry, delete/flush coherence with the detector, semantic-hold blocking, eviction immunity for operator corrections, and per-object graph-cache invalidation. Assisted-by: Claude Code:claude-opus-5
Four thin handlers over the coordinator, declared on Atlas next to the existing eight tools. ListObjects now echoes the map epoch (map_id + generation) that every correction call must assert. Assisted-by: Claude Code:claude-opus-5
The upstream MapObjectList only ever grows, so an object carried out of the room stayed in the map forever (#177). This adds the missing negative evidence: each tick, unmatched objects' stored clouds are projected into the current depth image, and a miss counts only where the measured surface is materially *behind* the stored object — a closer surface is occlusion, similar depth is a detector miss, and invalid depth is unknown; none of those delete state. Only after visibility_miss_ticks consecutive clear-absence verdicts do the object's entries leave the map; the projection sweep then soft-evicts the registry record and the TTL prune finishes the job unless the object is re-observed first. The projection/decision function is ported unchanged from the reviewed #199 draft — it was the part of that PR worth keeping — and runs against the untouched upstream ConceptGraphs structures. Assisted-by: Claude Code:claude-opus-5
github-actions
Bot
added
type:feature
CI's clippy flags chunks_exact(2) with a constant chunk size and asks for as_chunks, which also drops the per-chunk indexing. Assisted-by: Claude Code:claude-fable-5 (cherry picked from commit cec06ee)
Codegen derives the MCP type module from this list, so without the four new entries the generated semantic_map_mcp lacks the correction types and the service fails to import at boot. Assisted-by: Claude Code:claude-opus-5
|
Reviewed at Two notes, neither blocking. 1. A zero-detection tick skips absence evaluation entirely. In 2. Neither needs to hold the merge. Filing (1) as a follow-up is fine. |
fc4e1f1 to
ea067de
Compare
|
Field-design audit of the contract surface, at The five On the IDL, three things needed correcting and are now fixed:
One inconsistency is noted but deliberately left alone: counts across this library are mixed — Scene suite: 202 passed. The one failure, |
… side UpdateObjectGeometry requires size_x/size_y/size_z and frame_id, and rejects a frame that differs from the object's own. ListObjects published neither, and no message under capabilities/lib/semantic_map carried extents or a frame at all, so an operator had no contract-level way to obtain what the write call demands. The response type had the same hole: it claimed to return the object "with the geometry applied" while being unable to represent that geometry. Object gains size_x/size_y/size_z, frame_id, and observation_count. The extents and frame close the round trip. observation_count gives a caller that read an object and then edits it a way to tell whether perception moved underneath it: the map epoch orders edits against map reloads, not against ordinary perception updates, so it cannot answer that on its own. SceneGraphNode already published observation_count, so this is not new state on the wire. ListObjects gains generation_supported. A provider without a generation counter reports -1, which silently degrades the epoch check to comparing map_id alone and stops ordering two edits racing inside one map; callers that care can now read that instead of inferring it from the sentinel. The four mutating services gain an optional note. Operator corrections outrank perception and are exempt from TTL deletion, so they are permanent and were unattributable. Label and geometry corrections store it with the object's provenance and drop it when the override is cleared; delete and flush have no surviving record and log it instead. UpdateObjectLabel now bounds `label` in both modes. It is ignored when clear_override is set, but accepting an unbounded string there invites a caller to believe it was used.
…tions `observation_count` was uint64 on Object while SceneGraphNode has published the same quantity under the same name as int32. One name and one meaning must not carry two types across one IDL library, so Object follows the type that is already on the wire. `note` is stripped and capped at 512 characters by the coordinator. The IDL did not say so, which made a longer note truncate silently. The contract now states it, in all four services that accept one. Object.frame_id said an empty value means the entry has no metric frame. That is wrong for scene-graph derived entries in GetObjectContext.nearby_objects, whose coordinates are in the map frame but whose frame string is not recorded. The comment now says what empty actually means and what follows from it: a geometry edit built from such an entry is rejected rather than applied blind.
The branch was cut on 2026-08-19 and had fallen 174 commits behind. Webots CI on the old base would have run the pre-2026-08-22 workflow, which predates the SLAM-map summary embed, the cmd_vel-driven mapping loop and the Filatov map quality metrics — so a green run there would not have said anything about what lands on dev. Merged clean, no conflicts. Scene suite on the merged tree: 242 passed. The two failures it does show, test_goal_room_rejects_stale_geometry_before_navigation and test_runtime_protobuf_contract, both fail identically on unmodified dev.
54f465f to
2c90efa
Compare
GitHub Actions
Inactive
…'s own checks
start.sh expands four optional argument arrays as "${ARR[@]}". Under set -u,
bash 3.2 — still the default on macOS — treats that as an unbound variable when
the array is empty and aborts. The script died right before `docker run`, so the
service container never launched and the runtime-protobuf contract test only
ever saw the codegen container. Expanding as ${ARR[@]+"${ARR[@]}"} keeps bash
4.4 behaviour and works on 3.2.
goal_room checked the Soma footprint twice: once at the top and once where the
footprint is actually used. The early copy made robot readiness preempt every
check on the request itself, so an unknown or stale room came back as "robot
geometry is not ready" and the caller could not tell which of the two was
wrong. The check at the point of use is kept; the duplicate is removed.
Scene suite: 244 passed, 0 failed. Both of these failed on unmodified dev.
GitHub Actions
Failure
…ummary The map has been copied next to index.html and linked from summary.md since the feature landed, but the HTML report never referenced it: the published page had zero <img> tags, so the only way to see the map was to guess the slam-map.png URL. The report page now carries a SLAM Map section whenever the file is there. Two things had to change for that to work. write_html gained the flag, and the copy that sets it moved above the call — it ran after, so the flag was always false at render time and the section could never have appeared. The image is scaled with nearest-neighbour. An occupancy grid is a couple of hundred pixels wide at its own resolution, and smoothing it on the way up invents wall edges that are not in the map.
The previous commit changed ${#GPU_ARGS[@]} to ${#GPU_ARGS[@]:-0} while
guarding the array expansions. An array length is never unset, so the default
was pointless, and `${#arr[@]:-0}` is not valid syntax: bash rejects it as a
bad substitution. start.sh exited 1 and Scene never reached Atlas registration,
which took the whole Webots suite down with it.
The expansion guards themselves were fine and stay. Only the length test is
reverted. Checked on both bash 3.2 and the runner's bash 5.2 this time, with
empty and populated arrays and an argument containing an `=`, rather than
trusting `bash -n` — a bad substitution is a runtime error and parses clean.
GitHub Actions
Inactive
1 participant
Rebuild of #199 on top of
dev, per its review. Upstream ConceptGraphs stays the perception core — every feature here hooks in from outside theMapObjectList; the twelve upstream symbols Scene uses are untouched.Closes #177. Supersedes #199 (draft), which reached the right contracts and the right absence-detection algorithm but replaced the ConceptGraphs SLAM kernels with an in-repo reimplementation, undisclosed, inside a 13.5k-line commit. This PR is
+1,843 −7over 20 files against #199's+15,792 −2,089over 69.What it does
list/relabel/geometry/delete/flush, ported unchanged from feat(scene): make derived object state correctable and recoverable #199's review): every call asserts the map epoch it was read under (ListObjectsnow echoesmap_id+generation); an edit aimed at a stale map is rejected, not applied.visibility_miss_ticksconsecutive clear verdicts, then soft-evict → TTL, reversible by re-observation until the prune.Review findings from #199, resolved here
slam_classes,mapping,slam.utils,model_utils) unchanged; the aarch64 concern was moot —open3d>=0.17,<0.19ships aarch64 wheels, which is why that pin existsinvalidate_objectdrops only that object's entries; the full clear is reserved for flushdevDeliberately not ported
Web correction UI (+542), TensorRT cache (+422), model storage/baking (+210), lifecycle runtime (+240), perception profiles, and #199's 34-file test suite — the ten tests here cover the decisions (epoch rejection, guards, verbatim labels, narrow invalidation, absence verdicts, streak gating), not the plumbing. Each can be its own reviewable PR if wanted.
Validation
system/scenepytest: 146 passed, 30 skipped locally; the 10 failures are identical ondev(missing codegen env on this machine) — full suite + Webots runs on the GPU runner next.scene-benchmark-harnessbranch) scores via Scene's HTTP surface, so the five-world numbers can be re-measured on this tree as-is.