-
Notifications
You must be signed in to change notification settings - Fork 2.9k
[Bug Fix] Instantiate event class term_cfg.func with parameters to enable resetting #4131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Kyle Morgenstein <[email protected]>
Greptile OverviewGreptile SummaryFixed a critical bug where class-based event terms were stored as uninstantiated classes, causing Key Changes:
Impact:
Confidence Score: 5/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant EM as EventManager
participant Term as EventTermCfg
participant ClassTerm as Class-based Term
participant Instance as Term Instance
Note over EM,Instance: Before Fix (Broken)
EM->>Term: Detect inspect.isclass(term_cfg.func)
EM->>EM: Store class (not instance) in _mode_class_term_cfgs
EM->>ClassTerm: Later call reset(env_ids)
ClassTerm-->>EM: Error: missing required 'self' argument
Note over EM,Instance: After Fix (Working)
EM->>Term: Detect inspect.isclass(term_cfg.func)
EM->>ClassTerm: Instantiate: term_cfg.func(cfg=term_cfg, env=self._env)
ClassTerm->>Instance: Create instance
EM->>EM: Store instance in _mode_class_term_cfgs
EM->>Instance: Later call reset(env_ids)
Instance-->>EM: Success: bound method executes correctly
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, no comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix
It looks right to me, but I'm having a bit low confidence on cases if it ever double constructs term.
Will you be willing to write a test for it?
|
|
||
| # check if the term is a class | ||
| if inspect.isclass(term_cfg.func): | ||
| term_cfg.func = term_cfg.func(cfg=term_cfg, env=self._env) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be handled by ManagerBase class inside the _process_term_cfg_at_play function.
Description
Event manager needs to instantiate class-based terms or else func.reset(env_ids) will not work (requires positional argument self).
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there