fix(actuators): prevent cross-type parameter leakage in default classes - #3563
Open
anishesg wants to merge 2 commits into
Open
fix(actuators): prevent cross-type parameter leakage in default classes#3563anishesg wants to merge 2 commits into
anishesg wants to merge 2 commits into
Conversation
…ross-type default inheritance Actuator defaults defined for one subclass (such as `<position>`) were incorrectly being inherited by other actuator types (like `<motor>`, `<muscle>`, `<damper>`, etc.) when those actuators were created using the same default class. This happened because the bias parameters were not being cleared when converting an actuator to a specific type. Signed-off-by: anish <anishesg@users.noreply.github.com>
2 tasks
|
Thanks for looking at this, but I don't think this addresses the core requirements of #3561. Every actuator shortcut in a default class writes into the same actuator struct, so they still clobber each other while the default is parsed. Zeroing |
…pecific setup Move parameter clearing from mjs_setTo* functions to the XML reader's OneActuator function. This prevents cross-type parameter leakage when multiple actuator shortcuts are defined in the same default class, while preserving same-type inheritance (e.g., position under position default). The key insight is that parameters should be cleared before reading XML attributes for inheritance, not after. This way: - Cross-type contamination is prevented (different types start fresh) - Same-type inheritance works (inherited params are read before calling mjs_setTo*) - General and plugin actuators are unaffected Signed-off-by: anish <anishesg@users.noreply.github.com>
Contributor
Author
|
good catch, moved the fix to the XML parser. now clears gainprm/biasprm/dynprm before reading attributes for inheritance, so cross-type leakage is prevented but same-type inheritance still works |
anishesg
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When multiple actuator shortcut types (e.g.,
<position>,<motor>,<velocity>) are defined within the same default class, they all write to the same actuator struct, causing parameters from one type to leak into others.The fix clears the parameter arrays (
gainprm,biasprm,dynprm) in the XML parser before setting type-specific values. This happens inOneActuator()right after determining the actuator type, but before reading XML attributes for inheritance.This approach:
<position kp="5"/>under a<position kv="3"/>default correctly inherits kv=3)The key insight is that parameters must be cleared before reading attributes (which use current values as defaults), not after in the
mjs_setTo*functions.Fixes #3561