-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Moves meshes in the RayCaster
to classvar to be shared between sensors
#2183
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?
Moves meshes in the RayCaster
to classvar to be shared between sensors
#2183
Conversation
Co-authored-by: Mayank Mittal <[email protected]> Signed-off-by: Pascal Roth <[email protected]>
…oth/IsaacLab into fearture/mesh-storing-raycaster
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.
Pull Request Overview
This PR moves the mesh storage for raycasting from instance attributes to class-level variables to allow sharing between sensors and reduce memory usage.
- Moves warp mesh and mesh view dictionaries to class-level with type annotations.
- Introduces an instance counter and a del method to clear the shared dictionaries when no instances remain.
- Updates the typing import to include ClassVar.
Comments suppressed due to low confidence (1)
source/isaaclab/isaaclab/sensors/ray_caster/ray_caster.py:314
- [nitpick] Relying on del for cleanup of shared class variables can be unpredictable due to Python's garbage collection behavior. Consider using an explicit cleanup method or context management to ensure reliable resource release.
def __del__(self):
@pascal-roth Pre-commit is failing. Can you check? |
lets wait with a merge, just making some tests for the ray_caster to make sure that the new classvar is cleared correclty |
@Mayankm96 ready for a review, now the raycaster also has a first set of basic tests. IMPORTANT: the test already run with pytest, commented out part at the top is for the time when everything is upgraded to pytest |
Looks like some tests are failing, is that expected? |
so regarding the tests:
Lets really push for the pytest conversion, this will make the whole setup much easier |
Is the test_environment_determinism.py failure related to the changes? |
Not sure, will have to check today |
So, the tests are passing locally! When investigating the log, then it complains about reaching a recusion depth during the raycasting.
I don't really know what might cause the issue as now every script is run on its own. Any idea @kellyguo11 or @Mayankm96 ? |
hmm very strange! I can't repro it locally either, even inside the docker container. but CI seems to be failing quite consistently on the Anymal-C rough environment, which also feels somehow related to the ray caster changes... |
I tend to agree, we can put some debug statements all over the code and see if we find something haha |
@@ -157,6 +168,10 @@ def _initialize_warp_meshes(self): | |||
|
|||
# read prims to ray-cast | |||
for mesh_prim_path in self.cfg.mesh_prim_paths: | |||
# check if mesh already casted into warp mesh |
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.
Shouldn't
self.meshes[mesh_prim_path] = wp_mesh
Be:
RayCaster.meshes[mesh_prim_path] = wp_mesh
I thought all write operations need to happen through class for classvar
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.
The ClassVar
is only for static type-checking. It isn't enforced in any form by the interpreter.
@@ -288,3 +303,8 @@ def _invalidate_initialize_callback(self, event): | |||
super()._invalidate_initialize_callback(event) | |||
# set all existing views to None to invalidate them | |||
self._view = None | |||
|
|||
def __del__(self): |
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.
Would move the del after init to keep constructor and destructor near each other.
Description
Moves meshes in the
RayCaster
to classvar to be shared between sensors. This should save memory.Type of change
Checklist
pre-commit
checks with./isaaclab.sh --format
config/extension.toml
fileCONTRIBUTORS.md
or my name already exists there