Do I need to cleanup the old shape when using SetShape() on a body?

[DennisSmolek]

I'm still not 100% on ref based objects so not sure if I need to cleanup certain things.

Right now I do:

const newShape = newShapeSettings.Create().Get();
body.SetShape(newShape);

but I could do:

const existingShape = body.GetShape();
body.SetShape(newShape);
jolt.destroy(existingShape);

I ask this because when I set vectors I have to destroy the vector:

//leaves vector hanging in memory
const scaledShape = baseShape.ScaleShape(new jolt.Vec3(1,2,3)).Get();

// with cleanup
const scaleVec = new jolt.Vec3(4,5,6);
const otherScaledShape = baseShape.ScaleShape(scaleVec).Get();
jolt.destroy(scaleVec)

Right now it's obvious to destroy the vector because I created it a line above it.
And as a JS dev you might think the non-ref'd old shape would be garbage collected

However, it's likely I also created that shape too in an earlier call to SetShape()

So I'm just not sure if I need to destroy it or not....

[PhoenixIllusion]

You would not call destroy. This is automatically handled inside Jolt due to it being a special Reference class, assuming you manage shape-creation and shape-settings correctly. If that Shape was not used elsewhere by another Reference, by the time you run jolt.destroy Jolt already destroyed it during SetShape and you are double-free-ing the memory.

const existingShape = body.GetShape();
body.SetShape(newShape);
 //do not call this jolt.destroy(existingShape);

However, it's likely I also created that shape too in an earlier call to SetShape()

SetShape does not create a new shape, it just assigns the pointer newShape to the body. This allows you to make a single shape, such as a giant Mesh, and assign it to many different bodies without duplicating the internal memory.

Example Flow

Assuming [Body] and the 2x shapes, A & B.
When created, they both have a reference of 0.

• A - 0
• B - 0

When you either create [Body] or assign [Body] shape A, shape A gains an AddRef internally:

• A - AddRef - 1
• B - 0

When you call "SetShape B", it will internally trigger:

• A - Release - 0 - "Destroy"
• B - AddRef - 1

No Shape related memory management is needed by your JS code in this flow

I had to dig down to see where this Add/Release happened, and it is kind of concealed, but this is a side-effect on the line :
https://github.com/jrouwe/JoltPhysics/blob/f4f8e61f4bde526f74ab334ecacf6e319c575e85/Jolt/Physics/Body/Body.cpp#L136

The logic is "Internally, if I am assigning a new internal Shape pointer to an existing ConstReference, Release the current reference and AddRef the new one"
https://github.com/jrouwe/JoltPhysics/blob/f4f8e61f4bde526f74ab334ecacf6e319c575e85/Jolt/Core/Reference.h#L163

Details

Jolts internal management will assume ownership and automatic deletion as soon as you assign it to at least one Body for usage. When the last Reference that points to that Shape is destroyed, that particular Shape will automatically be destroyed. Besides just when Bodies are created and destroyed, this also counts for assigning via SetShape.

jolt.destroy(existingShape);

Regarding your second block, it is best practice to avoid jolt.destroy(shape). If some Body is currently using it, it will crash Jolt. In best-practice, the only memory management you would need for a Shape is AddRef and Release. AddRef would be used if you plan on taking ownership of the Shape, such as creating during startup and storing it away to use later even after all Bodies that use it are destroyed (like a Factory, map-builder, or object-spawning JS class). Release would then be called 1x time after you have assigned it to at least 1x Body and want Jolt to manage it again, or when cleaning up the entire app.

Assuming you are not tracking via AddRef calls elsewhere, and are taking care of destroying your ShapeSettings that hold a pointer to it, you should never need to Release the shape yourself.

[DennisSmolek]

Got it!
Thank you for the super detailed reply, it totally helped me understand now.

[LeXXik]

Great answer!

As an additional example, I'm using AddRef() and Release() for the character controller shapes. Usually there are multiple ones(e.g. standing, sitting, crawling, etc) and require creating those without a body, which would otherwise take an ownership.

[PhoenixIllusion]

I tried to do a memory profile test on it. After settings 6 or 7 different shapes on a body and relying on internal memory AddRef, it still had a stray extra 16 bytes due to the PhysicsSystem BodyManager ContactCache being initialized on the first SetShape, which remains up until you destroy JoltInterface at the end, at which point it goes back down to 0-extra bytes allocated.

