Skip to content

Conversation

@yami007007
Copy link
Collaborator

@yami007007 yami007007 commented Dec 12, 2025

Summary

Add a generic assembly_task (bring two objects into proximity) and corresponding task examples: peg insertion and gear meshing.

Detailed description

  • Why this change?
    • The assembly task is a common pattern for bringing two generic objects into proximity.
    • peg_insert and gear_mesh examples are added to demonstrate how to use this atomic task in practice.
  • What was changed?
    • Added a new atomic task: assembly_task, plus a test script for this task.
    • Added two new examples: peg_insert and gear_mesh.

Open issues

For the peg_insert task, the current peg and hole assets are not compatible with Pinocchio. Ideally, this configuration should be defined at the task environment level so that everyone uses the correct setup by default. However, an effective solution has not been found yet, so this is left to Vikram Ramasamy for follow-up (see the discussion on slack ).

Verification

Dataset Preparation

Please dowload the pre-recorded hdf5 files from link and place them in the datasets folder.

Test Commands

Peg insertion task example

python isaaclab_arena/examples/policy_runner.py \
        --policy_type zero_action \
        --disable_pinocchio \
        --environment isaaclab_arena_environments.tabletop_peginsert_environment:PegInsertEnvironment \
         peg_insert
python isaaclab_arena/scripts/imitation_learning/record_demos.py \
        --dataset_file datasets/franka_peginsert.hdf5 \
        --disable_pinocchio \
        --num_demos 1 \
        --environment isaaclab_arena_environments.tabletop_peginsert_environment:PegInsertEnvironment \
         peg_insert \
         --teleop_device keyboard

Since AVP teleop support is not available for Franka, I used an old HDF5 dataset with AbsEEF + binary gripper. Before running replay_demos.py, please update franka.py as follows, then you can use the original HDF5 dataset for demonstration replay.

  1. Set use_relative_mode from True to False.
  2. Set scale from 0.5 to 1.

Original implementation of arm_action in franka.py.

arm_action: ActionTermCfg = DifferentialInverseKinematicsActionCfg(
    asset_name="robot",
    joint_names=["panda_joint.*"],
    body_name="panda_hand",
    controller=DifferentialIKControllerCfg(command_type="pose", use_relative_mode=True, ik_method="dls"),
    scale=0.5,
    body_offset=DifferentialInverseKinematicsActionCfg.OffsetCfg(pos=[0.0, 0.0, 0.107]),
)
python isaaclab_arena/scripts/imitation_learning/replay_demos.py \
        --dataset_file datasets/franka_peginsert_10demos.hdf5 \
        --disable_pinocchio \
        --environment isaaclab_arena_environments.tabletop_peginsert_environment:PegInsertEnvironment \
        peg_insert

Gear meshing task example

python isaaclab_arena/examples/policy_runner.py \
        --policy_type zero_action \
        --environment isaaclab_arena_environments.tabletop_gearmesh_environment:GearMeshEnvironment \
         gear_mesh
python isaaclab_arena/scripts/imitation_learning/record_demos.py \
        --dataset_file datasets/franka_gearmesh.hdf5 \
        --num_demos 1 \
        --environment isaaclab_arena_environments.tabletop_gearmesh_environment:GearMeshEnvironment \
         gear_mesh \
         --teleop_device keyboard

For the same reason as above, please update the arm_action in franka.py accordingly.

python isaaclab_arena/scripts/imitation_learning/replay_demos.py \
        --dataset_file datasets/franka_gearmesh_6demos.hdf5 \
        --environment isaaclab_arena_environments.tabletop_gearmesh_environment:GearMeshEnvironment \
        gear_mesh

