Skip to content

Conversation

Milad-Rakhsha-NV
Copy link
Contributor

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
List any dependencies that are required for this change.

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (existing functionality will not work without user modification)
  • Documentation update

Screenshots

Please attach before and after screenshots of the change if applicable.

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 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

class randomize_rigid_body_material(ManagerTermBase):
def __init__(self, cfg: EventTermCfg, env: ManagerBasedEnv):
raise NotImplementedError("Not implemented")
"""Initialize the term.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this concept of bucketing for Newton?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, @Milad-Rakhsha-NV there is no limit of materials that can be applied in Newton right? They should just be parameters for MJWarp to process.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah there is no limit but since I wanted to keep API consistency with the physX-based lab I just did the same thing here;
we can keep the same API and just not use some parameters internally (e.g. num_buckets/restitution which aren't used)
or we can just have a nen API for Newton (which I wouldn't prefer for obvious reasons)

Copy link
Collaborator

@AntoineRichard AntoineRichard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Milad for the PR! It looks great though there are some concerns around the sampling scheme. Could you take a look? Thanks!

class randomize_rigid_body_material(ManagerTermBase):
def __init__(self, cfg: EventTermCfg, env: ManagerBasedEnv):
raise NotImplementedError("Not implemented")
"""Initialize the term.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, @Milad-Rakhsha-NV there is no limit of materials that can be applied in Newton right? They should just be parameters for MJWarp to process.

Comment on lines 67 to 74
# obtain number of shapes per body (needed for indexing the material properties correctly)
# note: this is a workaround since the Articulation does not provide a direct way to obtain the number of shapes
# per body. We use the physics simulation view to obtain the number of shapes per body.
self.num_shapes_per_body = None
if isinstance(self.asset, Articulation) and self.asset_cfg.body_ids != slice(None):
self.num_shapes_per_body = []
for shapes in self.asset.root_newton_view.body_shapes:
self.num_shapes_per_body.append(len(shapes))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move this to the articulation? Newton does provide a body/shape mapping.

Comment on lines 87 to 94
self.material_buckets = math_utils.sample_uniform(
ranges[:, 0], ranges[:, 1], (num_buckets, 3), device=self.asset.device
)

# ensure dynamic friction is always less than static friction
make_consistent = cfg.params.get("make_consistent", False)
if make_consistent:
self.material_buckets[:, 1] = torch.min(self.material_buckets[:, 0], self.material_buckets[:, 1])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there is no limit, we should sample these values dynamically on every call. Could you change the behavior to that?

Comment on lines 67 to 69
# obtain number of shapes per body (needed for indexing the material properties correctly)
# note: this is a workaround since the Articulation does not provide a direct way to obtain the number of shapes
# per body. We use the physics simulation view to obtain the number of shapes per body.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to update the comments! :)

material_mu[env_ids, :] = material_samples[:, :, 0]

# apply to simulation
mask = torch.zeros((env.scene.num_envs,), dtype=torch.bool, device=env.device)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be cached.

# retrieve material buffer from the physics simulation
# materials = self.asset.root_physx_view.get_material_properties()
material_mu = wp.to_torch(
self.asset.root_newton_view.get_attribute("shape_material_mu", self.asset.root_newton_model)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be cached. You could save the "default" friction on init or on the first call. Also, if the tensor is contiguous then there is no need to clone as the values could be directly changed in place without the need to call the set function.

if make_consistent:
self.material_buckets[:, 1] = torch.min(self.material_buckets[:, 0], self.material_buckets[:, 1])

def __call__(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider using warp for this? @Mayankm96 do you think it would make sense. Using warp would avoid copies and in general make the update much faster as we could do this whole function in a couple of lines. It's not really visible from the user POV so I'd say it's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for mjwarp actually this is not used since there is a single friction value (not including the spinning/rolling frictions) that can be set to the solver.

Comment on lines 113 to 114
# retrieve material buffer from the physics simulation
# materials = self.asset.root_physx_view.get_material_properties()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These would need to removed?

Comment on lines 120 to 135
bucket_ids = torch.randint(0, num_buckets, (len(env_ids), total_num_shapes), device=self.asset.device)
material_samples = self.material_buckets[bucket_ids]
# print("material_mu before update:\n", material_mu.shape, material_mu)
# update material buffer with new samples
if self.num_shapes_per_body is not None:
# sample material properties from the given ranges
for body_id in self.asset_cfg.body_ids:
# obtain indices of shapes for the body
start_idx = sum(self.num_shapes_per_body[:body_id])
end_idx = start_idx + self.num_shapes_per_body[body_id]
# assign the new materials
# material samples are of shape: num_env_ids x total_num_shapes x 3
material_mu[env_ids, start_idx:end_idx] = material_samples[:, start_idx:end_idx, 0]
else:
# assign all the materials
material_mu[env_ids, :] = material_samples[:, :, 0]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update to use direct sampling if possible!

Comment on lines 145 to 146
# material_mu = wp.to_torch(self.asset.root_newton_view.get_attribute("shape_material_mu", self.asset.root_newton_model)).clone()
# print("material_mu after update:\n", material_mu.shape, material_mu)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be removed

@AntoineRichard AntoineRichard changed the title Added friction randomization event [Newton] Add friction randomization event Sep 22, 2025
@Milad-Rakhsha-NV Milad-Rakhsha-NV force-pushed the milad/friction-randomization branch 4 times, most recently from 93ac402 to be1f27a Compare September 22, 2025 23:48
@Milad-Rakhsha-NV Milad-Rakhsha-NV force-pushed the milad/friction-randomization branch from be1f27a to 81490c2 Compare September 23, 2025 01:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants