Skip to content

Fixes inconsistent data reading in body, link, com for RigidObject, RigidObjectCollection and Articulation #2736

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

Merged
merged 4 commits into from
Jun 25, 2025

Conversation

ooctipus
Copy link
Contributor

@ooctipus ooctipus commented Jun 18, 2025

Description

When WriteState, WriteLink, WriteCOM, WriteJoint are invoked, there is a inconsistency when reading values of ReadState, ReadLink, ReadCOM. The Source of the bug is because of missing timestamp invalidation of relative data or missing update to the related data within the write function. Below I list the all functions that is problematics

RigitObject:
write_root_link_pose_to_sim
write_root_com_velocity_to_sim

RigitObjectCollection:
write_object_link_pose_to_sim
write_object_com_velocity_to_sim

Articulation:
write_joint_state_to_sim

The bug if fixed by invalidating the relevant data timestamps in write_joint_state_to_sim function for articulation, and added direct update to the dependent data in write_(state|link|com)_to_sim of RigitObject and RigitObjectCollection.

I have added the tests cases that checks the consistency among ReadState, ReadLink, ReadCOM when either WriteState, WriteLink, WriteCOM, WriteJoint is called and passed all tests.

Fixes #2534 #2702

Type of change

Bug fix (non-breaking change which fixes an issue)

Checklist

  • 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

… pos, quat, pose, state when write operation get performed
@ooctipus ooctipus force-pushed the feat/fix_data_inconsistencies_in_assets_read branch from 79589b7 to 2b78dd5 Compare June 18, 2025 23:23
@ooctipus ooctipus changed the title fix data consistency between body, link, com, and consistency between… fix inconsistent data reading in body, link, com for RigidObject, RigidObjectCollection and Articulation Jun 18, 2025
@kellyguo11 kellyguo11 changed the title fix inconsistent data reading in body, link, com for RigidObject, RigidObjectCollection and Articulation Fixes inconsistent data reading in body, link, com for RigidObject, RigidObjectCollection and Articulation Jun 19, 2025
@kellyguo11
Copy link
Contributor

@Mayankm96 @ozhanozen could you please help review this one to make sure it resolves the reported issues?

@ozhanozen
Copy link
Contributor

@Mayankm96 @ozhanozen could you please help review this one to make sure it resolves the reported issues?

Hi @kellyguo11, I’d be happy to help.

Quick thought: for pure-derived properties like body_state_w, root_com_pose_w, etc., which only concatenate or apply simple math and don’t call PhysX directly, could we drop their timestamp guards and recompute them on access instead? This would remove a lot of boilerplate and potential timestamp bugs with a negligible perf hit. What do you think?

@ooctipus
Copy link
Contributor Author

Hi @ozhanozen
Your concern is right, I am also not very sure what will be the best design-intent here, @Mayankm96 may have more thoughts on this. What I did is to write tests to ensure the data consistency so that the issue you saw(I also experienced the same issue in RigidBody.write) is tested before future code release. I think the test should be complete, in english, they are testing that for RigidBodies, Articulation, RigidBodiesCollection, root_(state|pos|quat|vel|ang), bodylink_(state|pos|quat|vel|ang), com_(state|pos|quat|vel|ang) must agree after any write operation.

@kellyguo11, feel free to add more input on this matter.

@kellyguo11
Copy link
Contributor

I agree it'd be less error-prone to remove the lazy updates if there are no perf hits, though it would be good to profile the operations to make sure there is indeed no impact on the performance before we make that change. Some of the torch operations could still be costly if called many times within a step.

@kellyguo11 kellyguo11 merged commit c75bc5c into main Jun 25, 2025
3 of 4 checks passed
@kellyguo11 kellyguo11 deleted the feat/fix_data_inconsistencies_in_assets_read branch June 25, 2025 20:40
@jtigue-bdai
Copy link
Collaborator

@kellyguo11 did we get a feel for the speed performance from this?

@kellyguo11
Copy link
Contributor

IIRC the initial change for optimizing the APIs showed slight performance improvement, but not too significant. @ooctipus did you get to do some performance analysis with the latest changes here?

@jtigue-bdai
Copy link
Collaborator

Ok that works. I know these buffers have been monkeyed around with a lot in the last year. I think consistent beets performance

@ooctipus
Copy link
Contributor Author

ooctipus commented Jun 25, 2025

I didn't do any formal analysis with code. But I think it is pretty easy to reason it,
last version were invalidating the timestamp where as the mayank's newer version transitions from some timestamp-invalidation to direct buffer modification upon write. This fix is matching the new style.

we basically have 3 ways to do about it:

1: get from physics:
pro: more direct
con: incur expense from physx everytime

2: invalidate time step
pro: save compute time, if that variable is never called
con: not aligning with current code how buffer is directly modified

3: direct update to all buffers (current)
pro: 1 time cost, align with direct buffer modification style in current code.
con: this 1 time cost is incurred every time write is invoked

I think all ways makes sense, and the performance gain is probably very minimum, if that is true, then I think style match can be important factor.

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.

[Bug Report] RIgidObject failed to invalidate timestamp-sensitive data when direct modifying root_pose and root_link_state
4 participants