@register_asset
class SmallGear(FactoryObject):
"""
A small reference gear for gear mesh task.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, this could be used by other tasks as well not just for "gear mesh task"? Suggest to update the comment



@register_asset
class FactoryTableBackground(LibraryBackground):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets maybe call this the SeattleLabTable :)

def __init__(self, prim_path: str | None = None, initial_pose: Pose | None = None):
super().__init__(prim_path=prim_path, initial_pose=initial_pose)

# Create factory-specific articulation configuration
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that this is being done so one can set other properties to the articulation. I think we should fix this directly where we generally create the articulations so we dont have extra functions.

from isaaclab_arena.terms.events import set_object_pose
from isaaclab_arena.utils.cameras import get_viewer_cfg_look_at_object

FRANKA_PANDA_FACTORY_HIGH_PD_CFG = FRANKA_PANDA_HIGH_PD_CFG.copy()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it would be nice to have this as a function ? Maybe we will also use this for other tasks?

Copy link
Collaborator Author

@yami007007 yami007007 Dec 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @viiik-inside, what do you mean by “having this as a function”? Do you mean wrapping all of this into a standalone function?
This is a reference implementation of franka.py in Isaac Lab. I've put these configs in franka.py of Arena embodiments. I'd like to know your suggestions on this.

from isaaclab_assets.robots.franka import FRANKA_PANDA_HIGH_PD_CFG
FRANKA_PANDA_FACTORY_HIGH_PD_CFG = FRANKA_PANDA_HIGH_PD_CFG.copy()
FRANKA_PANDA_FACTORY_HIGH_PD_CFG.spawn.activate_contact_sensors = True
FRANKA_PANDA_FACTORY_HIGH_PD_CFG.spawn.rigid_props.disable_gravity = True
FRANKA_PANDA_FACTORY_HIGH_PD_CFG.actuators["panda_shoulder"].stiffness = 150.0
FRANKA_PANDA_FACTORY_HIGH_PD_CFG.actuators["panda_shoulder"].damping = 30.0
FRANKA_PANDA_FACTORY_HIGH_PD_CFG.actuators["panda_forearm"].stiffness = 150.0
FRANKA_PANDA_FACTORY_HIGH_PD_CFG.actuators["panda_forearm"].damping = 30.0
FRANKA_PANDA_FACTORY_HIGH_PD_CFG.actuators["panda_hand"].stiffness = 150.0
FRANKA_PANDA_FACTORY_HIGH_PD_CFG.actuators["panda_hand"].damping = 30.0
FRANKA_PANDA_FACTORY_HIGH_PD_CFG.init_state.pos = (0.0, 0.0, 0.0)  # for factory assembly task``` 

self.held_asset = held_asset
self.assist_asset_list = assist_asset_list
self.background_scene = background_scene
self.scene_config = InteractiveSceneCfg(num_envs=1, env_spacing=3.0, replicate_physics=False)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this here? If yes, maybe lets make them parameters

params={
"object_cfg": SceneEntityCfg(self.held_asset.name),
"target_object_cfg": SceneEntityCfg(self.fixed_asset.name),
"max_x_separation": 0.020, # Tolerance for assembly alignment
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to have these as parameters. Maybe a dataclass ?


"""This sub-module contains the functions that are specific to the environment."""

from isaaclab.envs.mdp import * # noqa: F401, F403
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why this folder is called mdp? Just curious

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is following the convention used in IsaacLab mdp. The events, observations, and terminations for the task example are placed in the mdp folder.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. We're trying not to use mdp in arena, because I find the naming non-obvious for users who haven't used Isaac Lab before. What do you think about changing the folder name to helpers (or something similar)?

Copy link
Collaborator Author

@yami007007 yami007007 Jan 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For mdp, I’d suggest keeping the same folder structure as in IsaacLab.
Since most Arena users are likely already using IsaacLab, using mdp to organize observations, actions, rewards, terminations, and resets is a good way to stay consistent.
Just a suggestion, and I’m happy to update it once we reach a conclusion.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK let's merge this as is. Let's see what we think about the naming as we progress. Maybe we change it. I understand the motivation for remaining consistent with Isaac Lab. I just also remember being confused when I started using Isaac Lab by this naming.

func=randomize_object_serials_pose,
mode="reset",
params={
"pose_range": {"x": (0.25, 0.6), "y": (-0.20, 0.20), "z": (0.0, 0.0), "yaw": (-1.0, 1.0)},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use our pose dataclass here

from isaaclab_arena_environments.mdp.events import randomize_object_serials_pose

@configclass
class EventCfgGearMesh:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be in the task class?

from isaaclab_arena_environments import mdp

@configclass
class EventCfgPegInsert:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it has to be here, then we can add it outside this function, on top of the file maybe?

Copy link
Collaborator

@alexmillane alexmillane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you for doing this!

We have requested a few changes.

One additional request is that we add a test for the FactoryAssemblyTask. Such a such could just start the task, then teleport objects into a success configuration and then check that things succeed.

object_min_z = -0.1

def __init__(self):
super().__init__(scale=(1.0, 1.0, 1.0))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we passing the scale of 1.0 here? I would have thought that the default scale is 1.0? Can we remove this?

Comment on lines 97 to 105
def create_factory_articulation_cfg(
prim_path: str,
usd_path: str,
scale: tuple[float, float, float],
mass: float,
rigid_props: sim_utils.RigidBodyPropertiesCfg = RIGID_BODY_PROPS_HIGH_PRECISION,
contact_offset: float = 0.005,
rest_offset: float = 0.0,
) -> ArticulationCfg:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To echo what @viiik-inside said above, we need to expose this functionality through our Object class. I would suggest that we make a proposal next week, formulated as an MR against this one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modify the code based on the new MR.

Comment on lines 58 to 60
if (
i == 0
): # set the "small_gear" and "large_gear" positions according the position of fixed_asset('gear_base')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to make some assumptions about the order of the assets in the list. Is that correct? I would guess that it makes the assumption that the "fixed_asset" appears first in the list. Could we enforce this assumption using the function parameters? For example we could do something like:

def randomize_object_serials_pose(
    env: ManagerBasedEnv,
    env_ids: torch.Tensor,
    fixed_asset_cfg: SceneEntityCfg,
    asset_cfgs: list[SceneEntityCfg],
    min_separation: float = 0.0,
    pose_range: dict[str, tuple[float, float]] = {},
    max_sample_tries: int = 5000,
    relative_asset_cfgs: SceneEntityCfg | None = None,
):

Such that the fixed_asset is passed separately. That way we remove assumptions about the order of assets in the list.

Comment on lines +43 to +67
asset_cfg = asset_cfgs[i]
asset = env.scene[asset_cfg.name]

# Write pose to simulation
pose_tensor = torch.tensor([pose_list[i]], device=env.device)
positions = pose_tensor[:, 0:3] + env.scene.env_origins[cur_env, 0:3]
orientations = math_utils.quat_from_euler_xyz(pose_tensor[:, 3], pose_tensor[:, 4], pose_tensor[:, 5])

asset.write_root_pose_to_sim(
torch.cat([positions, orientations], dim=-1), env_ids=torch.tensor([cur_env], device=env.device)
)
asset.write_root_velocity_to_sim(
torch.zeros(1, 6, device=env.device), env_ids=torch.tensor([cur_env], device=env.device)
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code to get an asset and set it's pose and velocity as zero is repeated in this file. Here and below.

Suggestion to move it out to another function in this same file. Something like:

def set_pose_and_zero_velocity(env, asset_cfg, positions, orientations):

This will make this function more easily understood.

Comment on lines 20 to 28
def randomize_object_serials_pose(
env: ManagerBasedEnv,
env_ids: torch.Tensor,
asset_cfgs: list[SceneEntityCfg],
min_separation: float = 0.0,
pose_range: dict[str, tuple[float, float]] = {},
max_sample_tries: int = 5000,
relative_asset_cfgs: SceneEntityCfg | None = None,
):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion to rename this function to something that indicates what it does. It's more specific than just randomizing poses. It randomizes poses, and then does something special with the relative assets.

Perhaps a docstring would help?

Comment on lines 118 to 124
task = FactoryAssemblyTask(
fixed_asset=gear_base,
held_asset=medium_gear,
assist_asset_list=[small_gear, large_gear],
background_scene=background,
)
task.events_cfg = EventCfgGearMesh()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We replace the event_cfg in the underlying task. Do we ever use the original event_cfg defined in FactoryAssemblyTask?

FRANKA_PANDA_FACTORY_HIGH_PD_CFG.init_state.pos = (0.0, 0.0, 0.0) # for factory assembly task


class FactoryAssemblyTask(TaskBase):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FactoryAssemblyTask -> AssemblyTask

There's nothing specific to factories here.

Comment on lines 42 to 44
"""
Factory assembly task where an object needs to be assembled with a base object, like peg insert, gear mesh, etc.
"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion to expand this docstring. From what I can tell, the goal in this task is to bring two objects into proximity. There are some additional (distractor?) objects in the scene (which are not involved in the task but are there to make things harder?).

