Refine URDF assembly component prefixes and name casing policy#202
Refine URDF assembly component prefixes and name casing policy#202
Conversation
There was a problem hiding this comment.
Pull request overview
Refines the URDF assembly toolchain’s naming configuration by introducing per-component prefix overrides, a global link/joint casing policy, and signature inputs that invalidate cached assemblies when naming semantics change. Also wires these knobs into URDFCfg and user config merging, and documents the new configuration surface.
Changes:
- Add patch-style
component_prefixand globalname_casehandling in URDF assembly, and include both in the assembly signature. - Update component/connection naming normalization to use a centralized casing policy helper.
- Integrate new config fields through
URDFCfg/ config merging and extend documentation.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/sim/robot/dexforce_w1.py | Formatting-only bracket/comma adjustment. |
| embodichain/toolkits/urdf_assembly/urdf_assembly_manager.py | Adds name_case + component_prefix logic and injects naming metadata into signature; updates manager construction to pass casing policy. |
| embodichain/toolkits/urdf_assembly/signature.py | Stores component_order_and_prefix + name_case metadata in signature JSON. |
| embodichain/toolkits/urdf_assembly/connection.py | Adds name casing policy support for generated connection names and sensor attachment rewriting. |
| embodichain/toolkits/urdf_assembly/component.py | Introduces casing policy and replaces hard-coded .lower()/.upper() with _apply_case. |
| embodichain/lab/sim/utility/cfg_utils.py | Extends robot cfg merge to include sensors and URDF naming configuration fields. |
| embodichain/lab/sim/cfg.py | Adds URDFCfg.component_prefix and forwards prefix/casing configuration into the assembly manager. |
| docs/source/features/toolkits/urdf_assembly.md | Documents component_prefix and name_case, plus URDFCfg integration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ): | ||
| self.logger = setup_urdf_logging() | ||
|
|
||
| # Global name normalization strategy for this assembly. By default, | ||
| # this preserves the legacy behavior: link names are lowercase and |
There was a problem hiding this comment.
The constructor hard-codes the default name casing policy but does not accept a name_case parameter. This conflicts with the documented usage (URDFAssemblyManager(name_case=...)) and will raise TypeError for users following the docs. Consider adding an optional name_case argument to __init__ (and validating/merging it) or updating the docs to reflect the property-based configuration.
| # Add sensor links and joints to the main lists | ||
| for link in sensor_urdf.findall("link"): | ||
| # Ensure sensor link names are lowercase | ||
| link.set("name", link.get("name").lower()) | ||
| link.set("name", self._apply_case("link", link.get("name"))) | ||
| joints.append(link) # This should be added to links list instead |
There was a problem hiding this comment.
add_sensor_attachments appends <link> elements into the joints list (joints.append(link)), which will cause links to be emitted in the joint section if this method is used. Either accept a separate links list here (and append links to it) or rename/adjust the method so it cannot mix element types.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| ##### Name casing policy (`name_case`) | ||
|
|
||
| `URDFAssemblyManager` supports a global name casing policy that controls how | ||
| link and joint names are normalized during assembly. This is configured via | ||
| the optional `name_case` argument in the constructor: | ||
|
|
||
| ```python | ||
| manager = URDFAssemblyManager( | ||
| name_case={ | ||
| "joint": "upper", # or "lower" / "none" | ||
| "link": "lower", # or "upper" / "none" | ||
| } | ||
| ) | ||
| ``` | ||
|
|
||
| Semantics: | ||
|
|
||
| - Valid keys: `"joint"`, `"link"`. | ||
| - Valid values: `"upper"`, `"lower"`, `"none"`. | ||
| - Default behavior matches the legacy implementation: |
There was a problem hiding this comment.
The documentation states that name_case is configured via a URDFAssemblyManager(..., name_case=...) constructor argument, but the current URDFAssemblyManager.__init__ signature does not accept this parameter. Update the docs to match the actual API, or implement the constructor argument as documented.
| ##### Name casing policy (`name_case`) | |
| `URDFAssemblyManager` supports a global name casing policy that controls how | |
| link and joint names are normalized during assembly. This is configured via | |
| the optional `name_case` argument in the constructor: | |
| ```python | |
| manager = URDFAssemblyManager( | |
| name_case={ | |
| "joint": "upper", # or "lower" / "none" | |
| "link": "lower", # or "upper" / "none" | |
| } | |
| ) | |
| ``` | |
| Semantics: | |
| - Valid keys: `"joint"`, `"link"`. | |
| - Valid values: `"upper"`, `"lower"`, `"none"`. | |
| - Default behavior matches the legacy implementation: | |
| ##### Name casing policy | |
| `URDFAssemblyManager` applies a global name casing policy that controls how | |
| link and joint names are normalized during assembly. | |
| Semantics: | |
| - The policy applies separately to joint names and link names. | |
| - Current default behavior (matching the legacy implementation): |
| if use_signature_check: | ||
| # Calculate current assembly signature | ||
| # Calculate current assembly signature. In addition to the component | ||
| # registry contents, include the current component_order_and_prefix | ||
| # so that changes to name prefixes also invalidate the cache. | ||
| component_info = self.component_registry.all().copy() | ||
| component_info["__component_order_and_prefix__"] = list( | ||
| self.component_order_and_prefix | ||
| ) | ||
| # Also include the assembly-wide name_case policy so that | ||
| # renaming rules (e.g. link/joint casing) participate in the | ||
| # signature. This ensures that changing naming strategy forces | ||
| # a rebuild. | ||
| component_info["__name_case__"] = dict(self._name_case) | ||
|
|
||
| assembly_signature = self.signature_manager.calculate_assembly_signature( | ||
| self.component_registry.all(), output_path | ||
| component_info, output_path | ||
| ) |
There was a problem hiding this comment.
New naming semantics (component_prefix patching + name_case) affect URDF outputs and caching behavior, but there are no tests asserting: (1) unknown component keys raise, (2) per-component prefix patches preserve default ordering, (3) name_case changes affect link/joint names, and (4) signature changes when either setting changes. Adding focused unit tests with minimal URDF XML fixtures would prevent regressions.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| if not isinstance(new_prefixes, list) or not all( | ||
| isinstance(item, tuple) and len(item) == 2 for item in new_prefixes | ||
| ): | ||
| raise ValueError( | ||
| "component_order_and_prefix must be a list of (component_name, prefix) tuples." |
There was a problem hiding this comment.
The component_prefix setter error message (and subsequent validation messages) refer to component_order_and_prefix, which is an internal name. Since this is the public configuration surface, the exception text should reference component_prefix (and ideally mention the patch-style semantics / allowed component keys) to avoid confusing users.
| if prefix: | ||
| new_name = self._generate_unique_name( | ||
| orig_name, prefix, global_link_names | ||
| ).lower() | ||
| ) | ||
| else: |
There was a problem hiding this comment.
When prefix is set, link names produced by _generate_unique_name(...) are no longer normalized via _apply_case("link", ...). This makes the emitted link names inconsistent with the configured name_case policy and can also break the collision checks that rely on global_link_names being normalized.
| # For uniqueness checks we always operate on a normalized form that is | ||
| # consistent with the link case policy. This keeps collisions and | ||
| # generated names aligned with how names are written back to the URDF. | ||
| base_name = orig_name | ||
| if prefix and not orig_name.lower().startswith(prefix.lower()): |
There was a problem hiding this comment.
_generate_unique_name claims it operates on a normalized form for uniqueness checks, but new_name is never normalized before comparing against existing_names. Since callers build existing_names using _apply_case(...), this can miss collisions (e.g., Left_Link vs left_link) and generate duplicate names. Normalize base_name/new_name according to the relevant case policy before the uniqueness loop.
| transform=component.get("transform"), | ||
| ) | ||
| for sensor in user_urdf_cfg.get("sensors", []): | ||
| base_cfg.urdf_cfg.add_sensor( |
There was a problem hiding this comment.
URDFCfg.add_sensor(...) requires a sensor_name positional argument, but this merge path calls add_sensor(sensor_type=..., urdf_path=..., ...) without providing it. This will raise a TypeError when override_cfg_dict['urdf_cfg']['sensors'] is present. Pass sensor_name=sensor.get('sensor_name') (or clearly define which field is the name) and forward the rest as kwargs.
| base_cfg.urdf_cfg.add_sensor( | |
| base_cfg.urdf_cfg.add_sensor( | |
| sensor_name=sensor.get("sensor_name"), |
| self._name_case = new_name_case | ||
| self.component_manager.name_case = new_name_case | ||
| self.sensor_manager.name_case = new_name_case | ||
|
|
There was a problem hiding this comment.
name_case setter validates that the input is a dict and contains both keys, but it does not validate the values. If a caller passes an unsupported mode (e.g. "uppercase"), it will be accepted and _apply_case will silently fall back to returning the original name (since the mode won't match "upper"/"lower"). This contradicts the documented valid values; consider validating modes and raising a ValueError (or normalizing/merging with defaults) to fail fast.
| self._name_case = new_name_case | |
| self.component_manager.name_case = new_name_case | |
| self.sensor_manager.name_case = new_name_case | |
| # Validate and normalize the case modes to ensure they match the supported values. | |
| allowed_modes = {"upper", "lower", "none"} | |
| validated_name_case: dict[str, str] = {} | |
| for kind in ("joint", "link"): | |
| mode = new_name_case.get(kind) | |
| if not isinstance(mode, str): | |
| raise ValueError( | |
| f"name_case['{kind}'] must be a string; got {type(mode).__name__}." | |
| ) | |
| normalized_mode = mode.lower() | |
| if normalized_mode not in allowed_modes: | |
| raise ValueError( | |
| f"Unsupported case mode {mode!r} for '{kind}'. " | |
| f"Expected one of: {', '.join(sorted(allowed_modes))}." | |
| ) | |
| validated_name_case[kind] = normalized_mode | |
| self._name_case = validated_name_case | |
| self.component_manager.name_case = validated_name_case | |
| self.sensor_manager.name_case = validated_name_case |
|
@chase6305 I've opened a new pull request, #203, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@chase6305 I've opened a new pull request, #204, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
|
||
| class URDFComponentManager: | ||
| """Responsible for loading, renaming, and processing meshes for a single component.""" | ||
| """Responsible for loading, renaming, and processing meshes for a single component. |
There was a problem hiding this comment.
We should add some tests for URDF Assembly tool.
Description
Summary
This PR refines the URDF assembly pipeline with configurable component name prefixes, a global name casing policy for links and joints, and extended signature semantics so that naming‑related configuration changes correctly invalidate cached assemblies.
Motivation
lower()/upper()behavior via a singlename_casepolicy instead of hard‑coded transformations scattered across managers.URDFCfgand documentation with the new configuration surface.Changes
URDFAssemblyManager
component_prefixproperty:_component_order_and_prefix: list[tuple[str, str | None]].component_prefixacts as a patch‑style interface:(component_name, prefix)tuples.chassis,torso,left_arm, …) may be overridden.ValueError.name_caseargument:URDFAssemblyManager(..., name_case: dict[str, str] | None = None)."joint","link"."upper","lower","none"._apply_case(kind: str, name: str | None) -> str | Noneand remove scattered hard‑coded.lower()/.upper()calls in favor of this policy.name_caseto internal managers:URDFComponentManager(mesh_manager, name_case=self._name_case)URDFConnectionManager(self.base_link_name, name_case=self._name_case)URDFSensorManagercan also honor the same policy.component_infoforURDFAssemblySignatureManager, inject:__component_order_and_prefix__ = list(self.component_order_and_prefix)__name_case__ = dict(self._name_case)URDFAssemblySignatureManager
signature_datato include:component_order_and_prefixname_case__component_order_and_prefix__→ stored insignature_data["component_order_and_prefix"]__name_case__→ stored insignature_data["name_case"]URDFCfg integration
component_prefixsupport toURDFCfg:list[tuple[str, str | None]]with a default matchingURDFAssemblyManager’s internal order.None→ uses the default prefix list.dict[str, str]→ converted tolist(component_prefix.items())for convenience (e.g.{"left_hand": "l_", "right_hand": "r_"}).list[tuple[str, str | None]]→ used as is.assemble_urdf(), passself.component_prefixdirectly toURDFAssemblyManager.component_prefixso thatURDFCfgcan drive per‑component prefixes from robot configs.name_casetoURDFCfgand forward it toURDFAssemblyManagerto allow configuring naming policy from robot configuration.Component and connection managers (if part of this PR)
name_case: dict[str, str] | None = Noneand store internally._apply_case(kind, name)helper mirroring the assembly manager..lower()/.upper()on link and joint names with the configured casing policy.name_case: dict[str, str] | None = None._apply_case("joint", ...)for generated joint names._apply_case("link", ...)for parent/child link names in connection joints.key_casepolicy while keeping logical component names stable in the public API.Documentation
component_prefix) section describing:name_case) section describing:URDFAssemblyManagerconstructor.URDFCfg.component_prefixand its identical semantics toURDFAssemblyManager.component_prefix.Backward compatibility
component_prefixorname_caseshould observe identical URDF outputs.Checklist
black .command to format the code base.