Skip to content

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

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

pascal-roth
Copy link
Collaborator

@pascal-roth pascal-roth commented Mar 28, 2025

Description

Moves meshes in the RayCaster to classvar to be shared between sensors. This should save memory.

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

@pascal-roth pascal-roth requested a review from Mayankm96 March 28, 2025 15:34
@pascal-roth pascal-roth self-assigned this Mar 28, 2025
@pascal-roth pascal-roth requested a review from Mayankm96 March 31, 2025 16:37
@Mayankm96 Mayankm96 requested a review from Copilot April 2, 2025 10:07
Copy link

@Copilot Copilot AI left a 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):

@Mayankm96
Copy link
Contributor

@pascal-roth Pre-commit is failing. Can you check?

@pascal-roth
Copy link
Collaborator Author

lets wait with a merge, just making some tests for the ray_caster to make sure that the new classvar is cleared correclty

@pascal-roth
Copy link
Collaborator Author

@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

@kellyguo11
Copy link
Contributor

Looks like some tests are failing, is that expected?

@pascal-roth
Copy link
Collaborator Author

so regarding the tests:

  • test_ray_caster.py: fail is expected as written in pytest (not sure why our unittest framework even catches that one)
  • test_tiled_camera.py and test_contact_sensor.py fail but are not even in the log, don't think that is from this PR
  • test_environment.py times out again

Lets really push for the pytest conversion, this will make the whole setup much easier

@kellyguo11
Copy link
Contributor

Is the test_environment_determinism.py failure related to the changes?

@pascal-roth
Copy link
Collaborator Author

Not sure, will have to check today

@pascal-roth
Copy link
Collaborator Author

So, the tests are passing locally!

When investigating the log, then it complains about reaching a recusion depth during the raycasting.

source/isaaclab/isaaclab/utils/configclass.py:282: in _validate
    missing_fields.extend(_validate(value, prefix=current_path))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

obj = '/isaac-sim/extscache/omni.warp.core-1.5.0+lx64/warp/bin/warp.so'
prefix = 'height_scanner.class_type.meshes./World/ground.device.runtime.device_map.cpu.runtime.device_map.cpu.runtime.device_ma...map.cpu.runtime.device_map.cpu.runtime.device_map.cpu.runtime.device_map.cpu.runtime.device_map.cpu.runtime.core._name'

    def _validate(obj: object, prefix: str = "") -> list[str]:
        """Check the validity of configclass object.
    
        This function checks if the object is a valid configclass object. A valid configclass object contains no MISSING
        entries.
    
        Args:
            obj: The object to check.
            prefix: The prefix to add to the missing fields. Defaults to ''.
    
        Returns:
            A list of missing fields.
    
        Raises:
            TypeError: When the object is not a valid configuration object.
        """
        missing_fields = []
    
        if type(obj) is type(MISSING):
            missing_fields.append(prefix)
            return missing_fields
>       elif isinstance(obj, (list, tuple)):
E       RecursionError: maximum recursion depth exceeded in __instancecheck__

I don't really know what might cause the issue as now every script is run on its own. Any idea @kellyguo11 or @Mayankm96 ?

@kellyguo11
Copy link
Contributor

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

@pascal-roth
Copy link
Collaborator Author

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
Copy link
Contributor

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

Copy link
Contributor

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):
Copy link
Contributor

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.

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.

3 participants