If possible, could you expand the docstring to make that clear?

Comment on lines 57 to 66
randomize_gear_positions = EventTerm(
func=randomize_object_serials_pose,
mode="reset",
params={
"pose_range": {"x": (0.25, 0.6), "y": (-0.20, 0.20), "z": (0.0, 0.0), "yaw": (-1.0, 1.0)},
"min_separation": 0.18,
"asset_cfgs": [SceneEntityCfg("gear_base"), SceneEntityCfg("medium_gear")],
"relative_asset_cfgs": [SceneEntityCfg("small_gear"), SceneEntityCfg("large_gear")],
},
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we think of a way of incorperating this EventCfg in the FactoryAssemblyTask.

For example, the FactoryAssemblyTask could take an additional parameter randomization_mode where we select among a few different options for randomizing the assets.

What do you think? Is that possible?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion, thanks. I’ve updated the code accordingly.

Comment on lines 52 to 65
randomize_peg_hole_positions = EventTerm(
func=randomize_object_pose,
mode="reset",
params={
"pose_range": {
"x": (0.2, 0.6),
"y": (-0.20, 0.20),
"z": (0.0, 0.0),
"yaw": (-1.0, 1.0),
},
"min_separation": 0.1,
"asset_cfgs": [SceneEntityCfg("peg"), SceneEntityCfg("hole")],
},
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as my comment above: what do you think about incorperating this into FactoryAssemblyTask with an option for reset mode.

@yami007007 yami007007 force-pushed the feature/factory_assembly branch from 82ae523 to 87a2ac6 Compare December 24, 2025 16:09
@yami007007
Copy link
Collaborator Author

Hi @alexmillane @viiik-inside @cvolkcvolk, thanks for your review and suggestions.
I’ve refactored the assembly task based on your feedback:

  • Generalize FactoryAssemblyTask to AssemblyTask for generic base-object assembly
  • Centralize EventsCfg in the atomic task with randomization_mode for asset randomization variants
  • Simplify assembly asset initialization via spawn_cfg_addon and asset_cfg_addon
  • Add test script for the assembly task
  • Add docstrings for relevant functions
  • Use parameters instead of hard-coded values for flexibility

I won’t reply to every comment individually to keep the thread concise, but I’ve gone through all of your suggestions and applied the corresponding changes. Could you please take another look?

Copy link
Collaborator

@alexmillane alexmillane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thank you for updating this. It's looking good now! I just have a few more small requests.

Comment on lines 95 to 98
ASSEMBLY_ARTICULATION_INIT_STATE = ArticulationCfg.InitialStateCfg(
joint_pos={},
joint_vel={},
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the idea here? I guess these empty dicts differ from what is there by default?

A couple of suggestions:

  • Add a comment to clarify what the purpose of this initial state cfg is,
  • Generalize the name. I presume that this could be used outside of an assembly setting? Maybe something like EMPTY_ARTICULATION_INIT_STATE_CFG? Would that make sense?

Copy link
Collaborator Author

@yami007007 yami007007 Jan 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, great point — I’ve updated it with this more general name and a comment.
As you mentioned, yes, this is actually a patch for the IsaacLab factory assets (peg, hole, gear mesh, etc.). These assets are configured as articulations without any joints.
To work around this, I set the corresponding values to {} in the upper-layer code, since only a few assembly assets are affected. Otherwise, asset initialization fails with an error. The error looks like:
For more details, please see the internal Slack discussion.

self.cfg.init_state.joint_pos: {'.': 0.0}
self.joint_names: []
Exception caught in SimulationAppContext: ValueError: Not all regular expressions are matched! Please check that the regular expressions are correct:
.
: []
Available strings: []

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for updating this!

Comment on lines 40 to 49
FRANKA_PANDA_ASSEMBLY_HIGH_PD_CFG = FRANKA_PANDA_HIGH_PD_CFG.copy()
FRANKA_PANDA_ASSEMBLY_HIGH_PD_CFG.spawn.activate_contact_sensors = True
FRANKA_PANDA_ASSEMBLY_HIGH_PD_CFG.spawn.rigid_props.disable_gravity = True
FRANKA_PANDA_ASSEMBLY_HIGH_PD_CFG.actuators["panda_shoulder"].stiffness = 150.0
FRANKA_PANDA_ASSEMBLY_HIGH_PD_CFG.actuators["panda_shoulder"].damping = 30.0
FRANKA_PANDA_ASSEMBLY_HIGH_PD_CFG.actuators["panda_forearm"].stiffness = 150.0
FRANKA_PANDA_ASSEMBLY_HIGH_PD_CFG.actuators["panda_forearm"].damping = 30.0
FRANKA_PANDA_ASSEMBLY_HIGH_PD_CFG.actuators["panda_hand"].stiffness = 150.0
FRANKA_PANDA_ASSEMBLY_HIGH_PD_CFG.actuators["panda_hand"].damping = 30.0
FRANKA_PANDA_ASSEMBLY_HIGH_PD_CFG.init_state.pos = (0.0, 0.0, 0.0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it, this code is not used in this file. It is only used in the isaaclab_arena_environments package. Could we move it into a (subfolder) in the isaaclab_arena_environments package?

Comment on lines 34 to 37
randomization_mode (int):
0: Randomize the poses of the fixed and held assets.
1: Randomize the poses of the fixed, held, and auxiliary assets. The poses
of the auxiliary assets are set relative to the pose of the fixed asset.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

randomization_mode: Literal["held_and_fixed_only", "held_fixed_and_auxiliary"]

Comment on lines +73 to +87
rel_asset.write_root_pose_to_sim(
torch.cat([positions, orientations], dim=-1), env_ids=torch.tensor([cur_env], device=env.device)
)
rel_asset.write_root_velocity_to_sim(
torch.zeros(1, 6, device=env.device), env_ids=torch.tensor([cur_env], device=env.device)
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems strange to me. It would appear that set the pose of the fixed asset and the auxilliary assets to the same value. Is that intended?

If it is intended, could we add a comment describing why we do this? It's a bit confusing for the reader

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is intended to set the poses of the auxiliary assets based on the fixed asset (for example, the small and large gears are placed on the gear base so the operator only needs to place the medium gear).
There is already a comment above, and I can extend it a bit to make this clearer.
# set the poses of the auxiliary assets according to the pose of the fixed asset

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is confusing is that this is that the auxillary assets are not set to some (constant) offset from the fixed asset. They're set to exactly the same pose. For most assets that will result in an inconsistent scene, because it will generate a scene where objects have intersecting geometry, which will explode when the simulation starts. I would guess what's going on (it's not clear from the comment) is that the particular assets being used here have base frames defined such that an offset of 0 from the base asset generates some valid arrangement of the assets. This makes this function really only valid for a particular set of assets. Probably the assembly gears we're using. In general, assets passed to this function will generate an invalid scene that explodes at start up.

Could you expand the docstring to indicate that we only really expect this function to work with the assembly gears where 0 offset from the assets generates a valid scene.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point.
Yes, 0 works for the factory assembly task, but for other assets customers will need to adjust it themselves.
Let me expand the docstring a bit so that customers know how to adjust or extend it for their own assets.


"""This sub-module contains the functions that are specific to the environment."""

from isaaclab.envs.mdp import * # noqa: F401, F403
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. We're trying not to use mdp in arena, because I find the naming non-obvious for users who haven't used Isaac Lab before. What do you think about changing the folder name to helpers (or something similar)?

@yami007007 yami007007 force-pushed the feature/factory_assembly branch from 78c0948 to d5ae65d Compare January 3, 2026 12:03
@yami007007
Copy link
Collaborator Author

Hi @alexmillane, thanks for all these great suggestions.
I've updated the PR again, could you take another look?

@yami007007 yami007007 requested a review from alexmillane January 4, 2026 01:01
Copy link
Collaborator

@alexmillane alexmillane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding this, and for going back and forth on this with me.

I have one last request for expanding one docstring. However, I've approved it. So merge at will.


"""This sub-module contains the functions that are specific to the environment."""

from isaaclab.envs.mdp import * # noqa: F401, F403
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK let's merge this as is. Let's see what we think about the naming as we progress. Maybe we change it. I understand the motivation for remaining consistent with Isaac Lab. I just also remember being confused when I started using Isaac Lab by this naming.

Comment on lines 95 to 98
ASSEMBLY_ARTICULATION_INIT_STATE = ArticulationCfg.InitialStateCfg(
joint_pos={},
joint_vel={},
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for updating this!

Comment on lines +73 to +87
rel_asset.write_root_pose_to_sim(
torch.cat([positions, orientations], dim=-1), env_ids=torch.tensor([cur_env], device=env.device)
)
rel_asset.write_root_velocity_to_sim(
torch.zeros(1, 6, device=env.device), env_ids=torch.tensor([cur_env], device=env.device)
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is confusing is that this is that the auxillary assets are not set to some (constant) offset from the fixed asset. They're set to exactly the same pose. For most assets that will result in an inconsistent scene, because it will generate a scene where objects have intersecting geometry, which will explode when the simulation starts. I would guess what's going on (it's not clear from the comment) is that the particular assets being used here have base frames defined such that an offset of 0 from the base asset generates some valid arrangement of the assets. This makes this function really only valid for a particular set of assets. Probably the assembly gears we're using. In general, assets passed to this function will generate an invalid scene that explodes at start up.

Could you expand the docstring to indicate that we only really expect this function to work with the assembly gears where 0 offset from the assets generates a valid scene.

@yami007007 yami007007 enabled auto-merge (squash) January 5, 2026 03:16
@yami007007
Copy link
Collaborator Author

I’ve added the docstring for asset randomization.
For the mdp folder, let’s see how things progress.
Merging the PR. Thanks Alex.

@yami007007 yami007007 force-pushed the feature/factory_assembly branch from 4ee5de3 to 1663402 Compare January 5, 2026 13:45
- Generalize FactoryAssemblyTask to AssemblyTask for generic base-object assembly
- Centralize EventsCfg in the atomic task with randomization_mode for asset randomization variants
- Simplify assembly asset initialization via spawn_cfg_addon and asset_cfg_addon
- Add test script for the assembly task
- Add docstrings for relevant functions
- Use parameters instead of hard-coded values for flexibility
- Generalize FactoryAssemblyTask to AssemblyTask for generic base-object assembly
- Centralize EventsCfg in the atomic task with randomization_mode for asset randomization variants
- Simplify assembly asset initialization via spawn_cfg_addon and asset_cfg_addon
- Add test script for the assembly task
- Add docstrings for relevant functions
- Use parameters instead of hard-coded values for flexibility
Comment --use-current-year flag to prevent automatic copyright updates
from 2025 to 2025-2026 on unchanged files, which would add noise to this PR.
@yami007007 yami007007 force-pushed the feature/factory_assembly branch from bb84abd to 2b460f7 Compare January 6, 2026 13:06
@yami007007 yami007007 merged commit 73d1f64 into main Jan 6, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants