Conversation
There was a problem hiding this comment.
Changes requested. See the three inline findings for the affected code ranges, evidence and suggested fixes.
acrlw
left a comment
There was a problem hiding this comment.
Changes requested: two P2 findings and one P3 documentation correction, attached to the affected lines. All three require EmbodiChain changes.
Validation: 304 focused tests passed; both defects reproduced; live two-environment Viser attachment, following and cleanup passed. Four native overlay tests remain skipped pending runtime capabilities, and the complete sim CI lane remains unverified.
| env_visible = ( | ||
| True | ||
| if mesh is None or mesh.env_id is None | ||
| else self._env_visibility.get(mesh.env_id, True) |
There was a problem hiding this comment.
[P2] Include marker environments in visibility controls and preserve their state
An environment containing only markers gets no Environment N control. The environment inventory collects ordinary nodes, cameras, gizmos and joint controls from the manifest, while markers live in SceneOverlays.meshes. Their environment IDs are therefore omitted, and this lookup treats the missing visibility state as visible.
This also breaks visibility after asset removal. In a two-environment scene, hide Environment 0, then remove the sole parent asset so that only markers remain. The manifest refresh clears _env_visibility, and the next frame makes the hidden markers visible again. A backend probe reproduced this transition; a live Viser scene also confirmed that markers for both environments survive parent removal while the environment controls and visibility state disappear.
Please include marker environments when creating and refreshing the controls, and retain visibility state for environments that still exist. The existing visibility test inserts an ordinary SceneNode, so it misses this case. Extend that test file with marker-only environments and removal of the last parent asset.
| else self.counts[env_id] | ||
| ) | ||
| state = ( | ||
| self._defaults(count) | ||
| if count != self.counts[env_id] |
There was a problem hiding this comment.
[P2] Read the current environment's marker count directly during updates
update() calls _updated_state() for each selected environment, but each self.counts[env_id] first evaluates the counts property, traversing every environment and constructing a full tuple. Updating all E environments therefore adds O(E²) counting work. Color-only or orientation-only updates perform that full traversal twice per environment.
With 4096 environments and one box per environment, the CPU state update in group.update(translations=...) took 390.13 ms at the median of three runs after warm-up. A control that changed only the two count lookups to read the current environment's array length took 59.03 ms, with count and position assertions passing in both cases. This extra work runs on the caller's thread and slows simulations that update markers each step.
Please read old_count = len(self._states[env_id]["translations"]) once at the start of _updated_state() and reuse it for omitted translations and count-change detection. Reuse the existing full/partial-environment, count-change and attribute-preservation tests, then repeat the timing comparison at the same scale.
| The adapter creates an ordinary Arena-owned ``MeshObject`` without adding | ||
| physics components. It configures overlay routing, disables shadow and picking, | ||
| adds the mesh without automatic building, assigns an owned unlit RGBA material, | ||
| and then builds and attaches the object. Creation failures remove the incomplete | ||
| object. No Spawn factory or descriptor is required. |
There was a problem hiding this comment.
[P3] Update the native marker documentation to match the Scene descriptor API
This paragraph describes manually adding an object to an Arena, assigning a material and building it, and explicitly says that no Spawn descriptor is required. The current NativeMarkerRenderer._create() instead constructs MaterialDesc, RenderDesc and MeshObjectDesc, then calls Scene.add_mesh_object(). Initialization also checks that these descriptor and Scene APIs are available.
Users checking runtime compatibility against this documentation would miss required Scene/descriptor capabilities. Please update the creation, stable-handle and removal flow, including Scene ownership of object registration and the engine's reference-aware material collection. The visualization context already describes this behavior; the API documentation should agree with it.
Description
Add render-only mesh markers shared by native DexSim and Viser, with RGBA transparency and reusable box, sphere, cylinder, capsule, cone, arrow, frame, and custom-mesh prototypes.
Groups default to all
SimulationManager.num_envs; updates, visibility and clear accept selectedenv_ids.scope="world"creates one global batch. Groups attach to registered object/robot/articulation roots or links, follow host updates with local offsets, and detach while preserving world poses. Existing legacy drawing APIs remain available; there is no new publicarena_index.Automatic attachment refresh preserves browser capture cadence and isolates visual failures from simulation accounting. Manual mutations validate and publish synchronously with rollback. Native markers are excluded from camera outputs by default; opt-in overlay inclusion retains the documented Hybrid NRD limitation.
Docs CI also fetches the architecture snapshot's pinned evidence commit when squash history omits it. Exact-source assertions and generated data remain unchanged.
Refs #129.
Dependencies: native rendering requires DexSim MR !1423, engine revision 1acf56f57f7126f8eaa91d27c2eb15c8ca9ff916. The adapter creates ordinary
MeshObjectDesc(physics=None)withRenderDesc/MaterialDescthroughScene.add_mesh_object, retains existing stableSpawnedRigidBodyhandles, and removes them by Scene path. Lightweight Scenes exist before physics finalization on both backends; marker operations do not prepare physics, and handles survive Newton rebuild. Cleanup tolerates prior Scene-close invalidation. No DebugMesh or standalone public factory is added.The engine dependency now removes its temporary
_owned_materialslist and all newownedAPI parameters. Ordinary material assignment holds references; actor removal and Scene-handle invalidation are immediate, while unused material instances are reclaimed at existing manager cleanup boundaries. Live bodies, SHARE clones and external material handles remain valid until their final references are released. A retained material template also preserves its default parameters; unused named instances remain collectable. The engine ports the RenderService cleanup scheduling from unmerged DexSim !1431, source snapshot219bb7ba, and covers Filament default/Scene managers too. Collection runs after 60 seconds of accumulated render updatedt, not a wall-clock timeout. This does not change the marker group's public API or introduce consumer-side GC.Merged current main (
7ae9a501), including its COM/body-frame inertia adaptation. Resolved the simulation-update overlap by retaining the substep observer before marker attachment refresh and visualization capture; preserved both test suites. The registered-articulation attachment smoke now runs without the obsoletecom_quaternioncapability skip. Keep this PR draft pending the qualified engine release; no unpublished package version is pinned.Type of change
Validation
Current consumer submitted commit: d893318. Current consumer CI: CI #35559124254 — cancelled after the sim lane stopped producing output. Current engine submitted commit/pipeline: 1acf56f57f7126f8eaa91d27c2eb15c8ca9ff916 / DexSim #71280 — SUCCESS, all six jobs.
tests/sim/test_marker_rendering.pycases passed with--run-gpuin 12.71 s against engine1acf56f57: RGBA/sensor isolation, selected environments without preparation, repeated handle retirement with retained material references, registered-articulation root/link attach and detach.1acf56f57; it does not qualify the full sim lane or establish the cause of the remote released-SDK stall.1acf56f57: all six jobs in #71280 succeeded, including renderer/Python matrices, six lifecycle configurations × five rounds, Python 3.10–3.12 packaging and docs. Engine release remains pending. H1 performance is not qualified: teardown spread 10.44% exceeded the 10% stability limit (warning-only gate).Remaining integration work
TestWorkspaceResultsCacheIntegration::test_cache_hit_skips_recompute. A bounded local run of all four analyzer cases and that next cache case completed with all five passing against the revised engine. The remote released-SDK stall was not reproduced locally; obtain remote phase/stack evidence before changing production code or declaring the full sim lane qualified.Screenshots
No screenshots attached; focused tests and the runnable example document the behavior.
Checklist
black .command to format the code base.python docs/scripts/check_api_docs.py).