PhysicsSpace.delete() does not remove physicsNodes

I've just noticed that calling PhysicsSpace.delete() fails to clear physics nodes from the scene, because they are not automatically set to inactive.



Looking at the OdePhysicsSpace implementation, delete calls removeAllObjects() which calls removeNode() on each node. However, unlike removeJoint(), the OdePhysicsSpace implementation of removeNode() doesn't call super.removeNode(), which would in turn call setActive(false) on the node.

Adding that line would cause delete() to work as (at least I) expected it to.



In the meantime, I'm just manually setting each physics node to inactive before I delete the physics space.

You need to remove the physics nodes from the scene anyway, to allow the garbage collector to do it's job. Just calling PhysicsSpace.delete() is not enough. You need to clear all references to the space, the nodes and joints to really delete a physics space.



Still OdePhysicsSpace.removeNode() should call super.removeNode(). Please check if adding the super call after the if block fixes it for you.

Quickly testing it, this change does cause the physicsSpace.delete() to clear the geometry from the scene. You are right that the physics nodes should also be removed from the scene, but that seems (to me) to be something that physicsSpace.delete() should do.

Looking at OdePhysicsSpace.removeAllObjects(), shouldn't it call delete on each physics node, recursively, instead of just calling removeNode()?

That would cause the physics nodes to remove themselves from the scene, and then when they call setActive(false) on themselves, they will in turn call removeNode() anyways.



I tried both of these changes, and they don't immediately cause any problems - but I could see how these changes have the potential to cause problems for anyone depending on the present behavior of physicsSpace.delete().

sbayless said:

Quickly testing it, this change does cause the physicsSpace.delete() to clear the geometry from the scene. You are right that the physics nodes should also be removed from the scene, but that seems (to me) to be something that physicsSpace.delete() should do.

I don't agree here. The PhysicsSpace does not add nodes to the scenegraph when creating them, so it should not remove them either.

Ok, that does make sense actually.

Sorry to dig up and old thread, but it seemed appropriate seeing as I found the answer I needed here.



I was wondering why there is no .removeAllNodes() method in PhysicsSpace itself…



I have never released a patch before and wouldn't mind taking this opportunity to learn.  If a patch is deemed appropriate that is.

I don't think that this convenience method is really needed. It would be rarely used and only one line (or three if you' use braces) of code would replace it:

for ( PhysicsNode physicsNode : space.getNodes() ) space.removeNode( physicsNode );

Umm, thats a protected method so I can't call it directly :stuck_out_tongue:

uh, I meant

for ( PhysicsNode physicsNode : space.getNodes() ) physicsNode.delete();

I get a ConcurrentModificationException when I do that, however using

( (OdePhysicsSpace) PhysicsSpace() ).removeAllObjects()


works fine....
basixs said:

I get a ConcurrentModificationException when I do that, however using

( (OdePhysicsSpace) PhysicsSpace() ).removeAllObjects()


works fine....


i think this could be solved by executing the call in the GameTaskQueue.

I should have mentioned that it was (I did think about it, but forgot to mention)



here is the method that I am using:


    public void readMapFile( final String fileName ) {

        LoadingGfx.showLoadingGfx( true );
        setActive( false );

        final Area areaReference = this;
        GameTaskQueueManager.getManager().update( new Callable<Object>() {

            public Object call() throws Exception {

                TextureManager.clearCache();
                tileMap.destroy();
                tileMap = null;

                ObjectManager.destroyAllObjects();
                rootNode.detachAllChildren();

//                ( (OdePhysicsSpace) PhysicsControl.getPhysicsSpace() ).removeAllObjects();

                for( PhysicsNode physicsNode : PhysicsControl.getPhysicsSpace().getNodes() ){
                        physicsNode.delete();
                }

                rootNode.attachChild( PhysicsControl.getCollisionNode() );

                System.gc();
                System.gc();



                MapLoader.loadMap( areaReference, fileName );

                TextureManager.preloadCache( DisplaySystem.getDisplaySystem().getRenderer() );

                rootNode.updateRenderState();
                rootNode.updateGeometricState( 0, true );

                LoadingGfx.hideLoadingGfx();
                setActive( true );

                return null;
            }
            } );
    }




AHHHHH!  It's because the list in OdePhysicsSpace.removeAllObjects() is cloned()!!



This works fine, although it requires an extra cast....


                for( Object pObject : PhysicsControl.getPhysicsSpace().getNodes().toArray() ){
                    ( (PhysicsNode) pObject ).delete();
                }