Skip action_manager.process_action() when action is None

[lanafi00]

Description

self.action_manager.process_action(action.to(self.device)) was crashing when action is None, which happens for any step whose StateMachine action sequence has no agent_assets (ie a pure SemanticActionCfg step like TurnOnHeaterCfg). I fixed this by adding a check to only run self.action_manager.process_action(action.to(self.device)) if action is not None.

Type of change

• Bug fix (non-breaking change which fixes an issue)

Checklist

• I have run the pre-commit checks with <FULL_PATH_TO_ISAACLAB>/isaaclab.sh --format
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• I have updated the changelog and the corresponding version in the extension's config/extension.toml file
• I have added my name to the CONTRIBUTORS.md or my name already exists there

[SissiFeng]

Request changes: please add regression coverage for the complete supported workflow path before this is merged.

The underlying None condition is reproducible, but the current patch only guards MatterixBaseEnv.step(). The standard runner still calls action.to(env.device) before env.step(), so a semantic-only workflow fails before reaching this guard.

Please complete the following:

1. Add a StateMachine unit test for a sequence containing only TurnOnHeaterCfg:

   • StateMachine.step() returns None;
   • exactly one IsHeaterOn semantic action is emitted;
   • the sequence is marked successful.

2. Add an end-to-end runner regression test showing that scripts/run_workflow.py can execute the semantic-only sequence without calling .to() on None.

3. Add an Isaac runtime test for env.step(None, semantic_actions=...) verifying that:

   • action_manager.process_action() is not called;
   • the semantic action is applied exactly once;
   • subsequent action_manager.apply_action() calls do not apply zero or stale controller targets.

4. Cover both important transitions:

   • immediately after environment reset → semantic-only action;
   • physical robot action → semantic action, where the robot must hold its previous target.

5. Keep a mixed workflow regression test confirming that a sequence containing later robot actions still returns a valid hold tensor during its initial semantic step.

6. Update the MatterixBaseEnv.step() and StateMachine.step() type annotations/docstrings to include None, and document what None means for physical control.

Please run these tests after rebasing onto current main and include the exact Isaac Lab/Isaac Sim versions in the test evidence.

[SissiFeng]

@lanafi00

Thank you for the update. The core fix now covers both MatterixBaseEnv.step() and scripts/run_workflow.py, and the new StateMachine CPU tests pass locally. However, a few test gaps still need to be addressed before approval:

1. Use a real robot action in the mixed-workflow regression test.
   The current test uses WaitCfg(agent_assets="robot") without an ActionSpaceInfo, producing a (2, 1) tensor, while the Franka environment expects an 8-dimensional action. Please use a genuine later robot action with FRANKA_IK_ACTION_SPACE, assert the complete action shape and values, and ideally pass the returned tensor through the environment’s action manager.

2. Strengthen the Isaac runtime assertions.
   Comparing action_manager.action only checks the raw input buffer; it does not prove that the processed controller or actuator target applied during each decimation step is safe. Please verify the actual processed/controller target or the robot joint/EE target for both:

   • reset → semantic-only action;
   • physical robot action → semantic-only action, holding the previous target.

3. Verify that the semantic action is applied exactly once.
   Checking that the heater is on only proves that the action was applied at least once. Please add a call-count assertion around the relevant semantic set_value() or equivalent application point.

4. Make the end-to-end runner test bounded and automatable.
   proc.stdout.readline() can block beyond the stated timeout, and reaching EPISODE 2 only proves that the first episode completed. Please add a bounded runner option such as --max-episodes, require a normal process exit, and assert successful completion without relying on forced termination.

5. Separate CPU and Isaac/GPU test execution.
   When this PR is combined with PR ci: run CPU validation on hosted runners #13, the CPU job fails during collection because test_env_step_none_action.py imports isaaclab. Please keep Isaac-only tests out of the hosted CPU collection and run them in a dedicated trusted Isaac/GPU job.

6. Complete the validation evidence.
   Please run the repository’s pre-commit hooks—Black and isort currently report changes—and provide the successful test logs together with the exact Isaac Lab and Isaac Sim versions used.

Once these items are addressed and both CPU and Isaac runtime checks pass, this should be ready for merge.

