Merge pull request #503 from StanfordVL/remove-names

Make every prim remove their owned prims, avoid blind recursion in name removal

omnigibson/prims/entity_prim.py

@@ -160,6 +160,20 @@ def _post_load(self):
 
         self._materials = materials
 
+    def remove(self):
+        # First remove all joints
+        if self._joints is not None:
+            for joint in self._joints.values():
+                joint.remove()
+
+        # Then links
+        if self._links is not None:
+            for link in self._links.values():
+                link.remove()
+
+        # Finally, remove this prim
+        super().remove()
+
     def update_links(self):
         """
         Helper function to refresh owned joints. Useful for synchronizing internal data if

omnigibson/prims/prim_base.py

@@ -127,7 +127,7 @@ def remove(self):
             delete_prim(self.prim_path)
 
         # Also clear the name so we can reuse this later
-        self.remove_names(include_all_owned=True)
+        self.remove_names()
 
     @abstractmethod
     def _load(self):

omnigibson/prims/rigid_prim.py

@@ -144,6 +144,20 @@ def _initialize(self):
         self.update_handles()
         self._body_name = self.prim_path.split("/")[-1]
 
+    def remove(self):
+        # First remove the meshes
+        if self._collision_meshes is not None:
+            for collision_mesh in self._collision_meshes.values():
+                collision_mesh.remove()
+
+        # Make sure to clean up all pre-existing names for all visual_meshes
+        if self._visual_meshes is not None:
+            for visual_mesh in self._visual_meshes.values():
+                visual_mesh.remove()
+
+        # Then self
+        super().remove()
+
     def update_meshes(self):
         """
         Helper function to refresh owned visual and collision meshes. Useful for synchronizing internal data if

omnigibson/prims/xform_prim.py

@@ -83,6 +83,14 @@ def _post_load(self):
         if "scale" in self._load_config and self._load_config["scale"] is not None:
             self.scale = self._load_config["scale"]
 
+    def remove(self):
+        # Remove the material prim if one exists
+        if self._material is not None:
+            self._material.remove()
+
+        # Remove the prim
+        super().remove()
+
     def _set_xform_properties(self):
         current_position, current_orientation = self.get_position_orientation()
         properties_to_remove = [

omnigibson/utils/python_utils.py

@@ -354,11 +354,7 @@ def __init__(self, *args, **kwargs):
             f"UniquelyNamed object with name {self.name} already exists!"
         NAMES.add(self.name)
 
-    # def __del__(self):
-    #     # Remove this object name from the registry if it's still there
-    #     self.remove_names(include_all_owned=True)
-
-    def remove_names(self, include_all_owned=True, skip_ids=None):
+    def remove_names(self):
         """
         Checks if self.name exists in the global NAMES registry, and deletes it if so. Possibly also iterates through
         all owned member variables and checks for their corresponding names if @include_all_owned is True.
@@ -370,48 +366,9 @@ def remove_names(self, include_all_owned=True, skip_ids=None):
             skip_ids (None or set of int): If specified, will skip over any ids in the specified set that are matched
                 to any attributes found (this compares id(attr) to @skip_ids).
         """
-        # Make sure skip_ids is a set so we can pass this into the method, and add the dictionary so we don't
-        # get infinite recursive loops
-        skip_ids = set() if skip_ids is None else skip_ids
-        skip_ids.add(id(self))
-
         # Check for this name, possibly remove it if it exists
         if self.name in NAMES:
             NAMES.remove(self.name)
-
-        # Also possibly iterate through all owned members and check if those are instances of UniquelyNamed
-        if include_all_owned:
-            self._remove_names_recursively_from_dict(dic=self.__dict__, skip_ids=skip_ids)
-
-    def _remove_names_recursively_from_dict(self, dic, skip_ids=None):
-        """
-        Checks if self.name exists in the global NAMES registry, and deletes it if so
-
-        Args:
-            skip_ids (None or set): If specified, will skip over any objects in the specified set that are matched
-                to any attributes found.
-        """
-        # Make sure skip_ids is a set so we can pass this into the method, and add the dictionary so we don't
-        # get infinite recursive loops
-        skip_ids = set() if skip_ids is None else skip_ids
-        skip_ids.add(id(dic))
-
-        # Loop through all values in the inputted dictionary, and check if any of the values are UniquelyNamed
-        for name, val in dic.items():
-            if id(val) not in skip_ids:
-                # No need to explicitly add val to skip objects because the methods below handle adding it
-                if isinstance(val, UniquelyNamed):
-                    val.remove_names(include_all_owned=True, skip_ids=skip_ids)
-                elif isinstance(val, dict):
-                    # Recursively iterate
-                    self._remove_names_recursively_from_dict(dic=val, skip_ids=skip_ids)
-                elif hasattr(val, "__dict__"):
-                    # Add the attribute and recursively iterate
-                    skip_ids.add(id(val))
-                    self._remove_names_recursively_from_dict(dic=val.__dict__, skip_ids=skip_ids)
-                else:
-                    # Otherwise we just add the value to skip_ids so we don't check it again
-                    skip_ids.add(id(val))
 
     @property
     def name(self):

tests/test_object_removal.py

@@ -0,0 +1,86 @@
+from omnigibson.objects import DatasetObject
+import omnigibson as og
+from omnigibson.utils.python_utils import NAMES
+
+from utils import og_test
+
+import pytest
+
+
+@og_test
+def test_removal_and_readdition():
+    # Make a copy of NAMES
+    initial_names = NAMES.copy()
+
+    # Add an apple
+    apple = DatasetObject(
+        name="apple",
+        category="apple",
+        model="agveuv",
+    )
+
+    # Import it into the scene
+    og.sim.import_object(apple)
+
+    # Check that NAMES has changed
+    assert NAMES != initial_names
+
+    # Step a few times
+    for _ in range(5):
+        og.sim.step()
+
+    # Remove the apple
+    og.sim.remove_object(obj=apple)
+
+    # Check that NAMES is the same as before
+    extra_names = NAMES - initial_names
+    assert len(extra_names) == 0, f"Extra names: {extra_names}"
+
+    # Importing should work now
+    apple2 = DatasetObject(
+        name="apple",
+        category="apple",
+        model="agveuv",
+    )
+    og.sim.import_object(apple2)
+
+    # Clear the stuff we added
+    og.sim.remove_object(apple2)
+
+@og_test
+def test_readdition():
+    # Make a copy of NAMES
+    initial_names = NAMES.copy()
+
+    # Add an apple
+    apple = DatasetObject(
+        name="apple",
+        category="apple",
+        model="agveuv",
+    )
+
+    # Import it into the scene
+    og.sim.import_object(apple)
+
+    # Check that NAMES has changed
+    new_names = NAMES.copy()
+    assert new_names != initial_names
+
+    # Step a few times
+    for _ in range(5):
+        og.sim.step()
+
+    # Creating and importing a new apple should fail
+    with pytest.raises(AssertionError):
+        apple2 = DatasetObject(
+            name="apple",
+            category="apple",
+            model="agveuv",
+        )
+        og.sim.import_object(apple2)
+
+    # Check that NAMES has not changed
+    assert NAMES == new_names
+
+    # Clear the stuff we added
+    og.sim.remove_object(apple)