Add EventTermCfg.resample_interval_on_reset to EventCfg

[rubengrandia]

Description

Add a resample_interval_on_reset flag to EventTermCfg, defaulting to True to keep current behavior. This allows a user to skip the interval resampling that currently happens on reset.

See related Issue #5305

Type of change

• New feature (non-breaking change which adds functionality)

Checklist

• I have read and understood the contribution guidelines
• 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 NOT 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

[greptile-apps]

Greptile Summary

This PR adds resample_interval_on_reset: bool = True to EventTermCfg, giving users the ability to preserve a per-environment interval timer across episode resets instead of always restarting it. Both the main PyTorch-based EventManager and the experimental Warp-based EventManager are updated consistently.

• manager_term_cfg.py: New field resample_interval_on_reset (default True) with a thorough docstring explaining its interaction with is_global_time and the "interval" mode.
• event_manager.py (both variants): The reset() path now skips timer resampling when resample_interval_on_reset=False, and a new deterministic test (test_apply_interval_mode_resample_on_reset) verifies both branches using a zero-width interval range.

Confidence Score: 5/5

Safe to merge — the change is a single additive field with a backward-compatible default, and the core logic touch is a one-line guard in each manager's reset path.

The feature is minimal and well-contained: a new boolean field on EventTermCfg defaults to True so all existing configs are unaffected. Both the PyTorch and Warp manager implementations are updated symmetrically. The test uses a zero-width interval range to make assertions fully deterministic, and it covers both the resampling and non-resampling paths directly.

No files require special attention.

Important Files Changed

Filename	Overview
source/isaaclab/isaaclab/managers/manager_term_cfg.py	Adds resample_interval_on_reset: bool = True field to EventTermCfg with thorough docstring; default preserves existing behavior.
source/isaaclab/isaaclab/managers/event_manager.py	Adds and term_cfg.resample_interval_on_reset guard in reset() so per-environment timers are preserved across resets when the flag is False; logic is correct.
source/isaaclab_experimental/isaaclab_experimental/managers/event_manager.py	Mirrors the main manager change using an equivalent or not term_cfg.resample_interval_on_reset continue-guard in the Warp-based reset path.
source/isaaclab/test/managers/test_event_manager.py	Adds test_apply_interval_mode_resample_on_reset covering both flag values; uses a zero-width interval range for deterministic assertions.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[EventManager.reset called] --> B{interval terms present?}
    B -- No --> Z[return empty dict]
    B -- Yes --> C[for each interval term]
    C --> D{is_global_time?}
    D -- True --> C
    D -- False --> E{resample_interval_on_reset?}
    E -- False\nnew behavior --> C
    E -- True\ndefault --> F[Resample timer for reset env_ids]
    F --> C

Loading

_{Reviews (1): Last reviewed commit: "add contributor" | Re-trigger Greptile}

[isaaclab-review-bot]

Automated Review

Thanks for this well-crafted contribution, @rubengrandia! This is a thoughtful feature addition that addresses a real use case for interval-based events that should persist across episode resets.

Summary

Adds resample_interval_on_reset flag to EventTermCfg (defaulting to True for backward compatibility) to optionally preserve interval timers across environment resets.

Strengths

• Clean implementation: The logic change is minimal and surgical — just adding an extra condition to the existing reset guard
• Excellent documentation: The docstring clearly explains the behavior, the interaction with is_global_time, and when this flag is relevant
• Proper test coverage: The new test test_apply_interval_mode_resample_on_reset thoroughly validates both the default and non-default behavior using a deterministic fixed-width interval
• Backward compatible: Default value of True preserves existing behavior
• Complete implementation: Covers both the main EventManager and the experimental Warp-first implementation
• Changelog entries: Present for both isaaclab and isaaclab_experimental

Observations

1. Interaction clarity: The docstring nicely clarifies that this flag is orthogonal to is_global_time and only matters when is_global_time=False. The code logic mirrors this correctly.

2. Test design: Using a fixed interval range (interval_s, interval_s) is clever — it makes the test deterministic while still exercising the resampling path.