[SissiFeng]

@lanafi00 Thank you for the update. I tested the submitted PR and confirmed that the None guards resolve the original crash. However, additional runtime checks found incorrect controller targets after reset, so I am requesting changes before merge.

Test configuration

• PR commit: b83659b67e22c4184c6256a977f53990bf7de815
• Assets commit: 0d856a0572d3e0823204264fd3d2700e15a43f4b
• Python: 3.12.14
• Isaac Lab: 3.0.0b2.post1
• Isaac Sim: 6.0.1.0
• PyTorch: 2.11.0
• Device: cuda:0
• Runtime task: Matterix-Test-Semantics-Heat-Transfer-Franka-v1
• Parallel environments: 2

These results were obtained from the submitted PR code. Additional diagnostic scripts were used without modifying the PR’s production code.

Tests performed

Test	Result
CPU suite: source/matterix_sm/test and source/matterix/test	11 passed
Submitted runtime test: test_env_step_none_action.py	All checks passed
Actual scripts/run_workflow.py, using heater_only, two environments and two episodes	Both environments completed successfully in both episodes
Additional controller-target checks across every physics substep during five consecutive None steps	Failed in the fresh-reset and reset-after-motion scenarios

The additional checks inspected the IK controller’s desired end-effector position and orientation, the processed gripper targets, and whether actuator targets remained finite. They also verified that process_action() was skipped and that every decimation substep was observed.

These checks did not require IK-generated joint targets to remain identical between substeps.

Findings

1. Fresh reset → None: controller targets are not initialized to the reset pose.

In both environments, the observed end-effector position after reset was approximately:

[0.455927, -0.027194, 0.246329]

However, before applying the action terms, the controller contained:

Desired EE position:    [0, 0, 0]
Desired EE quaternion:  [0, 0, 0, 0]
Gripper targets:        [0, 0]
Current finger positions: [0.04, 0.04]

The all-zero quaternion is not a valid unit quaternion. Skipping process_action() therefore does not establish a valid hold command after a fresh reset.

2. Valid motion command → None: passed.

The initialized desired end-effector pose and gripper targets were preserved throughout the checked physics substeps. This behavior should be retained.

3. Motion → reset → None: the controller retains the pre-reset target.

After reset, the observed end-effector position returned to approximately:

[0.455927, -0.027194, 0.246329]

The desired position remained the previous motion command:

[0.465739, -0.027267, 0.249923]

This demonstrates that a stale pre-reset command remains active in the controller state.

These findings concern simulated controller targets. The diagnostic test did not quantify the resulting robot displacement.

Why the submitted tests did not catch this

The submitted runtime test checks that the raw action buffer remains unchanged and that joint targets are finite and not entirely zero. Those checks can pass even when the underlying desired EE pose is uninitialized or stale.

Similarly, successful completion of a semantic-only workflow does not establish that the robot’s physical-control targets are correct.

Required before merge

1. Define and implement the post-reset None contract.
   For the intended hold behavior, initialize valid controller and gripper targets from the post-reset state before they are applied. Cover both fresh initialization and reset after a previous command. Do not assume that a single generic zero tensor is a valid hold command for all action types.

2. Preserve the passing motion → None behavior.
   When a valid command has already been processed and no reset occurs, None should preserve that intended command.

3. Add the three scenarios to the committed runtime regression tests.
   Verify controller-level desired poses and gripper targets through the decimation loop. Include reset-after-motion coverage so stale commands cannot pass unnoticed. Cover partial environment resets as well if they are supported by this implementation.

4. Strengthen the committed runner regression test.
   Assert successful completion in every environment for both episodes. Episode banners and exit code 0 alone are insufficient because the runner can finish an episode on either success or failure.

5. Correct the documentation.
   None skips processing a new action, but apply_action() still executes on every physics substep. The guard alone does not guarantee a correctly initialized hold after reset.

6. Rerun validation on the revised PR and attach evidence.
   Include the tested commit and asset revision, dependency versions, commands, CPU/pre-commit results, and complete runtime logs showing all three controller scenarios and the two-episode runner test passing.

The original crash is resolved, but the reset-time control behavior needs correction before this PR is ready to merge.