implement passive damping for the featherstone solver#2304
Open
purmecia wants to merge 5 commits intonewton-physics:mainfrom
Open
implement passive damping for the featherstone solver#2304purmecia wants to merge 5 commits intonewton-physics:mainfrom
purmecia wants to merge 5 commits intonewton-physics:mainfrom
Conversation
Signed-off-by: purmecia <purmecia@gmail.com> Signed-off-by: purmecia <995460208@qq.com>
Contributor
📝 WalkthroughWalkthroughAdded a per-DOF joint damping field parsed from MJCF Changes
Sequence DiagramsequenceDiagram
participant MJCF as MJCF Input
participant Parser as import_mjcf (Parser)
participant Builder as ModelBuilder
participant Model as Model
participant Solver as SolverFeatherstone
participant Kernel as eval_rigid_tau (kernel)
participant Force as joint_force
MJCF->>Parser: parse joint `damping`
Parser->>Builder: set JointDofConfig.damping
Builder->>Builder: append to joint_damping array
Builder->>Model: finalize -> model.joint_damping (wp.array)
Solver->>Model: read model.joint_damping
Solver->>Kernel: launch kernel with joint_damping
Kernel->>Kernel: loop DOFs -> d = joint_damping[j]
Kernel->>Force: call joint_force(..., damping=d)
Force->>Force: compute passive = -d * qd
Force-->>Kernel: return augmented joint force
Kernel-->>Solver: accumulate tau -> update qdd
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
purmecia
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
purmecia
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
newton/_src/sim/builder.py (2)
3710-3772:⚠️ Potential issue | 🟠 MajorMirror
passive_dampinginadd_joint_ball()as well.Right now the new default/override path only exists for revolute and prismatic joints.
add_joint_ball()still synthesizes its three DOFs fromdefault_joint_cfg.armature/frictiononly, sobuilder.default_joint_cfg.passive_dampingis silently ignored for ball joints.Also applies to: 3806-3867
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/sim/builder.py` around lines 3710 - 3772, The ball-joint builder currently ignores builder.default_joint_cfg.passive_damping; update add_joint_ball to accept the passive_damping parameter (or use the existing one if already present) and, when synthesizing the three DOFs, set each DOF's passive_damping to passive_damping if not None else self.default_joint_cfg.passive_damping; specifically modify the code that constructs the three ModelBuilder.JointDofConfig instances in add_joint_ball so their passive_damping fields mirror the same override logic used in add_joint_revolute/add_joint_prismatic (refer to add_joint_ball, ModelBuilder.JointDofConfig, and default_joint_cfg.passive_damping).
435-463:⚠️ Potential issue | 🟡 MinorReject negative
passive_dampingvalues.A negative coefficient flips
-damping * qdinto anti-damping, so a typo here injects energy instead of dissipating it. Validating this once inJointDofConfigwould protect builder defaults, direct API calls, and imported MJCF values through the same entry point.Proposed fix
self.target_kd = target_kd """The derivative gain of the target drive PD controller. Defaults to 0.0.""" self.passive_damping = passive_damping """Passive velocity damping that is always active. Defaults to 0.0.""" + if self.passive_damping < 0.0: + raise ValueError("passive_damping must be non-negative.") self.armature = armature """Artificial inertia added around the joint axis [kg·m² or kg]. Defaults to 0."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/sim/builder.py` around lines 435 - 463, The passive_damping field in JointDofConfig (constructor in builder.py) must be validated to reject negative values: in JointDofConfig.__init__ (where passive_damping is assigned) add a guard that checks if passive_damping < 0 and raise a ValueError with a clear message including the invalid value (e.g., "passive_damping must be non-negative"), so builder defaults, direct API calls, and MJCF imports all get the same validation through JointDofConfig.
🧹 Nitpick comments (3)
newton/_src/sim/model.py (1)
458-459: Add SI units to docstring for consistency.Other joint damping fields like
joint_target_kdinclude units[N·s/m or N·m·s/rad, depending on joint type]. Thejoint_passive_dampingdocstring should follow the same convention.self.joint_passive_damping: wp.array[wp.float32] | None = None - """Passive velocity damping always active on the joint, shape [joint_dof_count], float.""" + """Passive velocity damping [N·s/m or N·m·s/rad, depending on joint type] always active on the joint, shape [joint_dof_count], float."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/sim/model.py` around lines 458 - 459, The docstring for self.joint_passive_damping is missing SI units; update its triple-quoted comment to match the style of joint_target_kd by appending the units "[N·s/m or N·m·s/rad, depending on joint type]" (i.e., make the docstring read something like "Passive velocity damping always active on the joint, shape [joint_dof_count], float. Units: [N·s/m or N·m·s/rad, depending on joint type]") so it is consistent with joint_target_kd and other joint damping fields.newton/_src/solvers/featherstone/kernels.py (1)
406-408: Trailing whitespace on line 407.There's a trailing whitespace/empty space after the assignment on line 407.
🧹 Remove trailing whitespace
passive_damping = joint_passive_damping[j] - + drive_f = joint_force(q, qd, target_pos, target_vel, target_ke, target_kd, lower, upper, limit_ke, limit_kd, passive_damping)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/solvers/featherstone/kernels.py` around lines 406 - 408, Remove the trailing whitespace after the assignment to passive_damping in the block where passive_damping = joint_passive_damping[j] is set; ensure the line with the assignment has no trailing spaces so the subsequent call to joint_force(q, qd, target_pos, target_vel, target_ke, target_kd, lower, upper, limit_ke, limit_kd, passive_damping) sits on the next line cleanly without extra whitespace characters.newton/_src/sim/builder.py (1)
462-463: Add SI units to the newpassive_dampingdocs.The new public API docs explain the behavior, but not the units callers are expected to pass.
As per coding guidelines "Include SI units for physical quantities in public API docstrings:
\"\"\"Particle positions [m], shape [particle_count, 3].\"\"\". Joint-dependent:[m or rad]. Spatial vectors:[N, N·m]. Compound arrays: per-component. Skip non-physical fields."Also applies to: 3740-3740, 3835-3835
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/sim/builder.py` around lines 462 - 463, The docstring for the new public attribute passive_damping is missing SI units; update the docstring for passive_damping (the attribute set in the class in builder.py) to include its units (e.g., "[s^-1]" for a velocity-damping coefficient) following the project's style (include units in brackets after the description), and apply the same units-formatting to the other listed occurrences (the other passive_damping/docstring locations referenced) so all public API docstrings consistently state the expected SI units.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@newton/_src/sim/builder.py`:
- Around line 3710-3772: The ball-joint builder currently ignores
builder.default_joint_cfg.passive_damping; update add_joint_ball to accept the
passive_damping parameter (or use the existing one if already present) and, when
synthesizing the three DOFs, set each DOF's passive_damping to passive_damping
if not None else self.default_joint_cfg.passive_damping; specifically modify the
code that constructs the three ModelBuilder.JointDofConfig instances in
add_joint_ball so their passive_damping fields mirror the same override logic
used in add_joint_revolute/add_joint_prismatic (refer to add_joint_ball,
ModelBuilder.JointDofConfig, and default_joint_cfg.passive_damping).
- Around line 435-463: The passive_damping field in JointDofConfig (constructor
in builder.py) must be validated to reject negative values: in
JointDofConfig.__init__ (where passive_damping is assigned) add a guard that
checks if passive_damping < 0 and raise a ValueError with a clear message
including the invalid value (e.g., "passive_damping must be non-negative"), so
builder defaults, direct API calls, and MJCF imports all get the same validation
through JointDofConfig.
---
Nitpick comments:
In `@newton/_src/sim/builder.py`:
- Around line 462-463: The docstring for the new public attribute
passive_damping is missing SI units; update the docstring for passive_damping
(the attribute set in the class in builder.py) to include its units (e.g.,
"[s^-1]" for a velocity-damping coefficient) following the project's style
(include units in brackets after the description), and apply the same
units-formatting to the other listed occurrences (the other
passive_damping/docstring locations referenced) so all public API docstrings
consistently state the expected SI units.
In `@newton/_src/sim/model.py`:
- Around line 458-459: The docstring for self.joint_passive_damping is missing
SI units; update its triple-quoted comment to match the style of joint_target_kd
by appending the units "[N·s/m or N·m·s/rad, depending on joint type]" (i.e.,
make the docstring read something like "Passive velocity damping always active
on the joint, shape [joint_dof_count], float. Units: [N·s/m or N·m·s/rad,
depending on joint type]") so it is consistent with joint_target_kd and other
joint damping fields.
In `@newton/_src/solvers/featherstone/kernels.py`:
- Around line 406-408: Remove the trailing whitespace after the assignment to
passive_damping in the block where passive_damping = joint_passive_damping[j] is
set; ensure the line with the assignment has no trailing spaces so the
subsequent call to joint_force(q, qd, target_pos, target_vel, target_ke,
target_kd, lower, upper, limit_ke, limit_kd, passive_damping) sits on the next
line cleanly without extra whitespace characters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: fc085151-1267-4439-af86-8b0029efe78c
📒 Files selected for processing (9)
CHANGELOG.mdnewton/_src/sim/builder.pynewton/_src/sim/model.pynewton/_src/solvers/featherstone/kernels.pynewton/_src/solvers/featherstone/solver_featherstone.pynewton/_src/solvers/flags.pynewton/_src/solvers/semi_implicit/kernels_body.pynewton/_src/utils/import_mjcf.pynewton/tests/test_import_mjcf.py
purmecia
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
purmecia
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
purmecia
temporarily deployed
to
external-pr-approval
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/sim/builder.py (1)
3761-3773:⚠️ Potential issue | 🟡 MinorAvoid silent ignore when
axisisJointDofConfigandpassive_dampingis also passed.If
axisis aModelBuilder.JointDofConfig, the newpassive_damping=argument is ignored silently. Please raise aTypeError(or explicitly document precedence) to prevent accidental misconfiguration.💡 Suggested guard
if isinstance(axis, ModelBuilder.JointDofConfig): + if passive_damping is not None: + raise TypeError( + "Cannot pass passive_damping when axis is JointDofConfig; " + "set axis.passive_damping instead." + ) ax = axisApply similarly in both
add_joint_revolute()andadd_joint_prismatic().Also applies to: 3858-3870
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/sim/builder.py` around lines 3761 - 3773, When axis is provided as a ModelBuilder.JointDofConfig the passed-in passive_damping is currently ignored; change both add_joint_revolute and add_joint_prismatic so they check if isinstance(axis, ModelBuilder.JointDofConfig) and passive_damping is not None, and in that case raise a TypeError describing that passive_damping cannot be passed when axis is a JointDofConfig (or alternatively document precedence). Ensure the check references ModelBuilder.JointDofConfig, the passive_damping parameter name, and is applied in the same manner in both methods (also update the same guard at the other location around lines 3858-3870).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/sim/builder.py`:
- Line 463: Update the passive-damping parameter docstrings so each joint method
only states its own unit: in add_joint_prismatic() describe damping as linear
units ("N·s/m") and in add_joint_revolute() describe damping with the
repo-preferred angular wording ("torque per radian") instead of using "N·m/rad"
notation; apply the same replacement to other passive-damping docstrings in this
file (e.g., the other occurrences referenced) so all angular damping
descriptions use "torque per radian" and linear ones use "N·s/m".
---
Outside diff comments:
In `@newton/_src/sim/builder.py`:
- Around line 3761-3773: When axis is provided as a ModelBuilder.JointDofConfig
the passed-in passive_damping is currently ignored; change both
add_joint_revolute and add_joint_prismatic so they check if isinstance(axis,
ModelBuilder.JointDofConfig) and passive_damping is not None, and in that case
raise a TypeError describing that passive_damping cannot be passed when axis is
a JointDofConfig (or alternatively document precedence). Ensure the check
references ModelBuilder.JointDofConfig, the passive_damping parameter name, and
is applied in the same manner in both methods (also update the same guard at the
other location around lines 3858-3870).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 3066ed27-85a3-4ade-b718-58170be2a9cb
📒 Files selected for processing (4)
CHANGELOG.mdnewton/_src/sim/builder.pynewton/_src/sim/model.pynewton/_src/solvers/featherstone/kernels.py
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/_src/solvers/featherstone/kernels.py
Member
|
Clean implementation of per-DOF passive damping through the Featherstone and semi-implicit solvers. The main question worth settling before this merges is whether |
eric-heiden
reviewed
Apr 4, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
purmecia
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
purmecia
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
purmecia
requested a deployment
to
external-pr-approval
GitHub Actions
Waiting
purmecia
requested a deployment
to
external-pr-approval
GitHub Actions
Waiting
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
newton/tests/test_import_mjcf.py (1)
3202-3217:⚠️ Potential issue | 🟠 MajorFix overwritten damping variable so both damping sources are actually validated.
Line 3202 overwrites the earlier
model.mujoco.dof_passive_dampingarray, and Line 3216/Line 3217 then assert the same array twice. This drops the intended parity check between MuJoCo andmodel.joint_damping.Suggested fix
- joint_stiffness = model.mujoco.dof_passive_stiffness.numpy() - joint_damping = model.mujoco.dof_passive_damping.numpy() + joint_stiffness = model.mujoco.dof_passive_stiffness.numpy() + joint_damping_mujoco = model.mujoco.dof_passive_damping.numpy() joint_target_ke = model.joint_target_ke.numpy() joint_target_kd = model.joint_target_kd.numpy() - joint_damping = model.joint_damping.numpy() + joint_damping_model = model.joint_damping.numpy() @@ - self.assertAlmostEqual(joint_damping[dof_idx], expected["damping"], places=4) - self.assertAlmostEqual(joint_damping[dof_idx], expected["damping"], places=4) + self.assertAlmostEqual(joint_damping_mujoco[dof_idx], expected["damping"], places=4) + self.assertAlmostEqual(joint_damping_model[dof_idx], expected["damping"], places=4)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/tests/test_import_mjcf.py` around lines 3202 - 3217, The test currently overwrites the earlier MuJoCo damping array and then asserts the same variable twice; preserve both arrays by reading model.joint_damping and model.mujoco.dof_passive_damping into two distinct variables (e.g., joint_damping and mujo_passive_damping) before the loop, and inside the for loop assert mujo_passive_damping[dof_idx] against expected["damping"] and assert joint_damping[dof_idx] against the intended expected value (e.g., expected["target_kd"] or the correct expected key), ensuring you reference model.joint_damping and model.mujoco.dof_passive_damping by their variable names in the assertions.newton/_src/sim/builder.py (1)
424-494:⚠️ Potential issue | 🟠 MajorMove the
dampingparameter to keyword-only to preserve positional API compatibility.The
dampingparameter was inserted before existing parameters inJointDofConfig.__init__(),add_joint_revolute(), andadd_joint_prismatic(). This breaks the positional API: any call passing arguments starting fromarmatureonward positionally will silently bind those values to the newdampingparameter and shift the rest.While in-repo call sites do not currently use positional arguments beyond this threshold, this is a public API breaking change that violates the "breaking changes require a deprecation first" guideline. Move
dampingto keyword-only using*to keep the signature clean and compatible:Proposed signature changes
target_ke: float = 0.0, target_kd: float = 0.0, armature: float = 0.0, effort_limit: float = 1e6, velocity_limit: float = 1e6, friction: float = 0.0, actuator_mode: JointTargetMode | None = None, + *, + damping: float = 0.0,Apply the same change to
add_joint_revolute()andadd_joint_prismatic().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/sim/builder.py` around lines 424 - 494, The new damping parameter in ModelBuilder.JointDofConfig.__init__ was inserted as a positional parameter and breaks positional API compatibility; make damping keyword-only to avoid shifting existing positional arguments by moving a bare '*' before the damping parameter in JointDofConfig.__init__ and apply the same change to the signatures of add_joint_revolute() and add_joint_prismatic() so damping (and any subsequent params) must be passed by keyword, preserving existing positional bindings for armature, effort_limit, velocity_limit, etc.
🧹 Nitpick comments (1)
newton/_src/solvers/semi_implicit/kernels_body.py (1)
233-240: BALL joint does not apply passive damping.The BALL joint path does not call
joint_force(), so the new passive damping term will not be applied to ball joints even ifjoint_dampingvalues are set for their DOFs. This may be intentional given the existing TODO comments, but creates an inconsistency with other multi-DOF joints (D6 with 3 angular axes does apply damping).If BALL joints should support passive damping, the implementation would need to incorporate the damping term into the torque calculation:
# Sketch (not a complete fix): t_total += wp.vec3(-joint_f[qd_start], -joint_f[qd_start + 1], -joint_f[qd_start + 2]) # Add passive damping per angular DOF t_total -= wp.vec3( joint_damping[qd_start] * wp.dot(w_err, axis_0), joint_damping[qd_start + 1] * wp.dot(w_err, axis_1), joint_damping[qd_start + 2] * wp.dot(w_err, axis_2), )Do you want me to open a follow-up issue to track adding passive damping support for BALL joints?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/solvers/semi_implicit/kernels_body.py` around lines 233 - 240, The BALL joint branch (JointType.BALL) never applies the new passive damping because it doesn't invoke joint_force() or add damping to t_total; update the BALL path so t_total includes the passive damping term similar to the D6/other multi-DOF joints by reading joint_damping[qd_start..qd_start+2] and projecting w_err onto the joint angular axes (or using w_err components in joint-local coords) and subtracting those damping torques from t_total, or simply call the existing joint_force(...) helper with the ball angular DOFs so the same damping logic is reused; reference symbols: JointType.BALL, joint_force(), t_total, joint_damping, qd_start, and w_err (and local axes e.g. axis_0/axis_1/axis_2) to locate and implement this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/solvers/featherstone/solver_featherstone.py`:
- Line 514: The kernel call passes model.joint_damping (a JOINT_DOF‑shaped
array) but jcalc_tau in kernels.py indexes it with the joint loop index j;
change the kernel to pass per‑DOF values or update jcalc_tau to index
joint_damping using the DOF offset (qd_start + axis) instead of j so multi‑DOF
joints (BALL, FREE, DISTANCE, D6) get correct damping; locate the joint_damping
usage and replace joint_damping[j] with joint_damping[qd_start + axis] (or
compute the DOF index before the axis loop) so it aligns with how other
DOF‑level properties are accessed.
---
Outside diff comments:
In `@newton/_src/sim/builder.py`:
- Around line 424-494: The new damping parameter in
ModelBuilder.JointDofConfig.__init__ was inserted as a positional parameter and
breaks positional API compatibility; make damping keyword-only to avoid shifting
existing positional arguments by moving a bare '*' before the damping parameter
in JointDofConfig.__init__ and apply the same change to the signatures of
add_joint_revolute() and add_joint_prismatic() so damping (and any subsequent
params) must be passed by keyword, preserving existing positional bindings for
armature, effort_limit, velocity_limit, etc.
In `@newton/tests/test_import_mjcf.py`:
- Around line 3202-3217: The test currently overwrites the earlier MuJoCo
damping array and then asserts the same variable twice; preserve both arrays by
reading model.joint_damping and model.mujoco.dof_passive_damping into two
distinct variables (e.g., joint_damping and mujo_passive_damping) before the
loop, and inside the for loop assert mujo_passive_damping[dof_idx] against
expected["damping"] and assert joint_damping[dof_idx] against the intended
expected value (e.g., expected["target_kd"] or the correct expected key),
ensuring you reference model.joint_damping and model.mujoco.dof_passive_damping
by their variable names in the assertions.
---
Nitpick comments:
In `@newton/_src/solvers/semi_implicit/kernels_body.py`:
- Around line 233-240: The BALL joint branch (JointType.BALL) never applies the
new passive damping because it doesn't invoke joint_force() or add damping to
t_total; update the BALL path so t_total includes the passive damping term
similar to the D6/other multi-DOF joints by reading
joint_damping[qd_start..qd_start+2] and projecting w_err onto the joint angular
axes (or using w_err components in joint-local coords) and subtracting those
damping torques from t_total, or simply call the existing joint_force(...)
helper with the ball angular DOFs so the same damping logic is reused; reference
symbols: JointType.BALL, joint_force(), t_total, joint_damping, qd_start, and
w_err (and local axes e.g. axis_0/axis_1/axis_2) to locate and implement this
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: f3e42c80-00bc-407a-93cd-3bb50fa24ca0
📒 Files selected for processing (9)
CHANGELOG.mdnewton/_src/sim/builder.pynewton/_src/sim/model.pynewton/_src/solvers/featherstone/kernels.pynewton/_src/solvers/featherstone/solver_featherstone.pynewton/_src/solvers/flags.pynewton/_src/solvers/semi_implicit/kernels_body.pynewton/_src/utils/import_mjcf.pynewton/tests/test_import_mjcf.py
✅ Files skipped from review due to trivial changes (1)
- newton/_src/solvers/flags.py
🚧 Files skipped from review as they are similar to previous changes (3)
- newton/_src/utils/import_mjcf.py
- newton/_src/solvers/featherstone/kernels.py
- newton/_src/sim/model.py
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
3 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.
Description
SolverFeatherstoneignores the MJCF<joint damping="...">attribute. The MJCF parser reads the value but only registers it inSolverMuJoCo, so the Featherstone joint-force computation never sees it.This PR adds a
passive_dampingfield that flows through the standardJointDofConfig → builder list → Model.joint_passive_damping → joint_force()path, making the MJCFdampingattribute work for the Featherstone solver. It adds a passive velocity-proportional damping force of -damping * qd that is always active, independent of joint limits or target drives.Closes #2303
Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
Running the reproduction script (damped pendulum,
damping=5.0) with this fix, the pendulum correctly decays to rest.Bug fix
Steps to reproduce:
Minimal reproduction:
New feature / API change
Summary by CodeRabbit
New Features
dampingattributes are now imported and applied in simulation (previously ignored).Tests