feat(scene-engine): add text-guided scene editing - #515
Open
MuziWong wants to merge 29 commits into
Open
Conversation
…mentation client)
…cs and tests may not be updated
…ption, and add check code
…ption in objects' names
…but no real-world scale...
Greptile SummaryThe PR adds text-guided scene editing, including edit planning, generated-asset preparation, graph-guided layout, import/export support, service configuration, documentation, and tests. The current implementation still leaves filesystem containment and multi-operation planar-constraint correctness issues unresolved. Confidence Score: 2/5The PR is not yet safe to merge because unchecked generated asset identifiers can escape the staging directory and chained planar moves can silently lose requested constraints. VLM-derived categories remain embedded in image and mask output paths before a safe identifier check, while sequential planar relation updates clear relations appended earlier in the same batch. Files Needing Attention: embodichain/gen_sim/scene_engine/pipeline/editing/scene_edit_understanding.py, embodichain/gen_sim/scene_engine/pipeline/editing/scene_edit_asset_preparation.py, embodichain/gen_sim/scene_engine/core/scene_graph.py
|
| Filename | Overview |
|---|---|
| embodichain/gen_sim/scene_engine/pipeline/editing/scene_edit_understanding.py | Converts VLM drafts into edit operations, but generated add IDs still preserve path components from category text. |
| embodichain/gen_sim/scene_engine/pipeline/editing/scene_edit_asset_preparation.py | Generates and segments added assets, with image and mask paths remaining vulnerable to unchecked generated IDs. |
| embodichain/gen_sim/scene_engine/core/scene_graph.py | Introduces support and planar graph updates, but chained planar moves can silently discard an earlier requested constraint. |
| embodichain/gen_sim/scene_engine/core/scene_edit_plan.py | Adds normalized operation validation for add, move, and delete edits against the pre-edit scene. |
| embodichain/gen_sim/scene_engine/pipeline/edit.py | Orchestrates import, instruction understanding, asset preparation, layout generation, and re-export. |
| embodichain/gen_sim/scene_engine/pipeline/utils/scene_importer.py | Imports exported scene metadata and validates scene graphs before editing. |
Reviews (7): Last reviewed commit: "replace the assets gravity settler with ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+24
to
+29
| from embodichain.gen_sim.scene_engine.configs.environment import ( | ||
| read_scene_engine_env_values, | ||
| ) | ||
|
|
||
|
|
||
| class ImageGenerationClient: |
There was a problem hiding this comment.
This new public module does not define __all__, leaving its intended API ambiguous and allowing wildcard imports to expose incidental imported names.
Suggested change
| from embodichain.gen_sim.scene_engine.configs.environment import ( | |
| read_scene_engine_env_values, | |
| ) | |
| class ImageGenerationClient: | |
| from embodichain.gen_sim.scene_engine.configs.environment import ( | |
| read_scene_engine_env_values, | |
| ) | |
| __all__ = ["ImageGenerationClient"] | |
| class ImageGenerationClient: |
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: 24-29
Comment:
**Public client lacks export declaration**
This new public module does not define `__all__`, leaving its intended API ambiguous and allowing wildcard imports to expose incidental imported names.
```suggestion
from 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!
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a text-guided scene editing workflow to the Scene Engine pipeline, extending exported artifacts to include an explicit SceneGraph representation and edit planning so scenes can be generated, edited, and re-exported in-place.
Changes:
- Introduces edit-time core structures (
SceneGraph,SceneEditPlan) and editing pipeline stages (understanding → asset prep → layout optimization → export). - Adds an image-generation client and updates segmentation config semantics, plus CLI support for
--edit_prompt(edit-only, generate-only, or generate-then-edit). - Persists additional spatial metadata (table support contour + conservative optimization rectangle, object
center_xy) into exports and import flows; updates docs and expands test coverage.
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/gen_sim/scene_engine/test_simready_processor_utils.py | Adds regression coverage for VLM-footprint-driven uniform scaling. |
| tests/gen_sim/scene_engine/test_scene_understanding.py | Updates imports and adds validation tests for location words in descriptions + initial scene graph initialization. |
| tests/gen_sim/scene_engine/test_scene_layout_optimizer.py | Adds tests for table 9-grid mapping and stacked parent/child placement. |
| tests/gen_sim/scene_engine/test_scene_graph.py | Adds validation/normalization tests for graph layers, planar relations, and serialization. |
| tests/gen_sim/scene_engine/test_scene_engine_config.py | Adds CLI tests for edit-only and generate-then-edit behavior. |
| tests/gen_sim/scene_engine/test_scene_edit.py | Adds tests for importing editable exports and validating edit prerequisites. |
| tests/gen_sim/scene_engine/test_scene_edit_plan.py | Adds comprehensive tests for edit operation parsing, validation, graph updates, and asset prep. |
| tests/gen_sim/scene_engine/test_scene_core_and_export.py | Extends export tests to cover scene_graph.json, scene.json, center_xy, and import round-trips. |
| tests/gen_sim/scene_engine/test_clients.py | Adds image-generation client tests and updates segmentation endpoint/config keys. |
| embodichain/gen_sim/scene_engine/pipeline/utils/table_support_surface.py | Persists a conservative optimization rectangle derived from the detected tabletop contour. |
| embodichain/gen_sim/scene_engine/pipeline/utils/simready_processor.py | Reworks SimReady processing to optionally query a VLM for rotation/scale and to persist support geometry. |
| embodichain/gen_sim/scene_engine/pipeline/utils/simready_processor_utils.py | Adds rendering + VLM query helpers and GLB rotation/scale utilities for VLM-based transforms. |
| embodichain/gen_sim/scene_engine/pipeline/utils/scene_layout_optimizer.py | Adds numerical optimization utilities for graph-constrained tabletop/stacked layouts. |
| embodichain/gen_sim/scene_engine/pipeline/utils/scene_layout_constructor.py | Adds orchestration for applying layout optimization across table-root and stacked groups. |
| embodichain/gen_sim/scene_engine/pipeline/utils/scene_importer.py | Adds importer for existing Scene Engine exports (scene + graph) to enable edit-only flows. |
| embodichain/gen_sim/scene_engine/pipeline/utils/scene_exporter.py | Extends export to write scene_graph.json + scene.json and cleans up stale mesh assets. |
| embodichain/gen_sim/scene_engine/pipeline/utils/image_segmentation_utils.py | Adds heuristic mask inversion for generated single-object images and related utilities. |
| embodichain/gen_sim/scene_engine/pipeline/generation/scene_understanding.py | Returns (Scene, SceneGraph) and tightens semantic constraints to exclude location words in descriptions. |
| embodichain/gen_sim/scene_engine/pipeline/generation/scene_generation.py | Wires graph validation through generation/refinement and reuses persisted support geometry for refinement. |
| embodichain/gen_sim/scene_engine/pipeline/generation/init.py | Initializes generation subpackage. |
| embodichain/gen_sim/scene_engine/pipeline/generate.py | Updates generation entrypoint to pass segmentation client and export the scene graph. |
| embodichain/gen_sim/scene_engine/pipeline/editing/scene_edit_understanding.py | Adds VLM-to-edit-plan parsing and graph update application utilities. |
| embodichain/gen_sim/scene_engine/pipeline/editing/scene_edit_layout_generation.py | Adds layout optimization stage for applying edit plan results. |
| embodichain/gen_sim/scene_engine/pipeline/editing/scene_edit_asset_preparation.py | Adds image→mask→geometry→SimReady asset generation for add operations. |
| embodichain/gen_sim/scene_engine/pipeline/editing/init.py | Initializes editing subpackage. |
| embodichain/gen_sim/scene_engine/pipeline/edit.py | Adds edit pipeline entrypoint and re-export of edited outputs. |
| embodichain/gen_sim/scene_engine/core/scene_object.py | Extends SceneObject with center_xy and table support geometry fields for edits/layout. |
| embodichain/gen_sim/scene_engine/core/scene_graph.py | Adds SceneGraph data model with validation, normalization, and update application. |
| embodichain/gen_sim/scene_engine/core/scene_edit_plan.py | Adds validated edit operation and plan data structures. |
| embodichain/gen_sim/scene_engine/clients/image_segmentation.py | Renames segmentation endpoint config to “by prompt” and updates request path usage. |
| embodichain/gen_sim/scene_engine/clients/image_generation.py | Adds image-generation service client and dotenv configuration loader. |
| embodichain/gen_sim/scene_engine/cli/start.py | Adds --edit_prompt and supports edit-only / generate-then-edit invocation. |
| docs/source/features/generative_sim/scene_engine.md | Documents edit workflow, new dotenv keys, and new export artifacts. |
Suppressed comments (1)
embodichain/gen_sim/scene_engine/pipeline/utils/simready_processor.py:251
_vlm_transform_for_objectwrites debug artifacts undersimready_geometry_root.parent / "debug"even when the caller provideddebug_output_root. This makes outputs harder to control/relocate and can create unexpected files during batch runs.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+385
to
+406
| best_rectangle: Polygon | None = None | ||
| best_area = 0.0 | ||
| 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 |
…tate for assets who need to be calibrated
…libration in image-conditioned scene engine pipeline (but only calibrated the upright bottle-like assets currently)
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated no new comments.
Suppressed comments (5)
embodichain/gen_sim/scene_engine/pipeline/utils/image_segmentation_utils.py:340
- Mask files are opened without being closed (
Image.open(mask_path)); since the convertedImageobjects are retained indecoded_masks, this can keep file handles open longer than intended. Open masks with a context manager and.copy()the converted image before storing it.
mask = Image.open(mask_path).convert("L")
_require_image_size(mask, image.size)
decoded_masks.append((asset_id, mask))
embodichain/gen_sim/scene_engine/pipeline/utils/scene_layout_constructor.py:368
- Using
list.pop(0)for the BFS queue makes group construction O(n^2) in the number of nodes. Switchingpendingto acollections.dequekeeps this O(n).
pending = [TABLE_OBJECT_ID]
while pending:
parent_id = pending.pop(0)
embodichain/gen_sim/scene_engine/pipeline/utils/simready_processor.py:247
- This prompt string has a grammatical error ("needs to be place" → "needs to be placed"), which can reduce clarity for VLM prompting and for future maintainers reading the code.
embodichain/gen_sim/scene_engine/pipeline/utils/image_segmentation_utils.py:327 Image.open(...)is used without a context manager here; the underlying file handle may remain open until the image object is GC'd, which can leak descriptors in long-running pipelines. Usewith Image.open(...)and.copy()after.convert()to fully detach from the file.
This issue also appears on line 338 of the same file.
image = Image.open(image_path).convert("RGBA")
overlay = Image.new("RGBA", image.size, (0, 0, 0, 0))
embodichain/gen_sim/scene_engine/pipeline/utils/scene_layout_constructor.py:215
- Using
list.pop(0)in a loop is O(n) per iteration and can make descendant propagation quadratic as the number of nodes grows. Using acollections.dequemakes the BFS queue operations O(1).
This issue also appears on line 366 of the same file.
pending = list(children_by_parent.get(root_id, []))
while pending:
descendant_id = pending.pop(0)
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated no new comments.
Suppressed comments (4)
embodichain/gen_sim/scene_engine/pipeline/utils/simready_processor.py:247
- The
needed_layoutprompt string has grammatical issues ("needs to be place") and theuse_scale/use_rotationparameters are immediately deleted, which obscures intent.
embodichain/gen_sim/scene_engine/pipeline/utils/table_support_surface.py:405 _largest_inscribed_rectangledoes up to ~5Mpolygon.covers(rectangle)checks (48^4 worst-case), and each covers test can be expensive. Preparing the polygon once can significantly reduce the per-rectangle predicate cost.
area = rectangle.area
if area > best_area and polygon.covers(rectangle):
best_rectangle = rectangle
embodichain/gen_sim/scene_engine/pipeline/utils/simready_processor.py:215
- VLM transform flags are effectively ignored here:
rotate_about_xand the baked GLB rotation should be conditioned onuse_vlm_rotation, and the code currently hard-fails when Blenderbpyis unavailable (common in non-Blender runtimes), which breaks scene-edit add flows.
embodichain/gen_sim/scene_engine/pipeline/utils/image_segmentation_utils.py:339 render_asset_mask_id_overlayopens images/masks without a context manager. On some platforms this can leave file descriptors open longer than needed (and can prevent deleting temp directories on Windows).
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)
… follow the assets' orientation_states
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated no new comments.
Suppressed comments (3)
embodichain/gen_sim/scene_engine/pipeline/generation/scene_understanding.py:71
- The scene-understanding VLM prompt still instructs the model that
namemust not contain spatial words like “left”/“right”, but the parser/tests now accept names containing those words (e.g. "left cup"). This leaves the contract fornameambiguous (prompt says “never”, validation allows it), which can make VLM outputs less consistent and harder to debug.
Consider aligning these by either (a) relaxing the prompt’s name rules/examples to match the accepted schema, or (b) reinstating validation with a narrower rule that only rejects true positional/relational phrasing (while allowing structural words like “top”).
embodichain/gen_sim/scene_engine/pipeline/utils/table_support_surface.py:391
_largest_inscribed_rectangledoes a brute-force search over pairs of unique x/y coordinates withpolygon.covers(rectangle)inside the innermost loop. Even with the 48-point cap, this is up to ~48^4 (~5.3M) rectangle checks, which can be very slow given Shapely predicate costs and will run during table processing.
Consider replacing this with a more efficient heuristic/algorithm (e.g., iterative shrink from bounds, grid search over a much smaller candidate set, or an optimization-based approach), or lowering the search bound further and documenting the accuracy/performance tradeoff.
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]):
embodichain/gen_sim/scene_engine/pipeline/utils/image_segmentation_utils.py:340
render_asset_mask_id_overlayopens the source image and each mask withImage.open(...).convert(...)but never closes the underlying file handles. In long-running processes or scenes with many assets, this can leak file descriptors.
Prefer with Image.open(...) as img: / as mask: (or explicitly calling .close() after .convert()), especially since this code is used in the main scene-understanding path.
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)
decoded_masks.append((asset_id, mask))
Comment on lines
+232
to
+236
| if not support.equals(rectangle): | ||
| raise ValueError( | ||
| "Initial AABB projection requires an axis-aligned rectangular " | ||
| "support region." | ||
| ) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated no new comments.
Suppressed comments (2)
embodichain/gen_sim/scene_engine/pipeline/generation/scene_understanding.py:71
- The scene-understanding system prompt still states that name/description must not contain spatial/location terms (e.g., “left”), but the parser no longer enforces that constraint (and tests now expect such values to be accepted). This mismatch makes the prompt misleading during debugging/tuning because the true contract is now “best-effort guidance” rather than a hard validation rule. Consider relaxing the prompt wording to “prefer/avoid” instead of “must not”, or reintroduce the validator if strictness is still desired.
embodichain/gen_sim/scene_engine/pipeline/utils/table_support_surface.py:383 - The largest-inscribed-rectangle search is still worst-case O(n^4) over the sampled x/y coordinate sets (up to 48 each => ~5.3M rectangles), and each iteration calls
polygon.covers(...)which is relatively expensive. On complex tabletops this can become a noticeable startup cost during SimReady table processing. Consider either (a) reducing the candidate grid further/adaptively, (b) adding early pruning based on current best area, or (c) switching to a more direct/max-rectangle-in-polygon strategy.
# Keep the search bounded for highly tessellated support contours.
if len(x_values) > 48:
x_values = x_values[np.linspace(0, len(x_values) - 1, 48, dtype=int)]
if len(y_values) > 48:
y_values = y_values[np.linspace(0, len(y_values) - 1, 48, dtype=int)]
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 41 changed files in this pull request and generated no new comments.
Suppressed comments (3)
embodichain/gen_sim/scene_engine/pipeline/generation/scene_generation.py:401
- The table support geometry persisted on
scene.table(support_contour_xy / support_optimization_rect_xy) is produced during SimReady processing in the table’s local z-up frame, but here it is consumed as if it were in z-up world coordinates while the asset AABBs are measured in z-up world. This can cause clamping/overlap optimization to use the wrong support region whenever the table has a non-identity pose (translation/rotation). Consider transforming the persisted contour/rectangle into z-up world usingrefined_table_layoutbefore building the Shapely polygons (and likewise ensuresupport_surface_zis in the same frame used for placement during edits).
embodichain/gen_sim/scene_engine/pipeline/utils/simready_processor.py:177 TableSupportSurfaceDetectoris fed with_z_up_table_mesh(simready_mesh), which is a canonicalized table mesh in table-local coordinates (no coarse/world pose applied). The resultingsupport_surface_z/support_contour_xy/support_optimization_rect_xystored on theSceneObjectwill therefore be table-local, but other pipeline stages treat these as world-frame quantities. Either apply the table’s pose when detecting support geometry, or clearly persist it as local and require consumers to transform it before use.
embodichain/gen_sim/scene_engine/pipeline/utils/scene_layout_constructor.py:88output_rootis stored onSceneLayoutConstructorbut never used anywhere in this class, and callers (e.g., edit_layout) create/delete a stage directory for it. If no debug artifacts are written here, consider removingoutput_root(and the stage directory management) to reduce API surface and filesystem churn, or plumb it through to optimizers that emit debug outputs.
self.layout_variable_ids = layout_variable_ids
self.generated_scene_objects = generated_scene_objects
self.output_root = Path(output_root).expanduser().resolve()
# Table surface optimizer.
self.table_surface_layout_optimizer = TableSurfaceLayoutOptimizer(
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.
Description
This PR adds a text-guided scene editing workflow to Scene Engine.
Main changes
SceneGraphandSceneEditPlancore data structures for tabletop support hierarchies, 9-grid tableregions, planar relations, and edit validation.
--edit_promptsupport:--imageonly: generate a scene.--edit_promptonly: edit an existing valid Scene Engine export.add,move, anddeleteoperations.scene_graph.json,scene.json,category, andname.scene_exportdirectory, copy new assets, and remove deleted assetdirectories.
y, back is smallery.Dependencies: No new Python package dependencies.
Issue reference: N/A
Type of change
Validation
black --check --diff --color .pytest tests/gen_sim/scene_engine -q(73 passed)Screenshots
Not applicable.
Checklist