Uh oh!
There was an error while loading. Please reload this page.
feat(scene-engine): add generated-scene authoring and editing - #532
feat(scene-engine): add generated-scene authoring and editing#532skywhite1024 wants to merge 2 commits into
Conversation
Greptile SummaryThe PR introduces generated-scene authoring and editing APIs, versioned authoring graphs, portable import/export, service boundaries, and layout processing.
Confidence Score: 3/5The PR does not yet appear safe to merge because non-add edits remain coupled to unavailable asset services and edited exports can corrupt object coordinate frames. Move-only and delete-only edits still initialize and health-check services they never use, while the importer retains the export-frame rotation and the edit exporter applies that rotation again. Files Needing Attention: embodichain/gen_sim/scene_engine/pipeline/api.py, embodichain/gen_sim/scene_engine/pipeline/utils/scene_importer.py, embodichain/gen_sim/scene_engine/pipeline/utils/scene_exporter.py
|
| Filename | Overview |
|---|---|
| embodichain/gen_sim/scene_engine/pipeline/api.py | Adds the public generation/edit orchestration boundaries, but move/delete edits still require unrelated asset services. |
| embodichain/gen_sim/scene_engine/pipeline/utils/scene_importer.py | Adds portable scene import, but does not invert the exporter's additional 180-degree frame rotation before re-export. |
| embodichain/gen_sim/scene_engine/pipeline/utils/scene_exporter.py | Extends versioned scene export and authoring metadata while retaining the rotation that is reapplied during edit cycles. |
| embodichain/gen_sim/scene_engine/pipeline/editing/scene_edit_understanding.py | Adds edit parsing and now constrains generated add categories to filename-safe identifiers. |
| embodichain/gen_sim/scene_engine/core/scene_graph.py | Introduces the versioned authoring-only GeneratedSceneGraph model and stable identifier validation. |
Reviews (3): Last reviewed commit: "Merge branch 'ljd/gen-sim-atomic-prerequ..." | Re-trigger Greptile
Uh oh!
There was an error while loading. Please reload this page.
| image_generation = image_generation_client or ImageGenerationClient.from_dotenv() | ||
| geometry = geometry_generation_client or GeometryGenerationClient.from_dotenv() | ||
| segmentation = image_segmentation_client or ImageSegmentationClient.from_dotenv() | ||
| owned_clients = ( | ||
| (image_generation, image_generation_client is None), | ||
| (geometry, geometry_generation_client is None), | ||
| (segmentation, image_segmentation_client is None), | ||
| ) | ||
| log_info("Starting Objects Preparation") | ||
| try: | ||
| for client, _ in owned_clients: | ||
| client.check_health() |
There was a problem hiding this comment.
Unused services gate all edits
When a move-only or delete-only edit runs while any asset-generation service is unconfigured or unavailable, materialize_edit still constructs and health-checks all three clients before reaching the no-add early return, causing a valid edit to fail before layout.
Prompt To Fix With AI
This is a comment left during a code review.
Path: embodichain/gen_sim/scene_engine/pipeline/api.py
Line: 255-266
Comment:
**Unused services gate all edits**
When a move-only or delete-only edit runs while any asset-generation service is unconfigured or unavailable, `materialize_edit` still constructs and health-checks all three clients before reaching the no-add early return, causing a valid edit to fail before layout.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| scene_config_path = SceneExporter( | ||
| scene=scene, | ||
| scene_graph=scene_graph, | ||
| output_root=output_root, | ||
| ).export() |
There was a problem hiding this comment.
Edit exports rotate coordinates again
When an existing export is edited, the importer retains coordinates in the additional 180-degree export frame and this call constructs SceneExporter with the same rotation enabled again, causing unchanged positions and XY metadata to flip between frames across edit cycles.
Prompt To Fix With AI
This is a comment left during a code review.
Path: embodichain/gen_sim/scene_engine/pipeline/api.py
Line: 304-308
Comment:
**Edit exports rotate coordinates again**
When an existing export is edited, the importer retains coordinates in the additional 180-degree export frame and this call constructs `SceneExporter` with the same rotation enabled again, causing unchanged positions and XY metadata to flip between frames across edit cycles.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| from embodichain.gen_sim.scene_engine.configs.environment import ( | ||
| read_scene_engine_env_values, | ||
| ) | ||
| class ImageGenerationClient: |
There was a problem hiding this comment.
Public module lacks export declaration
The new public client module defines ImageGenerationClient without __all__, leaving its intended API implicit and exposing helper symbols inconsistently with the repository's public-module convention.
| fromembodichain.gen_sim.scene_engine.configs.environmentimport ( | |
| read_scene_engine_env_values, | |
| ) | |
| classImageGenerationClient: | |
| fromembodichain.gen_sim.scene_engine.configs.environmentimport ( | |
| read_scene_engine_env_values, | |
| ) | |
| __all__= ["ImageGenerationClient"] | |
| classImageGenerationClient: |
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: embodichain/gen_sim/scene_engine/clients/image_generation.py
Line: 26-31
Comment:
**Public module lacks export declaration**
The new public client module defines `ImageGenerationClient` without `__all__`, leaving its intended API implicit and exposing helper symbols inconsistently with the repository's public-module convention.
```suggestionfrom embodichain.gen_sim.scene_engine.configs.environment import ( read_scene_engine_env_values,)__all__ = ["ImageGenerationClient"]class ImageGenerationClient:```**Context Used:** CLAUDE.md ([source](https://github.com/dexforce/embodichain/blob/main/CLAUDE.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Pull request overview
This PR refactors the GenSim Scene Engine pipeline into auditable “blueprint” stage boundaries, adds first-class scene-graph + edit flows, and expands export/import + layout utilities to support deterministic generation and iterative editing.
Changes:
- Introduces
pipeline.apiwithanalyze_*(blueprint capture) andmaterialize_*(generation/materialization) entry points, and updates CLI/generate/edit entry points to use them. - Adds/extends core data structures and utilities:
SceneGraph,SceneEditPlan, export/import of scene+graph, table support-surface metadata, and new layout construction helpers. - Adds new service clients (image generation) and updates existing clients (segmentation endpoint naming, geometry seed handling), with broad unit test coverage.
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/gen_sim/scene_engine/test_simready_processor_utils.py | Adds unit coverage for VLM-driven SimReady scaling behavior. |
| tests/gen_sim/scene_engine/test_scene_understanding.py | Updates/extends scene understanding tests (mask overlays, orientation states, parsing rules). |
| tests/gen_sim/scene_engine/test_scene_layout_optimizer.py | Adds tests for table-region bounds and stacked placement behavior. |
| tests/gen_sim/scene_engine/test_scene_graph.py | Adds validation/serialization/constraint derivation tests for scene graphs. |
| tests/gen_sim/scene_engine/test_scene_generation.py | Adds calibration tests for scene-graph-conditioned orientation handling. |
| tests/gen_sim/scene_engine/test_scene_engine_config.py | Extends CLI behavior tests (generate vs edit vs both). |
| tests/gen_sim/scene_engine/test_scene_edit.py | Adds import/edit validation tests for existing exports. |
| tests/gen_sim/scene_engine/test_scene_edit_plan.py | Adds comprehensive tests for edit-plan parsing, validation, asset prep, and graph updates. |
| tests/gen_sim/scene_engine/test_scene_core_and_export.py | Expands export/import tests (graph export, global z-rotation, support metadata). |
| tests/gen_sim/scene_engine/test_pipeline_api.py | Adds end-to-end tests for blueprint persistence and immutability during materialization. |
| tests/gen_sim/scene_engine/test_clients.py | Updates client tests (segmentation endpoint rename, new image generation client, seed propagation). |
| embodichain/gen_sim/scene_engine/pipeline/utils/table_support_surface.py | Adds optimization rectangle computation + debug visualization for support regions. |
| embodichain/gen_sim/scene_engine/pipeline/utils/simready_processor.py | Refactors SimReady processing to support SceneGraph-conditioned transforms and table support-surface detection. |
| embodichain/gen_sim/scene_engine/pipeline/utils/scene_layout_utils.py | Adds shared pose/mesh utilities for y-up↔z-up placement and AABB measurement. |
| embodichain/gen_sim/scene_engine/pipeline/utils/scene_layout_constructor.py | Introduces graph-driven layout construction over table + stacked parents. |
| embodichain/gen_sim/scene_engine/pipeline/utils/scene_importer.py | Adds importer for exported scene+graph back into editable in-memory state. |
| embodichain/gen_sim/scene_engine/pipeline/utils/scene_exporter.py | Extends export to include graph + rotated export frame + metadata transforms + stale asset cleanup. |
| embodichain/gen_sim/scene_engine/pipeline/utils/image_segmentation_utils.py | Adds mask inversion heuristic and asset-ID overlay rendering utilities. |
| embodichain/gen_sim/scene_engine/pipeline/utils/gravity_settler.py | Adds generalized gravity settling for dynamic/static participants using Lab simulation. |
| embodichain/gen_sim/scene_engine/pipeline/utils/assets_group_layout_optimizer.py | Adjusts initial constraint handling by projecting AABBs into a rectangular support region. |
| embodichain/gen_sim/scene_engine/pipeline/utils/assets_gravity_settler.py | Removes legacy asset gravity settling implementation (superseded by GravitySettler). |
| embodichain/gen_sim/scene_engine/pipeline/generation/scene_understanding.py | Refactors understanding to return (Scene, SceneGraph) and adds orientation-state querying. |
| embodichain/gen_sim/scene_engine/pipeline/generation/init.py | Adds package init for generation pipeline module. |
| embodichain/gen_sim/scene_engine/pipeline/generate.py | Switches generation entrypoint to blueprint API (analyze_image → materialize_blueprint). |
| embodichain/gen_sim/scene_engine/pipeline/editing/scene_edit_layout_generation.py | Adds edit-time layout dispatch driven by updated goal scene graph. |
| embodichain/gen_sim/scene_engine/pipeline/editing/scene_edit_asset_preparation.py | Adds image→mask→geometry pipeline for newly added assets during editing. |
| embodichain/gen_sim/scene_engine/pipeline/editing/init.py | Adds package init for editing pipeline module. |
| embodichain/gen_sim/scene_engine/pipeline/edit.py | Adds edit entrypoint using blueprint API (analyze_edit → materialize_edit). |
| embodichain/gen_sim/scene_engine/pipeline/api.py | Introduces auditable blueprint/materialization API + manifest hashing + artifact recording. |
| embodichain/gen_sim/scene_engine/pipeline/init.py | Exposes new blueprint/materialization API from the pipeline package. |
| embodichain/gen_sim/scene_engine/errors.py | Adds SceneServiceError for consistent transient/remote failure signaling. |
| embodichain/gen_sim/scene_engine/core/scene_object.py | Extends scene object schema with center/support metadata used by layout/export/import. |
| embodichain/gen_sim/scene_engine/core/scene_edit_plan.py | Adds edit operation + plan model with validation and serialization. |
| embodichain/gen_sim/scene_engine/clients/image_segmentation.py | Renames segmentation endpoint configuration key/field to “by prompt”. |
| embodichain/gen_sim/scene_engine/clients/image_generation.py | Adds image generation client with seed acknowledgment and PNG validation. |
| embodichain/gen_sim/scene_engine/clients/geometry_generation.py | Adds seed propagation + unified SceneServiceError and response seed verification. |
| embodichain/gen_sim/scene_engine/cli/start.py | Extends CLI to support edit-only, generate-only, or generate+edit flows. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| outside_pixel_count = width * height - center_mask.width * center_mask.height | ||
| outside_foreground_ratio = outside_foreground_pixels / outside_pixel_count | ||
| if center_foreground_ratio >= outside_foreground_ratio: | ||
| return candidate |
| image = Image.open(image_path).convert("RGBA") | ||
| overlay = Image.new("RGBA", image.size, (0, 0, 0, 0)) | ||
| colors = ( | ||
| (239, 83, 80, 255), | ||
| (66, 165, 245, 255), | ||
| (102, 187, 106, 255), | ||
| (255, 202, 40, 255), | ||
| (171, 71, 188, 255), | ||
| (38, 198, 218, 255), | ||
| ) | ||
| decoded_masks: list[tuple[str, Image.Image]] = [] | ||
| for index, (asset_id, mask_path) in enumerate(asset_masks): | ||
| mask = Image.open(mask_path).convert("L") | ||
| _require_image_size(mask, image.size) |
| for x_index, minimum_x in enumerate(x_values[:-1]): | ||
| for maximum_x in x_values[x_index + 1 :]: | ||
| if maximum_x <= minimum_x: | ||
| continue | ||
| for y_index, minimum_y in enumerate(y_values[:-1]): | ||
| for maximum_y in y_values[y_index + 1 :]: | ||
| if maximum_y <= minimum_y: | ||
| continue | ||
| rectangle = Polygon( | ||
| [ | ||
| (minimum_x, minimum_y), | ||
| (maximum_x, minimum_y), | ||
| (maximum_x, maximum_y), | ||
| (minimum_x, maximum_y), | ||
| ] | ||
| ) | ||
| area = rectangle.area | ||
| if area > best_area and polygon.covers(rectangle): | ||
| best_rectangle = rectangle | ||
| best_area = area |
| def y_up_to_z_up_matrix() -> np.ndarray: | ||
| """Return the coordinate transform used by layout and export stages.""" | ||
| matrix = np.eye(4) | ||
| matrix[:3, :3] = np.array([[1.0, 0.0, 0.0], [0.0, 0.0, -1.0], [0.0, 1.0, 0.0]]) |
| def _y_up_to_z_up_matrix() -> np.ndarray: | ||
| """Return the coordinate conversion used by Scene Engine layouts.""" | ||
| matrix = np.eye(4) | ||
| matrix[:3, :3] = np.array([[1.0, 0.0, 0.0], [0.0, 0.0, -1.0], [0.0, 1.0, 0.0]]) |
| rot: list[float] | None = None # Final y-up Euler XYZ rotation in degrees. | ||
| pos: list[float] | None = None # Final y-up world position in metres. | ||
| scale: list[float] | None = None # Final y-up object scale. | ||
| center_xy: list[float] | None = None # Z-up table-frame XY AABB center. |
c45034a to
a4afa0dComparea4afa0d to
4de2335CompareThere was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 44 changed files in this pull request and generated 1 comment.
Suppressed comments (9)
embodichain/gen_sim/scene_engine/pipeline/generation/scene_generation.py:399
- The table-only path returns before
_update_scene_final_y_up_layout_and_z_up_centers(). Consequently the table'srot,pos, andscaleremainNoneeven thoughSimReadyProcessorproduced a valid table layout, and the subsequentSceneExporterrejects the scene because_scene_vector()requires all three final vectors. Update the table scene object before returning from this early branch.
embodichain/gen_sim/scene_engine/pipeline/generation/scene_generation.py:274 - For edit/import paths where the table has a non-zero XY rotation, these support points are still in the table-local z-up frame, but only
table_center_xyis added here. The final clamp and overlap optimizer then interpret an unrotated contour/rectangle in world XY, producing incorrect bounds and potentially placing assets outside the actual tabletop. Apply the table's complete final XY transform to both metadata arrays (or explicitly normalize the table frame before this stage).
embodichain/gen_sim/scene_engine/pipeline/utils/image_segmentation_utils.py:115 - For masks up to 5 pixels wide/high, the 1/6 crop still covers the entire image, so
outside_pixel_countis zero and this division raisesZeroDivisionError. This helper is otherwise intended to validate arbitrary server masks; return the original candidate (or use a defined fallback) when there is no outside region.
outside_pixel_count = width * height - center_mask.width * center_mask.height
outside_foreground_ratio = outside_foreground_pixels / outside_pixel_count
embodichain/gen_sim/scene_engine/pipeline/utils/scene_exporter.py:75
- The default 180° export rotation is applied to object poses and
center_xy/support metadata, but the semantic graph is written unchanged andSceneExportImporternever records or reverses this transform. After exporting then editing, table regions andleft_of/in_front_ofrelations no longer describe the imported coordinates, so new/moved objects can be placed in the opposite region/direction. Persist the export transform and undo it during import, or keep authoring coordinates unrotated in the editable export.
Scene layouts are y-up. The simulator automatically converts each y-up
GLB to z-up, so this exporter copies each GLB unchanged and converts
only its world position and rotation for ``init_pos`` and ``init_rot``.
The default applies one additional 180-degree global z-up rotation about
the table center to every object and XY layout metadata. ``body_scale``
embodichain/gen_sim/scene_engine/pipeline/utils/simready_processor.py:235
- When
use_vlm_scale=Truebutuse_vlm_rotation=False(a supported configuration), the scale is computed withrotate_about_x=rotate_about_x, even though the laterrotate_glb_about_x_axiscall does not apply that rotation. The resulting XY footprint is therefore scaled for a pose that is never used. Compute the effective applied-rotation flag first and use it for both scale calculation and mesh rotation.
embodichain/gen_sim/scene_engine/clients/image_segmentation.py:141 - This now makes
ImageSegmentationClient.from_dotenv()requireSCENE_ENGINE_IMAGE_SEGMENTATION_BY_PROMPT_PATH, but the existing Scene Engine setup instructions still defineSCENE_ENGINE_IMAGE_SEGMENTATION_SINGLE_OBJECT_PATH(docs/source/features/generative_sim/scene_engine.md:46). A documented.envtherefore fails during client construction with a missing-key error; update the configuration documentation/sample in the same change or support the old key as a compatibility alias.
"SCENE_ENGINE_IMAGE_SEGMENTATION_BY_PROMPT_PATH",
embodichain/gen_sim/scene_engine/core/scene_graph.py:143
relationis only aLiteralannotation here and is never checked at runtime. An invalid value such as"adjacent_to"is accepted, then_inverse_planar_relation()silently maps it toin_front_of, sovalidate()can persist a malformed semantic graph instead of rejecting it. Validate membership against the four supported planar relations before normalization.
def __post_init__(self) -> None:
"""Validate local relation fields before graph-level checks."""
_validate_stable_id(self.source_id, field_name="source_id")
_validate_stable_id(self.target_id, field_name="target_id")
if self.source_id == self.target_id:
raise ValueError("relation endpoints must be different.")
embodichain/gen_sim/scene_engine/core/scene_graph.py:247
- Although this method is documented as applying an atomic batch, it mutates
self.nodesandself.relationsbefore validating all additions, parents, and relations. If any later check raises (for example, an unknown parent or a planar conflict), the caller is left with a partially applied graph and cannot safely retry. Apply updates to a copy and commit only afterrefresh()succeeds, or roll back on failure.
# Delete all requested nodes before resolving new parents and relations.
self.nodes = [
node for node in self.nodes if node.object_id not in deleted_object_ids
]
self.relations = [
relation
for relation in self.relations
if relation.source_id not in deleted_object_ids
and relation.target_id not in deleted_object_ids
embodichain/gen_sim/scene_engine/pipeline/api.py:258
- This API eagerly constructs the VLM, image-generation, geometry, and segmentation clients and health-checks all of them even when the edit plan contains only moves/deletes.
prepare_scene_edit_assetsexplicitly skips service use for plans without adds, but this wrapper still requires every.envkey and live service, so a non-generative edit fails unnecessarily. Gate client creation and health checks on whether an add operation is present.
effective_vlm = vlm_client or OpenAICompatibleVLM.from_dotenv()
image_generation = image_generation_client or ImageGenerationClient.from_dotenv()
geometry = geometry_generation_client or GeometryGenerationClient.from_dotenv()
segmentation = image_segmentation_client or ImageSegmentationClient.from_dotenv()
owned_clients = (
| if op == "add": | ||
| if object_id is not None: | ||
| raise ValueError("VLM add operations must set object_id to null.") | ||
| if category is None: | ||
| raise ValueError("VLM add operations must provide a category.") |
…tor-01-scene-engine # Conflicts: # embodichain/gen_sim/scene_engine/clients/image_generation.py # embodichain/gen_sim/scene_engine/core/scene_edit_plan.py # embodichain/gen_sim/scene_engine/core/scene_graph.py # embodichain/gen_sim/scene_engine/pipeline/edit.py # embodichain/gen_sim/scene_engine/pipeline/editing/scene_edit_asset_preparation.py # embodichain/gen_sim/scene_engine/pipeline/editing/scene_edit_layout_generation.py # embodichain/gen_sim/scene_engine/pipeline/editing/scene_edit_understanding.py # embodichain/gen_sim/scene_engine/pipeline/generate.py # embodichain/gen_sim/scene_engine/pipeline/generation/scene_generation.py # embodichain/gen_sim/scene_engine/pipeline/generation/scene_understanding.py # embodichain/gen_sim/scene_engine/pipeline/utils/parent_surface_layout_optimizer.py # embodichain/gen_sim/scene_engine/pipeline/utils/scene_exporter.py # embodichain/gen_sim/scene_engine/pipeline/utils/scene_importer.py # embodichain/gen_sim/scene_engine/pipeline/utils/scene_layout_constructor.py # embodichain/gen_sim/scene_engine/pipeline/utils/scene_layout_utils.py # embodichain/gen_sim/scene_engine/pipeline/utils/simready_processor.py # embodichain/gen_sim/scene_engine/pipeline/utils/table_surface_layout_optimizer.py # tests/gen_sim/scene_engine/test_clients.py # tests/gen_sim/scene_engine/test_scene_core_and_export.py # tests/gen_sim/scene_engine/test_scene_edit_plan.py # tests/gen_sim/scene_engine/test_scene_generation.py # tests/gen_sim/scene_engine/test_scene_graph.py # tests/gen_sim/scene_engine/test_scene_layout_optimizer.py # tests/gen_sim/scene_engine/test_scene_understanding.py # tests/gen_sim/scene_engine/test_simready_processor_utils.py
Description
Stack
ljd/gen-sim-atomic-prerequisiteAdd the generated-scene authoring foundation for the task-first GenSim workflow. This layer owns scene generation and editing, layout and settling, service boundaries, and portable authoring import/export.
Architecture boundary
GeneratedSceneGraphas an authoring-only graph, not live scene state.scene_authoring_evidence.jsonas an explicitaudit_onlysidecar with affordance evidence, GLB metadata and checksums, and physics provenance.SceneRegistry,SceneManifest, providers, pose readers, simulator handles, or execution types. CanonicalSceneManifestconversion remains owned by the feat(task-engine): add orchestration and end-to-end integration #538 integration layer.This layer is independently reviewable and does not depend on Task Engine execution.
Refs #531
Type of change
Validation
pytest -q tests/gen_sim/scene_engine- 89 passed, 9 warningspytest -q tests/docs/test_check_api_docs.py- 8 passedgit diff --check- passedsphinx-buildis unavailable in the validation environmentChecklist