Add scene engine pipeline - #436
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new “scene engine” image-to-tabletop-scene generation pipeline under embodichain/gen_sim/scene_engine, covering semantic scene understanding, image segmentation, coarse geometry/layout generation, heuristic layout refinement + gravity settling, Gym export, and CLI preview tooling. It also standardizes use of VHACD for collision decomposition in the simulation-facing parts of the pipeline.
Changes:
- Added core scene data structures (table/assets/scene) plus stage modules for understanding, segmentation, generation/refinement, and Gym export.
- Added service clients/config loaders for an OpenAI-compatible VLM endpoint, an image-segmentation service, and a geometry-generation service.
- Added CLI entry points to run the pipeline and preview the generated Gym export in
SimulationManager.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| embodichain/gen_sim/scene_engine/utils/logger.py | Adds simple stage logging helpers for the pipeline. |
| embodichain/gen_sim/scene_engine/utils/init.py | Initializes the utils package. |
| embodichain/gen_sim/scene_engine/pipeline/utils/scene_segmentation_utils.py | Implements RLE mask decode/merge and mask visualization helpers. |
| embodichain/gen_sim/scene_engine/pipeline/utils/scene_generation_utils.py | Adds geometry/layout utilities: transforms, table support heuristics, packing, and gravity-settling helpers. |
| embodichain/gen_sim/scene_engine/pipeline/utils/init.py | Initializes pipeline utils package. |
| embodichain/gen_sim/scene_engine/pipeline/scene_understanding.py | VLM-driven semantic parsing + strict JSON validation for table/assets. |
| embodichain/gen_sim/scene_engine/pipeline/scene_segmentation.py | Service-driven segmentation with VLM validation/assignment and mask outputs. |
| embodichain/gen_sim/scene_engine/pipeline/scene_generation.py | Orchestrates coarse geometry download, SimReady conversion, refinement, and scene updates. |
| embodichain/gen_sim/scene_engine/pipeline/gym_export.py | Exports final scene + meshes into a Gym config + mesh assets layout. |
| embodichain/gen_sim/scene_engine/pipeline/generate.py | End-to-end pipeline entrypoint tying all stages together. |
| embodichain/gen_sim/scene_engine/pipeline/init.py | Initializes pipeline package. |
| embodichain/gen_sim/scene_engine/llms/openai_compatible_client.py | Implements an OpenAI-compatible multimodal chat-completions client. |
| embodichain/gen_sim/scene_engine/llms/load_config.py | Loads VLM config with environment-variable overrides. |
| embodichain/gen_sim/scene_engine/llms/init.py | Initializes llms package. |
| embodichain/gen_sim/scene_engine/core/table.py | Defines the Table dataclass and serialization. |
| embodichain/gen_sim/scene_engine/core/scene.py | Defines the Scene dataclass and serialization. |
| embodichain/gen_sim/scene_engine/core/asset.py | Defines the Asset dataclass and serialization. |
| embodichain/gen_sim/scene_engine/core/init.py | Initializes core package. |
| embodichain/gen_sim/scene_engine/configs/scene_engine_config.json | Adds a unified config template for VLM/segmentation/geometry services. |
| embodichain/gen_sim/scene_engine/configs/init.py | Initializes configs package. |
| embodichain/gen_sim/scene_engine/clients/image_segmentation.py | Adds image segmentation service client + config loader. |
| embodichain/gen_sim/scene_engine/clients/geometry_generation.py | Adds geometry generation service client + response parsing + downloads. |
| embodichain/gen_sim/scene_engine/clients/init.py | Initializes clients package. |
| embodichain/gen_sim/scene_engine/cli/start.py | Adds CLI entry to run the full pipeline from an image. |
| embodichain/gen_sim/scene_engine/cli/preview.py | Adds CLI tool to preview exported Gym scenes in simulation. |
| embodichain/gen_sim/scene_engine/cli/init.py | Initializes cli package. |
Comments suppressed due to low confidence (1)
embodichain/gen_sim/scene_engine/pipeline/generate.py:106
- If generate_scene_and_refine() (or check_health()) raises, GeometryGenerationClient.close() is skipped, leaking the underlying requests.Session. Use try/finally to ensure cleanup.
# 3. Objects + Coarse Layout Generation
log_stage_start("Objects + Coarse Layout Generation")
# Load the config and fail if the Geometry Generation Server is unavailable.
geometry_generation_client = GeometryGenerationClient.from_config(
geometry_generation_config_path
)
geometry_generation_client.check_health()
scene = generate_scene_and_refine(
image_path=image_path,
output_root=resolved_output_root,
scene=scene,
vlm_client=vlm_client,
geometry_generation_client=geometry_generation_client,
)
geometry_generation_client.close() # Kill the session.
log_stage_end("Objects + Coarse Layout Generation")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # 2. Scene Segmentation | ||
| log_stage_start("Scene Segmentation") | ||
| # Load the config and fail if the Image Segmentation Server is unavailable. | ||
| image_segmentation_client = ImageSegmentationClient.from_config( | ||
| image_segmentation_config_path | ||
| ) | ||
| image_segmentation_client.check_health() # Error raising will happen internally. | ||
| scene = segment_scene( | ||
| image_path=image_path, | ||
| output_root=resolved_output_root, | ||
| scene=scene, | ||
| vlm_client=vlm_client, | ||
| image_segmentation_client=image_segmentation_client, | ||
| ) | ||
| image_segmentation_client.close() # Kill the session. | ||
| log_stage_end("Scene Segmentation") |
There was a problem hiding this comment.
I have already fixed this part.
| finally: | ||
| sim.destroy() | ||
|
|
There was a problem hiding this comment.
I have already fixed this part.
| rendered_mask = ( # If weuse outline, then need to do some another processings. | ||
| mask if mask_style == "fill" else _mask_outer_outline(mask, image.size) | ||
| ) |
There was a problem hiding this comment.
I have changed weuse into we use.
| if ( | ||
| scene.table.id != "table" | ||
| ): # Currently it will always return true. For we hardcode the table id to "table". | ||
| raise ValueError("Scene table id must be 'table'.") |
There was a problem hiding this comment.
I have already shorten this into one line.
| def generate_scene_from_image( | ||
| image_path: str | Path, | ||
| output_root: str | Path, | ||
| *, | ||
| llm_config_path: str | Path | None = None, | ||
| image_segmentation_config_path: str | Path | None = None, | ||
| geometry_generation_config_path: str | Path | None = None, | ||
| ) -> Scene: | ||
| """Generate the initial core scene state from an input image.""" |
There was a problem hiding this comment.
Thank you for reviewing!
| print("Successfully completed!") | ||
|
|
||
|
|
||
| def main() -> None: |
There was a problem hiding this comment.
[P2] 请将新增命令注册到统一 CLI。当前项目公开入口是 embodichain console script，但 embodichain.__main__.COMMANDS 中没有 Scene Engine 或预览命令；因此安装后用户无法从统一 CLI 发现或调用这里的 main()，只能依赖内部模块路径。请在 COMMANDS 中注册 Scene Engine 和 preview 对应的子命令。
There was a problem hiding this comment.
Resolved by registering scene-engine and preview-scene in embodichain.__main__.COMMANDS.
The unified dispatcher lazily imports the selected command handler and forwards the remaining command-line arguments to its main(argv) function. Both Scene Engine CLI entry points accept the forwarded argv, allowing their own argument parsers to process the expected options instead of relying on the process-wide sys.argv.
Installed users can invoke Scene Engine through the public CLI:
embodichain scene-engine \
--image <image> \
--output_root <output_root> \
--config <scene_engine_config>
embodichain preview-scene \
--output_root <output_root>preview-scene opens the native preview window by default. To use the browser-based Viser preview:
embodichain preview-scene \
--output_root <output_root> \
--viser \
--viser-host 0.0.0.0 \
--viser-port 9000The corresponding module entry points are:
python -m embodichain.gen_sim.scene_engine.cli.start \
--image <image> \
--output_root <output_root> \
--config <scene_engine_config>
python -m embodichain.gen_sim.scene_engine.cli.preview \
--output_root <output_root>I also verified that:
scene-engineandpreview-sceneappear inembodichain --help;embodichain scene-engine --helpandembodichain preview-scene --helpdispatch to their command-specific parsers correctly;preview-scene --helpexposes the required--output_rootargument and the optional Viser-related flags;- the relevant CLI and Scene Engine unit tests pass (
21 passed).
| resolved_output_root = Path(output_root).expanduser().resolve() | ||
| resolved_output_root.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| generate_scene_from_image( |
There was a problem hiding this comment.
[P1] Pass service configuration through the generation CLI. As written, users cannot run this command without editing the JSON installed with the package: this call forwards only the image and output paths, while the packaged service URLs are empty. The LLM endpoint can be overridden through environment variables, but the segmentation and geometry services have no CLI or environment-variable entry point, so generation fails during base_url validation. Please accept a config path or explicit service URL options here and forward them into the pipeline.
There was a problem hiding this comment.
Resolved by adding an optional --config argument to the generation CLI. The provided Scene Engine JSON configuration is propagated to the LLM, image segmentation, and geometry generation clients, allowing users to override service endpoints without modifying the package-installed default configuration.
| for scene_object in scene_objects | ||
| } | ||
| gym_config = { | ||
| "id": f"Prompt2Scene-{int(time.time() * 1000)}-v0", |
There was a problem hiding this comment.
[P1] Export a registered environment ID. embodichain run-env passes this generated ID directly to gymnasium.make, but the codebase does not register any Prompt2Scene-* environment. Consequently, every exported config fails immediately with NameNotFound. Please export an environment ID that is already registered with the runner, or provide and invoke the corresponding registration logic.
There was a problem hiding this comment.
Resolved by changing the artifact from a Gym environment configuration to an explicit scene export (embodichain.scene-export/v1).
The generated identifier is now scene_id, which uniquely identifies the exported scene data. It is not a Gymnasium environment ID and is never passed to gymnasium.make(). The exported scene is loaded through the Scene Engine preview path instead of embodichain run-env, so no Gym environment registration is required.
| "max_episodes": 10, | ||
| "max_episode_steps": 300, | ||
| "env": {"events": {}, "observations": {}, "dataset": {}}, | ||
| "robot": {}, |
There was a problem hiding this comment.
[P1] Supply a valid robot configuration in the Gym export. Even after the environment ID is fixed, EmbodiedEnv parses this empty object as RobotCfg(fpath=None); _setup_robot then receives None from add_robot and proceeds to access robot joints. If this file is intended to load directly through run-env, export a valid robot configuration. Otherwise, target an explicitly scene-only environment that supports running without a robot.
There was a problem hiding this comment.
Resolved by changing the artifact from a Gym environment configuration to an explicit scene export (embodichain.scene-export/v1).
The generated scene does not infer a robot model, robot placement, controller, or sensor setup from the input image, so exporting a default robot configuration would produce an incomplete and potentially misleading task setup. The scene export therefore omits the robot field entirely and is loaded through the Scene Engine preview path, which directly creates a SimulationManager for the table and assets without constructing an EmbodiedEnv.
A downstream task or environment layer can attach an explicit robot configuration when one is available, keeping scene generation independent from task-specific execution.
| destination_glb_path = mesh_assets_root / object_id / f"{object_id}.glb" | ||
| destination_glb_path.parent.mkdir(parents=True, exist_ok=True) | ||
| shutil.copy2(source_glb_path, destination_glb_path) | ||
| return destination_glb_path.relative_to(mesh_assets_root.parent).as_posix() |
There was a problem hiding this comment.
[P1] Emit mesh paths that the Gym loader can resolve. When this config is loaded through run-env, config_to_cfg passes the relative Mesh path to get_data_path, which interprets mesh_assets as a dataset name and raises Dataset class 'mesh_assets' not found. Only the custom preview resolves this path relative to the config directory. Please export an absolute or data-root-relative path, or update the common loader to resolve Mesh paths relative to the config file.
There was a problem hiding this comment.
Resolved by changing this artifact from a run-env Gym configuration to an explicit scene export (embodichain.scene-export/v1). The output is now written as scene_export/scene_config.json and is loaded through the Scene Engine preview path, which resolves mesh_assets/... relative to the config file directory.
It is no longer passed to config_to_cfg() or get_data_path(), so exporting absolute paths or modifying the common Gym loader is unnecessary here. Keeping config-relative paths also preserves portability when the complete scene_export/ directory is moved.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (13)
embodichain/gen_sim/scene_engine/pipeline/utils/scene_generation_utils.py:510
- Calling the private SimulationManager method
_deferred_destroy()bypasses the public teardown path and can become brittle if the internal cleanup mechanism changes. Prefer usingdestroy(exit_process=False)plusSimulationManager.flush_cleanup_queue()to run the deferred cleanup without forcibly exiting the process.
finally:
sim._deferred_destroy()
embodichain/gen_sim/scene_engine/pipeline/utils/scene_generation_utils.py:485
RigidObjectCfg.max_convex_hull_num/acd_methodare deprecated in favor of the shape-levelMeshCfgfields. Moving these toMeshCfg(...)avoids relying on deprecated overrides and keeps the config consistent with the rest of the sim stack.
body_scale=tuple(asset_info["y_up_scale"]),
body_type="dynamic",
max_convex_hull_num=max_convex_hull_num,
acd_method="vhacd", # Use vhacd by default.
embodichain/gen_sim/scene_engine/cli/preview.py:89
SimulationManager.destroy()may callos._exit(0)by default (viaEMBODICHAIN_SIM_EXIT_PROCESS), which is surprising for a CLI preview tool. Also, the sim manager uses deferred cleanup; flushing the cleanup queue avoids leaked C++ resources/segfaults on interpreter shutdown (seeembodichain/lab/sim/sim_manager.py:2676-2797).
finally:
sim.destroy()
embodichain/gen_sim/scene_engine/pipeline/generate.py:88
- If
segment_scene(...)raises, theImageSegmentationClientsession never closes. Wrap usage in atry/finallyso the HTTP session is closed on all error paths.
image_segmentation_client = ImageSegmentationClient.from_config(
image_segmentation_config_path
)
image_segmentation_client.check_health() # Error raising will happen internally.
scene = segment_scene(
image_path=image_path,
output_root=resolved_output_root,
scene=scene,
vlm_client=vlm_client,
image_segmentation_client=image_segmentation_client,
)
image_segmentation_client.close() # Kill the session.
log_stage_end("Scene Segmentation")
embodichain/gen_sim/scene_engine/pipeline/utils/scene_segmentation_utils.py:216
- Spelling/grammar in the inline comment: "weuse" / "some another processings".
rendered_mask = ( # If weuse outline, then need to do some another processings.
mask if mask_style == "fill" else _mask_outer_outline(mask, image.size)
embodichain/gen_sim/scene_engine/pipeline/generate.py:54
- This PR introduces a new end-to-end scene-engine pipeline (VLM parsing/validation, RLE decoding, mask unioning, geometry/layout refinement, gym export) but adds no tests. There is existing gen_sim test coverage (e.g.
tests/gen_sim/simready_pipeline/*), so adding unit tests for the pure functions (JSON schema validators, RLE encode/decode, IoU/union logic, layout transform helpers) and lightweight integration tests with mocked clients would help prevent regressions.
def generate_scene_from_image(
image_path: str | Path,
output_root: str | Path,
*,
llm_config_path: str | Path | None = None,
image_segmentation_config_path: str | Path | None = None,
geometry_generation_config_path: str | Path | None = None,
) -> Scene:
embodichain/gen_sim/scene_engine/cli/preview.py:160
RigidObjectCfg.max_convex_hull_num/acd_methodare deprecated (they override the shape-level settings). Set convex-decomposition settings onMeshCfginstead, so preview matches the supported config path.
max_convex_hull_num=max_convex_hull_num,
acd_method="vhacd", # Use vhacd by default.
)
embodichain/gen_sim/scene_engine/pipeline/utils/scene_generation_utils.py:468
RigidObjectCfg.max_convex_hull_num/acd_methodare deprecated in favor ofMeshCfg.max_convex_hull_num/MeshCfg.acd_method(seeembodichain/lab/sim/cfg.py:963-984). Set these on theMeshCfginstead to match the supported configuration surface.
This issue also appears on line 482 of the same file.
body_scale=tuple(table_y_up_scale),
body_type="static",
max_convex_hull_num=max_convex_hull_num,
acd_method="vhacd", # Use vhacd by default.
embodichain/gen_sim/scene_engine/cli/preview.py:85
- The preview loop opens a window and then only sleeps; if physics is manually updated (the common case), the viewer may not render / respond because the simulation never steps. Stepping once per loop keeps the window responsive and ensures the scene is actually rendered.
This issue also appears on line 158 of the same file.
sim.open_window()
while True:
time.sleep(0.1)
embodichain/gen_sim/scene_engine/pipeline/generate.py:106
- If
generate_scene_and_refine(...)raises, theGeometryGenerationClientsession is left open. Usetry/finallyto guarantee.close()and avoid leaking connections.
geometry_generation_client = GeometryGenerationClient.from_config(
geometry_generation_config_path
)
geometry_generation_client.check_health()
scene = generate_scene_and_refine(
image_path=image_path,
output_root=resolved_output_root,
scene=scene,
vlm_client=vlm_client,
geometry_generation_client=geometry_generation_client,
)
geometry_generation_client.close() # Kill the session.
log_stage_end("Objects + Coarse Layout Generation")
embodichain/gen_sim/scene_engine/clients/geometry_generation.py:67
check_health()ignores the configuredtimeout_sby hardcodingtimeout=10. If the intention is to cap the timeout for health checks, usemin(self._timeout_s, 10)so the config is still respected when it is smaller.
response = self._session.get(
self._url(self._health_path),
# timeout=self._timeout_s,
timeout=10, # Use a shorter timeout for avoiding long waits.
)
embodichain/gen_sim/scene_engine/pipeline/scene_generation.py:188
- Spelling/wording in comments: "assets(includes table)", "seperately".
# Simready all the assets(includes table).
# Treat table and assets seperately.
# Notice that, currently the simready process is only
embodichain/gen_sim/scene_engine/utils/logger.py:22
- Logger naming is inconsistent with the rest of the codebase, which generally uses
logging.getLogger(__name__)(e.g.embodichain/utils/logger.py:24,embodichain/lab/gym/utils/registration.py:41). Using__name__keeps the logger hierarchy aligned with the module path and makes filtering/configuration easier.
_LOGGER = logging.getLogger("embodichain.scene_engine")
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (8)
embodichain/gen_sim/scene_engine/pipeline/utils/scene_generation_utils.py:479
- Same as the table config above:
RigidObjectCfg.max_convex_hull_num/acd_methodare deprecated; for new code setmax_convex_hull_num/acd_methodon theMeshCfgshape so collision decomposition settings live with the mesh (and avoid legacy overrides).
RigidObjectCfg(
uid=asset_id,
shape=MeshCfg(fpath=str(asset_info["mesh_path"])),
init_pos=tuple(rigid_layout["pos"]),
init_rot=tuple(
embodichain/gen_sim/scene_engine/pipeline/utils/scene_generation_utils.py:511
- Calling the private
SimulationManager._deferred_destroy()couples this pipeline to internal cleanup implementation details and bypasses the supporteddestroy()API. Sincedestroy()can already avoidos._exit()viaexit_process=False, prefer the public method and then flush the cleanup queue for deterministic teardown inside the pipeline.
finally:
sim._deferred_destroy()
embodichain/gen_sim/scene_engine/pipeline/scene_understanding.py:180
- The inline comment is incorrect/misleading: since the table id is hardcoded to "table" in
validate_scene_understanding_json, this condition will normally be false (it won't "always return true"). Consider rewording so readers don't misinterpret the control flow.
if (
scene.table.id != "table"
): # Currently it will always return true. For we hardcode the table id to "table".
raise ValueError("Scene table id must be 'table'.")
embodichain/gen_sim/scene_engine/pipeline/utils/scene_segmentation_utils.py:216
- Spelling/grammar in the comment makes it harder to read ("weuse", "processings").
rendered_mask = ( # If weuse outline, then need to do some another processings.
mask if mask_style == "fill" else _mask_outer_outline(mask, image.size)
embodichain/gen_sim/scene_engine/pipeline/utils/scene_generation_utils.py:469
RigidObjectCfg.max_convex_hull_num/acd_methodare marked deprecated inembodichain.lab.sim.cfg.RigidObjectCfg(preferMeshCfg.max_convex_hull_num/MeshCfg.acd_method). Since this is new code, set these onMeshCfginstead of the top-level cfg to match the current API and avoid relying on legacy overrides.
This issue also appears on line 475 of the same file.
RigidObjectCfg(
uid=table_id,
shape=MeshCfg(fpath=str(table_mesh_path)),
init_pos=tuple(table_rigid_layout["pos"]),
init_rot=tuple(
embodichain/gen_sim/scene_engine/cli/preview.py:160
RigidObjectCfg.max_convex_hull_num/acd_methodare deprecated in favor ofMeshCfg.max_convex_hull_num/MeshCfg.acd_method(seeembodichain.lab.sim.cfg.RigidObjectCfg). For new code, attach VHACD settings to theMeshCfgso they stay with the shape config.
RigidObjectCfg(
uid=uid,
shape=MeshCfg(fpath=str(mesh_path)),
# Keep every preview body static: exported poses are already the
# final gravity-settled poses and should not be simulated again.
embodichain/gen_sim/scene_engine/pipeline/gym_export.py:189
- PR description says VHACD is the default collision decomposition method, but the exported gym config does not specify any
acd_method. In EmbodiChain,MeshCfg.acd_methoddefaults to"coacd"(seeembodichain/lab/sim/shapes.py), so consumers loading this gym config viaRigidObjectCfg.from_dict()will still decompose with CoACD unless explicitly overridden.
"shape": {
"shape_type": "Mesh",
"fpath": asset_relative_path,
"compute_uv": False,
},
embodichain/gen_sim/scene_engine/pipeline/utils/scene_segmentation_utils.py:55
- This PR introduces substantial new parsing/validation logic (e.g., RLE decoding, IoU-based mask merging, strict VLM JSON parsing) but does not add unit tests. The repo already has
tests/gen_sim/coverage for similar geometry/pipeline utilities, so adding focused tests would help prevent regressions (e.g., RLE decode/encode round-trip, union_overlapping_mask_candidates grouping, and error handling for malformed payloads).
def decode_rle_mask(mask_rle: dict[str, Any]) -> Image.Image:
"""Decode an uncompressed RLE mask into a binary image."""
# Check the return value's format.
size = mask_rle.get("size")
counts = mask_rle.get("counts")
if (
not isinstance(size, list)
or len(size) != 2
or not all(isinstance(value, int) and value > 0 for value in size)
):
raise ValueError("Image Segmentation Server RLE needs size=[height, width].")
if not isinstance(counts, list):
raise ValueError("Image Segmentation Server RLE counts must be a list.")
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (9)
embodichain/gen_sim/scene_engine/pipeline/utils/scene_segmentation_utils.py:215
- Typo/grammar in the inline comment ("weuse" / "processings"). This comment is user-facing for future maintainers; please correct it for clarity.
rendered_mask = ( # If weuse outline, then need to do some another processings.
embodichain/gen_sim/scene_engine/pipeline/utils/scene_segmentation_utils.py:195
Image.open(...).convert(...)leaves the file handle open until the Image is closed. Wrap the open in a context manager so repeated calls don't leak descriptors.
image = Image.open(image_path).convert("RGBA")
embodichain/gen_sim/scene_engine/pipeline/utils/scene_generation_utils.py:510
- Calling SimulationManager's private
_deferred_destroy()ties this pipeline to an internal cleanup implementation. Prefer using the publicdestroy(exit_process=False)plusSimulationManager.flush_cleanup_queue()to guarantee cleanup without relying on a private method.
finally:
sim._deferred_destroy()
embodichain/gen_sim/scene_engine/pipeline/generate.py:80
image_segmentation_client.close()is not in afinallyblock, so an exception during segmentation/validation will leak the underlyingrequests.Session. Wrap the segmentation call intry/finallyto always close the client.
image_segmentation_client = ImageSegmentationClient.from_config(
image_segmentation_config_path
)
image_segmentation_client.check_health() # Error raising will happen internally.
scene = segment_scene(
embodichain/gen_sim/scene_engine/pipeline/scene_understanding.py:180
- The inline comment says the condition "will always return true", but the code hardcodes
id="table", so the condition should never be true. Updating the comment avoids misleading future debugging.
if (
scene.table.id != "table"
): # Currently it will always return true. For we hardcode the table id to "table".
raise ValueError("Scene table id must be 'table'.")
embodichain/gen_sim/scene_engine/pipeline/generate.py:54
- This PR adds a large new scene-engine pipeline (VLM JSON validation, RLE decoding, layout transforms, export schema) but does not add any tests. The repo already has
tests/gen_sim/simready_pipeline/*, so adding focused unit tests (e.g., RLE decode/encode round-trip, transform_matrix<->layout_object invariants, exported scene_config schema) would prevent regressions.
def generate_scene_from_image(
image_path: str | Path,
output_root: str | Path,
*,
llm_config_path: str | Path | None = None,
image_segmentation_config_path: str | Path | None = None,
geometry_generation_config_path: str | Path | None = None,
) -> Scene:
embodichain/gen_sim/scene_engine/pipeline/utils/scene_segmentation_utils.py:170
- Image files are opened without a context manager here (and in the mask loop). PIL keeps the underlying file handle open until the Image is closed, which can exhaust file descriptors in batch runs. Use
with Image.open(...)and keep only the converted in-memory image.
This issue also appears on line 195 of the same file.
image = Image.open(image_path).convert("RGB")
ignored_mask = Image.new("L", image.size, 0)
for mask_path in mask_paths:
mask = Image.open(mask_path).convert("L")
_require_image_size(mask, image.size)
embodichain/gen_sim/scene_engine/pipeline/generate.py:106
geometry_generation_client.close()is not protected byfinally, so failures during geometry generation/refinement can leak therequests.Session. Usetry/finallyaround the client usage to ensure it is always closed.
geometry_generation_client = GeometryGenerationClient.from_config(
geometry_generation_config_path
)
geometry_generation_client.check_health()
scene = generate_scene_and_refine(
embodichain/gen_sim/scene_engine/cli/preview.py:90
- In interactive mode the preview loop never advances the simulation (
sim.update(...)). If physics is manually stepped (the common case in SimulationManager), the window may never render/refresh. Stepping at a low rate keeps the viewer responsive and consistent with the headless path.
sim.open_window()
while True:
time.sleep(0.1)
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 (4)
embodichain/gen_sim/scene_engine/cli/preview.py:112
- The preview loop only sleeps and never advances the simulation after opening the window / starting Viser.
SimulationManager.open_window()does not run a render/update loop by itself, so the preview will appear frozen. Callsim.update(step=1)inside the loop (optionally with a small sleep to throttle).
print("Close with Ctrl-C.")
while True:
time.sleep(0.1)
docs/source/features/generative_sim/scene_engine.md:70
- The documented
segment_single_object_pathdoes not match the packaged Scene Engine config (embodichain/gen_sim/scene_engine/configs/scene_engine_config.jsonuses/predict). This can cause copy/paste configs from the docs to fail against the default service settings.
"max_attempts": 3,
"health_path": "/health",
"segment_single_object_path": "/segment_single_object"
},
docs/source/features/generative_sim/scene_engine.md:77
- The documented
generate_objects_pathdoes not match the packaged Scene Engine config (embodichain/gen_sim/scene_engine/configs/scene_engine_config.jsonuses/generate_multiple_objects). Aligning the doc example with the packaged default reduces setup friction.
"max_attempts": 3,
"health_path": "/health",
"generate_objects_path": "/generate_objects"
}
embodichain/gen_sim/scene_engine/llms/load_config.py:65
- This line is likely over Black's default line length and will be reformatted (or may fail format checks if CI enforces
black). Split it across lines to keep the file Black-stable.
base_url = os.getenv("OPENAI_BASE_URL") or llm_config.get("base_url", "")
default_query = llm_config.get("default_query", {})
max_attempts = os.getenv("OPENAI_MAX_ATTEMPTS") or llm_config.get("max_attempts", 3)
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 (2)
embodichain/gen_sim/scene_engine/pipeline/scene_understanding.py:180
- The inline comment is incorrect: since the table id is hard-coded to "table" in validate_scene_understanding_json(), this condition should normally be false. The current wording (“always return true”) is misleading for future debugging.
if (
scene.table.id != "table"
): # Currently it will always return true. For we hardcode the table id to "table".
raise ValueError("Scene table id must be 'table'.")
embodichain/gen_sim/scene_engine/cli/preview.py:112
- The preview loop never advances the SimulationManager, so neither the native window nor the Viser backend will receive regular updates. Other preview tooling keeps the visualizer alive by calling sim.update(step=1) in the main loop (e.g., embodichain/lab/scripts/preview_asset.py:255-258).
while True:
time.sleep(0.1)
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 (5)
embodichain/gen_sim/scene_engine/cli/start.py:86
- The CLI flag
--configis described as optional, but the pipeline always loadsImageSegmentationClient.from_config()/GeometryGenerationClient.from_config()which reject emptybase_urland other required keys. With the packaged config containing placeholders, running without--configwill fail immediately, so the argument should be required (or the code should provide an alternative config source).
parser.add_argument(
"--config",
type=Path,
default=None,
help=(
"Optional Scene Engine JSON config containing the llm, "
"image_segmentation, and geometry_generation service settings."
),
)
docs/source/guides/cli.md:102
- This section says the command requires a config, but the argument table lists
--configdefaulting to a “packaged config”. The packaged config file is a template with required fields left blank, so it is not a usable default. Update the table to mark--configas required (or clarify that the packaged config is only a template).
| ``--image`` | *(required)* | Input ``.jpg``, ``.jpeg``, or ``.png`` scene image |
| ``--output_root`` | *(required)* | Directory that receives intermediate artifacts and ``scene_export/`` |
| ``--config`` | packaged config | Scene Engine JSON config containing ``llm``, ``image_segmentation``, and ``geometry_generation`` settings |
docs/source/features/generative_sim/scene_engine.md:70
- The example config uses
segment_single_object_path: "/segment_single_object", but the packaged Scene Engine config shipped with this PR uses"/predict". Either align the example with the packaged default, or explicitly call out that these paths are service-specific placeholders so users don't copy/paste a non-working endpoint.
"image_segmentation": {
"base_url": "http://segmentation-host:port",
"timeout_s": 120,
"max_attempts": 3,
"health_path": "/health",
"segment_single_object_path": "/segment_single_object"
},
docs/source/features/generative_sim/scene_engine.md:77
- Similarly, the example config uses
generate_objects_path: "/generate_objects", but the packaged config in this PR uses"/generate_multiple_objects". Aligning them avoids users copying a config that won't match the default client settings.
"geometry_generation": {
"base_url": "http://geometry-host:port",
"timeout_s": 600,
"max_attempts": 3,
"health_path": "/health",
"generate_objects_path": "/generate_objects"
}
embodichain/gen_sim/scene_engine/pipeline/utils/scene_segmentation_utils.py:217
- Minor grammar in the inline comment (“some another processings”) makes this section harder to read/maintain while debugging mask rendering.
rendered_mask = ( # If we use outline, then need to do some another processings.
mask if mask_style == "fill" else _mask_outer_outline(mask, image.size)
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 (4)
embodichain/gen_sim/scene_engine/pipeline/scene_understanding.py:180
- The inline comment here is inaccurate/unclear (the table id is hard-coded to "table" in validate_scene_understanding_json(), so this condition should normally be false). It’s better to remove the comment to avoid confusion during debugging.
if (
scene.table.id != "table"
): # Currently it will always return false. For we hardcode the table id to "table".
raise ValueError("Scene table id must be 'table'.")
embodichain/gen_sim/scene_engine/cli/preview.py:112
- The preview loop never calls sim.update(), so the simulation window / Viser session may not render or process events after opening. This effectively turns the command into an idle sleep loop.
while True:
time.sleep(0.1)
embodichain/gen_sim/scene_engine/llms/load_config.py:64
- This line exceeds typical Black formatting and is the only long unwrapped assignment in this file. Wrapping it improves consistency and avoids future formatting churn.
max_attempts = os.getenv("OPENAI_MAX_ATTEMPTS") or llm_config.get("max_attempts", 3)
embodichain/gen_sim/scene_engine/pipeline/utils/scene_generation_utils.py:38
- Calling matplotlib.use("Agg") at import time can raise if pyplot has already been imported elsewhere, and it also globally overrides the user's chosen Matplotlib backend (e.g., notebooks). Consider guarding this so it only runs when safe/needed.
matplotlib.use("Agg")
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
embodichain/gen_sim/scene_engine/cli/preview.py:112
- The preview loop never advances the simulation/rendering; it only sleeps. Unlike
preview_asset.py, which continuously callssim.update(step=1), this will typically leave the window/visualization frozen after the first frame (and may not publish updates to Viser). Replace the sleep-only loop with a loop that steps the simulation.
print(f"Previewing: {config_path}")
sim.open_window()
print("Close with Ctrl-C.")
while True:
time.sleep(0.1)
embodichain/gen_sim/scene_engine/clients/image_segmentation.py:125
- The error check
response_data.get("ok") is Falseis too narrow: it will treatok=0(or other non-True values) as success and silently return empty masks. If the server returns anokfield, it should be treated as a strict success indicator and raise for anything other thanTrue.
if not isinstance(response_data, dict):
raise RuntimeError(
"Image Segmentation Server response must be a JSON object."
)
if response_data.get("ok") is False:
raise RuntimeError(
"Image Segmentation Server request failed: "
f"{response_data.get('error', 'unknown error')}"
)
return _extract_rle_masks(response_data)
docs/source/guides/cli.md:102
- The CLI docs claim
--configdefaults to a "packaged config", but the packaged template has empty service URLs and the CLI will fail without a user-provided config. Update this default/description to reflect the actual behavior (e.g., mark required, or clarify that the packaged file is only a template and must be filled in).
| Argument | Default | Description |
|---|---|---|
| ``--image`` | *(required)* | Input ``.jpg``, ``.jpeg``, or ``.png`` scene image |
| ``--output_root`` | *(required)* | Directory that receives intermediate artifacts and ``scene_export/`` |
| ``--config`` | packaged config | Scene Engine JSON config containing ``llm``, ``image_segmentation``, and ``geometry_generation`` settings |
embodichain/gen_sim/scene_engine/pipeline/utils/scene_generation_utils.py:34
matplotlib.use("Agg")is executed at import time, which globally overrides the Matplotlib backend for the entire Python process wheneverscene_generation_utilsis imported (the pipeline imports it unconditionally). This can break interactive usage (e.g., Qt backends) in downstream code. Prefer setting the backend only inside the debug-rendering helper(s), or guarding it so it does not override a user-selected backend.
matplotlib.use("Agg")
| parser.add_argument( | ||
| "--config", | ||
| type=Path, | ||
| default=None, | ||
| help=( | ||
| "Optional Scene Engine JSON config containing the llm, " | ||
| "image_segmentation, and geometry_generation service settings." | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Thank you for your reviewing, I have already fix this problem and updated relevant docs :)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
embodichain/gen_sim/scene_engine/pipeline/utils/scene_segmentation_utils.py:196
render_numbered_mask_candidatesopens the source image without closing it. Using awith Image.open(...)block avoids leaking file descriptors when this helper is called repeatedly.
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_segmentation_utils.py:170
Image.open(...)is used without a context manager inrender_image_without_masks. PIL keeps file handles open until the image object is closed/GC’d, which can leak descriptors when processing many images/masks.
This issue also appears on line 195 of the same file.
image = Image.open(image_path).convert("RGB")
ignored_mask = Image.new("L", image.size, 0)
for mask_path in mask_paths:
mask = Image.open(mask_path).convert("L")
_require_image_size(mask, image.size)
embodichain/gen_sim/scene_engine/pipeline/scene_understanding.py:179
- The inline comment is grammatically incorrect ("For we hardcode...") and makes the intent harder to read.
): # Currently it will always return false. For we hardcode the table id to "table".
| output_path = resolved_output_root / f"{object_id}.glb" | ||
| self._download_glb(response_object["mesh"], output_path) |
There was a problem hiding this comment.
Thank you, dear Copilot~ I have fix this problem and updated the test script.
| mesh_path = (config_dir / shape["fpath"]).resolve() | ||
| if not mesh_path.is_file(): | ||
| raise FileNotFoundError(f"Gym mesh for {uid!r} not found: {mesh_path}") |
There was a problem hiding this comment.
Thank you for your review. I have fix this problem and updated the test script.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated no new comments.
Suppressed comments (3)
embodichain/gen_sim/scene_engine/pipeline/utils/scene_generation_utils.py:484
- RigidObjectCfg.max_convex_hull_num and RigidObjectCfg.acd_method are deprecated (MeshCfg owns these settings). Keeping them on RigidObjectCfg risks the convex-decomposition settings being ignored if/when the deprecated fields are removed.
Move these parameters onto the MeshCfg for each asset body.
max_convex_hull_num=max_convex_hull_num,
acd_method="vhacd", # Use vhacd by default.
embodichain/gen_sim/scene_engine/cli/preview.py:202
- RigidObjectCfg.max_convex_hull_num and RigidObjectCfg.acd_method are marked deprecated in the simulation config (use MeshCfg.max_convex_hull_num / MeshCfg.acd_method instead). Setting the deprecated fields here may be ignored in future refactors and duplicates the same settings across two config objects.
Consider moving these settings onto MeshCfg and removing the deprecated RigidObjectCfg fields.
max_convex_hull_num=max_convex_hull_num,
acd_method="vhacd", # Use vhacd by default.
embodichain/gen_sim/scene_engine/pipeline/utils/scene_generation_utils.py:469
- RigidObjectCfg.max_convex_hull_num and RigidObjectCfg.acd_method are deprecated (the active fields live on MeshCfg). To avoid relying on deprecated configuration that may be removed/ignored, set max_convex_hull_num/acd_method on the MeshCfg instance instead.
This issue also appears on line 483 of the same file.
max_convex_hull_num=max_convex_hull_num,
acd_method="vhacd", # Use vhacd by default.
yuecideng
left a comment
yuecideng
left a comment
There was a problem hiding this comment.
The overall layering is strong: the staged pipeline, strict service-boundary validation, explicit coordinate-frame handling, debug artifacts, and portable scene export form a solid end-to-end MVP. The inline comments below focus on the gaps that should be addressed before treating this as a robust scene reconstruction engine. P0 marks scene-correctness risks; P1 marks reliability and maintainability risks.
| ## Quick Start | ||
|
|
||
| Install EmbodiChain with the generative-simulation dependencies. See | ||
| [Installation (gensim extra)](../../quick_start/install.md#optional-generative-simulation-gensim). |
There was a problem hiding this comment.
[P1] The gensim extra currently declares only bpy and pyrender, while Scene Engine directly imports requests, Pillow, SciPy, trimesh, Open3D, and matplotlib. A clean embodichain[gensim] installation therefore relies on undeclared transitive dependencies. Please declare these dependencies directly, possibly under a dedicated scene-engine extra, and exercise that installation path in CI.
| # Absolute path to the canonicalized GLB used by the final simulation. | ||
| simready_glb_path: str | None = None | ||
| # Final y-up layout after scene refinement and gravity settling. | ||
| rot: list[float] | None = None |
There was a problem hiding this comment.
[P1] This mutable dataclass represents every pipeline stage through nullable fields, while Table duplicates almost the same shape. That makes invalid state combinations representable until a late export-time check. Please consider a shared SceneObject model with typed pose/frame/artifact fields, or stage-specific validated models. The scene IR will also need support relations and confidence/provenance if stacked or contained objects are to be preserved.
| *, | ||
| llm_config_path: str | Path | None = None, | ||
| image_segmentation_config_path: str | Path | None = None, | ||
| geometry_generation_config_path: str | Path | None = None, |
There was a problem hiding this comment.
[P1] One Scene Engine JSON file is exposed here as three independently parsed paths, while important behavior such as IoU thresholds, retry counts, packing clearances, settle steps, physics settings, and hull counts remains hard-coded elsewhere. Please load and validate one project-style @configclass SceneEngineCfg, then inject the relevant sub-config into each stage/client. This would also make runs reproducible through a single config snapshot.
| # Create stage output directory. | ||
| stage_output_root = Path(output_root).expanduser().resolve() / "scene_generation" | ||
| if stage_output_root.exists(): | ||
| shutil.rmtree(stage_output_root) |
There was a problem hiding this comment.
[P1] This removes the previous, potentially expensive generation artifacts before a replacement has completed. Any service or simulation failure leaves a partial stage and prevents resuming. Please build each stage in a temporary directory, validate it, write an input/config/model manifest, and atomically replace the completed stage. A --resume/--from-stage path would be especially valuable for this multi-service pipeline.
| :, :2 | ||
| ] # Ignore z, for we wanna get the x-y plane projection. | ||
| try: | ||
| projected_hull = ConvexHull( |
There was a problem hiding this comment.
[P0] This is the convex hull of the projection of the entire table mesh, not the actual top support surface. It fills concavities and holes and may include footprints from legs or overhanging geometry, so the packer can place assets over unsupported space. Please extract near-top, upward-facing triangles (or a support height field/polygon) and validate candidate footprints with downward raycasts. The existing comment about L-shaped tables indicates this is a known correctness limitation.
|
|
||
| # All assets share this one simulation, so they can collide with the | ||
| # table and with one another while settling. | ||
| sim.update(step=settle_steps) |
There was a problem hiding this comment.
[P0] A fixed number of simulation steps is not a convergence or validity criterion. Before exporting, please require linear/angular velocities to remain below thresholds for several frames and validate finite poses, table support, footprint containment, and unacceptable penetration/fall-off. If those postconditions fail, the pipeline should retry layout/settling or return a clear quality failure rather than exporting the pose.
| "dynamic_friction": 0.9, | ||
| "restitution": 0.01, | ||
| } | ||
| _ASSET_PHYSICS_ATTRS = { |
There was a problem hiding this comment.
[P1] The settling simulation uses RigidObjectCfg defaults and 32 convex hulls, while the exported assets use these different physical attributes and 16 hulls. The final dynamic scene may therefore not reproduce the behavior under which its pose was settled. Please share the same object physics and collision-decomposition configuration between settlement, export, and downstream loading.
|
|
||
| export_root = Path(output_root).expanduser().resolve() / "scene_export" | ||
| mesh_assets_root = export_root / "mesh_assets" | ||
| mesh_assets_root.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
[P1] scene_export is neither refreshed nor written atomically. Re-running into the same output root with fewer objects leaves stale mesh directories, and a copy/write failure can leave a config and asset set from different runs. Please assemble a fresh temporary export, validate all entries, then atomically replace the previous directory.
| object_masks: list[tuple[str, Path]], | ||
| ) -> tuple[dict[str, Any], list[dict[str, Any]]]: | ||
| last_error: Exception | None = None | ||
| for _ in range(self._max_attempts): |
There was a problem hiding this comment.
[P1] Retrying the entire submission immediately can create duplicate expensive geometry jobs when the server accepted the request but the response was lost. Please separate submit/poll/download retry policies, add exponential backoff and jitter, and use an idempotency key or persisted request ID. The polling endpoint, interval, and 600-second deadline should also come from configuration.
| ) | ||
|
|
||
|
|
||
| def test_layout_transform_round_trip_preserves_pose_and_scale() -> None: |
There was a problem hiding this comment.
[P1] The current tests cover a transform round trip and a simple AABB happy path, but not the high-risk behavior introduced here. Please add fixtures for concave/rotated/asymmetric tables, stacked or contained assets, dense/infeasible packing, gravity convergence and fall-off checks, queued geometry jobs, and a mocked end-to-end pipeline. Golden-scene metrics such as support validity and relation retention would make scene quality regressions visible.
Description
This PR adds the image to tabletop scene generation pipeline, including semantic understanding, image segmentation and verification, geometry generation, layout refinement, gravity settling, Gym export, and preview tooling.
It also uses VHACD as the default simulation collision-decomposition method.
Dependencies: Existing scene-engine service endpoints and simulation dependencies.
Type of change
Screenshots
N/A
Checklist
black .command to format the code base. (blackis unavailable in the currentenvironment; the branch includes its existing
RAN blackcommit.)