Example demoing how just calling SetShape frees the prior shape, via a loop of SetShape and using our memory-tracking sGetFreeMemory. To "hide" the extra bytes of the contact cache, I manually called InvalidateContactCache before the loop, so it doesn't appear in the demo's allocation.

import initJolt from './js/jolt-physics.wasm-compat.js';


initJolt().then(function (Jolt) {
  const mem = () => Jolt.JoltInterface.prototype.sGetFreeMemory();

  const beforeInit = mem(); // memory before our physics system allocated
  window.Jolt = Jolt;
  initPhysics();

  const initialMem = mem(); // post physics system init

  const sphereSettingsA = new Jolt.SphereShapeSettings(1)
  const shapeA = sphereSettingsA.Create().Get();

  const bodyPosition = new Jolt.RVec3(1, 2, 3);
  const bodyRotation = new Jolt.Quat(0, 0, 0, 1);
  const creationSettings = new Jolt.BodyCreationSettings(shapeA, bodyPosition, bodyRotation, Jolt.EMotionType_Dynamic, 1); // 'creationSettings' now holds a reference to 'shape'
  Jolt.destroy(bodyPosition);
  Jolt.destroy(bodyRotation);
  Jolt.destroy(sphereSettingsA);
  const body = bodyInterface.CreateBody(creationSettings);
  bodyInterface.AddBody(body.GetID(), Jolt.EActivation_DontActivate);
  Jolt.destroy(creationSettings);
  bodyInterface.InvalidateContactCache(body.GetID());

  const startMem = mem();
  console.log("Initial Mem", initialMem - startMem);
  ['B','C','D','E','F','G'].forEach(key => {
    const sphereSettings = new Jolt.SphereShapeSettings(1)
    const shapeRes = sphereSettings.Create();
    const shape = shapeRes.Get();
    const withSphere = mem();
    console.log("With Sphere "+key, startMem - withSphere);

    bodyInterface.SetShape(body.GetID(), shape, false, Jolt.EActivation_Activate);
    Jolt.destroy(sphereSettings);
    shapeRes.Clear(); // frees a global cache of 40bytes
    const afterSphere = mem();

    console.log("After SetShape Sphere "+key, startMem - afterSphere);
  });

  bodyInterface.RemoveBody(body.GetID());
  bodyInterface.DestroyBody(body.GetID());

  const afterDestroy = mem();
  console.log("After Body Destroy ", initialMem - afterDestroy);

  Jolt.destroy(jolt);
  const destroyed = mem();
  console.log("After Jolt Destroy ", beforeInit - destroyed);

});

Byte Log

Initial Mem 376 (360 body + 16 contact-cache)
With Sphere B 96
After SetShape Sphere B 0
With Sphere C 96
After SetShape Sphere C 0
With Sphere D 96
After SetShape Sphere D 0
With Sphere E 96
After SetShape Sphere E 0
With Sphere F 96
After SetShape Sphere F 0
With Sphere G 96
After SetShape Sphere G 0
After Body Destroy 16 (contact-cache)
After Jolt Destroy 0

Note: to truly get back down to 0 bytes, I had to add something to the Jolt JS's example.JS. setupCollisionFilteringhas the B_LAYERSs passed by-value, not by-ref, so MapObjectToBroadPhase is making a copy and we are left owning the originals and can clean them up:

	Jolt.destroy(BP_LAYER_MOVING);
	Jolt.destroy(BP_LAYER_NON_MOVING);

[jrouwe]

I don't think InvalidateContactCache should have any effect. All it does is set a bit on the body and add its body ID to a list. The contact cache itself doesn't keep a reference to shapes. In fact, the contact cache doesn't even get populated until the PhysicsSystem.Update call (which you're not calling).

[PhoenixIllusion]

No real-life effect. It's just triggering the BodyManager's initial allocation of the array when it pushes the body during mBodiesCacheInvalid.push_back(ioBody.GetID()); during BodyInterface::SetShape, which resizes the mBodiesCacheInvalid vector to support a list-size of 1x until the BodyManager gets disposed, so as a side effect alloc'd the 16 bytes. In real-life scenarios, that array will likely take up more and will get allocated during actual collision events.

I was only mentioning it because when showing the memory-trace there was a stray 16 bytes until the Jolt Destroy, and was noting what it was and that it wasn't related to any SetShape or leaks of the SetShape process and Reference tracking.