3. Minor note on the linked issue: The PR body references https://github.com/issues/created?issue=isaac-sim%7CIsaacLab%7C5305 which appears to be a malformed link. You might want to update it to the canonical form https://github.com/isaac-sim/IsaacLab/issues/5305 for traceability.

Overall, this is a well-scoped enhancement that follows the existing patterns in the codebase. The implementation is clean and the test coverage is solid.

---

This review was generated by an automated assistant. Please verify the feedback and use your judgment.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR adds a resample_interval_on_reset boolean flag to EventTermCfg (defaulting to True for backward compatibility) that allows interval-based event terms to preserve their per-environment timer across episode resets. This addresses the use case of modeling events whose firing period is independent of — or may exceed — episode length.

Design Assessment

Architecture: Well-designed and minimal. The implementation is surgical: a single additional boolean condition (and term_cfg.resample_interval_on_reset) gates the existing reset-resampling logic. This follows the same guard pattern already used for is_global_time, making it immediately readable to anyone familiar with the codebase.

Key design decisions:

• ✅ Default True preserves backward compatibility — no existing configs break
• ✅ Flag declared orthogonal to is_global_time — the docstring and code correctly handle that is_global_time=True already skips reset resampling, so resample_interval_on_reset only has observable effect when is_global_time=False
• ✅ Applied consistently to both the main EventManager and the experimental Warp-first implementation
• ✅ Changelog entries cover both packages

Findings

🟡 Minor suggestions (non-blocking)

1. Missing validation warning for irrelevant flag usage (event_manager.py):
   The existing code emits a logger.warning when min_step_count_between_reset != 0 is set on a non-"reset" mode term (lines 373–377 in _prepare_terms). For consistency, consider adding a similar warning when resample_interval_on_reset=False is set on a non-"interval" mode term. This would help users catch typos in their configs.

2. Issue link in PR description: The referenced issue link (https://github.com/issues/created?issue=isaac-sim%7CIsaacLab%7C5305) is a GitHub search URL rather than a canonical issue link. Consider updating to https://github.com/isaac-sim/IsaacLab/issues/5305 for proper cross-reference tracking.

3. Edge case: resample_interval_on_reset=False combined with is_global_time=True: While the docstring correctly states this combination is a no-op (global-time terms already ignore resets), there is no runtime warning if a user sets resample_interval_on_reset=False alongside is_global_time=True. This is perfectly safe but could indicate a user misunderstanding. A debug-level log or documentation note might help.

🟢 No issues found

• Tensor shapes: The env_ids indexing into _interval_term_time_left[index] is correct and unchanged — only the conditional gate is modified.
• Lifecycle correctness: The initialization path in _prepare_terms always samples initial time regardless of resample_interval_on_reset, which is correct (the flag only controls resampling on reset).
• Warp implementation consistency: The experimental manager uses if term_cfg.is_global_time or not term_cfg.resample_interval_on_reset: continue which is logically equivalent to the main manager's if not term_cfg.is_global_time and term_cfg.resample_interval_on_reset: (De Morgan's law). Both correctly skip resampling.
• No silent failure paths: The flag is a simple boolean with a default; there are no paths where an unexpected value could cause silent misbehavior.

Test Coverage

Adequate. The new test test_apply_interval_mode_resample_on_reset is well-designed:

• Uses a fixed-width interval (1.0, 1.0) to make assertions deterministic
• Tests both the default (resample) and non-default (preserve) behaviors side-by-side
• Verifies initial state, post-apply state, and post-reset state
• Uses torch.testing.assert_close for proper floating-point comparison

Potential additions (not required for merge):

• A test with partial reset (env_ids being a subset) to confirm only the specified environments get their timer resampled/preserved
• A test verifying the timer eventually fires after accumulating enough simulation time across multiple episodes (integration-level)
• Tests for the experimental Warp-first manager (though this may be covered by a separate test suite)

Verdict

No issues found. This is a clean, well-documented, backward-compatible feature addition with solid test coverage. Ready to merge.

[ooctipus]

LGTM