Fixes navigation env config for PresetCfg compatibility

[kellyguo11]

Description

The navigation environment configuration was broken after the Newton preset system was introduced (#4794, #5014). The LOW_LEVEL_ENV_CFG contained unresolved PresetCfg wrappers (e.g. VelocityEnvContactSensorCfg for contact forces, preset() for actuator armature) which caused two issues:

1. update_period silently lost: Setting update_period on a PresetCfg wrapper in __post_init__ worked (as a dynamic attribute), but was discarded when resolve_presets() later replaced the wrapper with the actual ContactSensorCfg. The resolved config retained the default update_period=0.0 instead of the intended 0.005.

2. sim.physics_material not propagated: The navigation env's __post_init__ did not copy the terrain's physics material to sim.physics_material, resulting in default friction values (0.5/0.5/average) instead of the terrain's configured values (1.0/1.0/multiply).

Fix

• Call resolve_presets() on LOW_LEVEL_ENV_CFG at module import time so all PresetCfg wrappers (contact sensor, actuator armature, etc.) are resolved before they are referenced by NavigationEnvCfg defaults.
• Call resolve_presets(self.scene) in __post_init__ so that any remaining PresetCfg nodes in the deep-copied scene are resolved before update_period is assigned.
• Propagate sim.physics_material from the terrain, matching the pattern used by LocomotionVelocityRoughEnvCfg.

Type of change

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

Checklist

• I have run the pre-commit checks with ./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

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR fixes a configuration bug in the navigation environment where PresetCfg wrappers were not being resolved before their attributes were modified, causing update_period and physics_material settings to be silently lost. The fix correctly calls resolve_presets() at module import time for the shared LOW_LEVEL_ENV_CFG and again in __post_init__ for the instance's deep-copied scene.

Architecture Impact

The change is well-scoped to the navigation environment configuration. The pattern of calling resolve_presets() at module level for shared config objects and again in __post_init__ for instance-specific copies aligns with how the preset system is designed to work. The LOW_LEVEL_ENV_CFG is referenced by ActionsCfg at class definition time (lines 51-56), so resolving it at import time is correct. Other environments using similar patterns may need analogous fixes.

Implementation Verdict

Minor fixes needed — The core fix is correct, but there's a potential issue with the physics material assignment order.

Test Coverage

The PR description claims tests were added ("I have added tests that prove my fix is effective"), but no test files are included in this PR. For a bug fix of this nature, a regression test verifying that:

1. contact_forces.update_period equals sim.dt (0.005, not 0.0)
2. sim.physics_material matches the terrain's physics material

would be appropriate. Without tests, this bug could regress silently.

CI Status

No CI checks available yet — cannot assess whether the fix passes existing tests.

Findings

🟡 Warning: navigation_env_cfg.py:141 — Physics material assignment occurs before resolve_presets(self.scene)

self.sim.physics_material = self.scene.terrain.physics_material  # line 141
...
resolve_presets(self.scene)  # line 146

If self.scene.terrain itself contains a PresetCfg wrapper, physics_material might be read from an unresolved wrapper. The safer order would be to call resolve_presets(self.scene) first, then assign self.sim.physics_material. Looking at the code flow: self.scene is assigned from LOW_LEVEL_ENV_CFG.scene (line 123), and LOW_LEVEL_ENV_CFG is already resolved at import time (line 21). However, when NavigationEnvCfg is instantiated, self.scene may be a deep copy with fresh PresetCfg wrappers depending on how @configclass handles copying. Recommend moving resolve_presets(self.scene) before the physics_material assignment for safety.

🔵 Improvement: navigation_env_cfg.py:143-146 — Comment placement could be clearer
The comment block (lines 143-146) explains why resolve_presets is called, but it's placed between unrelated assignments (decimation, episode_length_s) and the actual resolve_presets call. Consider moving the comment directly above the call, or restructuring so resolve_presets(self.scene) comes immediately after scene-related setup.

🔴 Critical: Missing regression tests
The PR claims tests were added but none are present in the changeset. Given the subtle nature of this bug (silent data loss), a test that instantiates NavigationEnvCfg and asserts:

• cfg.scene.contact_forces.update_period == cfg.sim.dt
• cfg.sim.physics_material.static_friction == 1.0 (or whatever the terrain's configured value is)

would prevent regression. This should be added before merge.

🔵 Improvement: navigation_env_cfg.py:21 — Consider documenting the resolve_presets call
The module-level resolve_presets(LOW_LEVEL_ENV_CFG) is a side effect at import time. A brief comment explaining why this is necessary would help future maintainers understand that this isn't incidental but required for correctness when LOW_LEVEL_ENV_CFG attributes are referenced in class defaults.

[greptile-apps]

Greptile Summary

This PR fixes two bugs introduced by the Newton preset system: update_period being silently dropped when PresetCfg wrappers were resolved after the value was set, and sim.physics_material not being propagated from the terrain config. The fixes — calling resolve_presets at module import time and again in __post_init__, plus copying physics_material — match the pattern used by LocomotionVelocityRoughEnvCfg.

• One ordering concern: self.sim.physics_material is assigned from self.scene.terrain.physics_material before resolve_presets(self.scene) runs; if physics_material is ever a PresetCfg, the unresolved wrapper would be stored in self.sim and not fixed by the subsequent scene-only resolution.

Confidence Score: 4/5

Safe to merge for the current codebase; the physics_material ordering is a future-proofing concern, not a current defect.

All remaining findings are P2: the physics_material ordering issue is latent (physics_material is a concrete RigidBodyMaterialCfg today, not a PresetCfg), so no current breakage. The core fixes are correct and match the established pattern.

navigation_env_cfg.py — the ordering of physics_material assignment relative to resolve_presets(self.scene)

Important Files Changed

Filename	Overview
source/isaaclab_tasks/isaaclab_tasks/manager_based/navigation/config/anymal_c/navigation_env_cfg.py	Adds module-level resolve_presets(LOW_LEVEL_ENV_CFG) and in-post_init resolve_presets(self.scene) to fix PresetCfg wrapper issues; adds physics_material propagation — minor ordering concern: physics_material is assigned before presets are resolved.

Sequence Diagram

sequenceDiagram
    participant Module as Module Import
    participant LLCFG as LOW_LEVEL_ENV_CFG
    participant NavCfg as NavigationEnvCfg
    participant Scene as self.scene
    participant Sim as self.sim

    Module->>LLCFG: AnymalCFlatEnvCfg()
    Module->>LLCFG: resolve_presets(LOW_LEVEL_ENV_CFG)
    Note over LLCFG: All PresetCfg wrappers resolved

    Module->>NavCfg: class definition uses LOW_LEVEL_ENV_CFG.scene

    NavCfg->>NavCfg: __post_init__()
    NavCfg->>Sim: sim.physics_material = scene.terrain.physics_material
    Note over Sim: Assigned BEFORE resolve_presets(scene)
    NavCfg->>Scene: resolve_presets(self.scene)
    Note over Scene: Remaining PresetCfg nodes resolved
    NavCfg->>Scene: contact_forces.update_period = sim.dt

Loading

_{Reviews (1): Last reviewed commit: "Fixes navigation env config for PresetCf..." | Re-trigger Greptile}

[greptile-apps]

physics_material assigned before presets are resolved

self.sim.physics_material is assigned from self.scene.terrain.physics_material on line 142, before resolve_presets(self.scene) is called on line 149. If terrain.physics_material is ever wrapped in a PresetCfg (possible as the preset system expands), the copied value in self.sim.physics_material will be an unresolved wrapper and won't be patched by the subsequent resolve_presets(self.scene) call (which only walks self.scene, not self.sim). Swapping the order to resolve first, then assign, is safer and more consistent with the stated intent of this fix.

Suggested change

	self.sim.dt = LOW_LEVEL_ENV_CFG.sim.dt
	self.sim.render_interval = LOW_LEVEL_ENV_CFG.decimation
	self.sim.physics_material = self.scene.terrain.physics_material
	self.decimation = LOW_LEVEL_ENV_CFG.decimation * 10
	self.episode_length_s = self.commands.pose_command.resampling_time_range[1]
	# Resolve any PresetCfg wrappers in the scene so that update_period
	# is set on the actual sensor config rather than the wrapper (which
	# would be discarded by a later resolve_presets call).
	resolve_presets(self.scene)
	self.sim.dt = LOW_LEVEL_ENV_CFG.sim.dt
	self.sim.render_interval = LOW_LEVEL_ENV_CFG.decimation
	self.decimation = LOW_LEVEL_ENV_CFG.decimation * 10
	self.episode_length_s = self.commands.pose_command.resampling_time_range[1]
	# Resolve any PresetCfg wrappers in the scene so that update_period
	# is set on the actual sensor config rather than the wrapper (which
	# would be discarded by a later resolve_presets call).
	resolve_presets(self.scene)
	self.sim.physics_material = self.scene.terrain.physics_material