Should DecoratedShapeSettings allow for raw pointers in its constructor?

[jdumas]

Consider the following modified HelloWorld.cpp example:

diff --git i/Source/HelloWorld.cpp w/Source/HelloWorld.cpp
index 72b6b55..eea0ccb 100644
--- i/Source/HelloWorld.cpp
+++ w/Source/HelloWorld.cpp
@@ -15,6 +15,7 @@
 #include <Jolt/Physics/PhysicsSystem.h>
 #include <Jolt/Physics/Collision/Shape/BoxShape.h>
 #include <Jolt/Physics/Collision/Shape/SphereShape.h>
+#include <Jolt/Physics/Collision/Shape/RotatedTranslatedShape.h>
 #include <Jolt/Physics/Body/BodyCreationSettings.h>
 #include <Jolt/Physics/Body/BodyActivationListener.h>

@@ -294,8 +295,11 @@ int main(int argc, char** argv)
 	// Note that for simple shapes (like boxes) you can also directly construct a BoxShape.
 	BoxShapeSettings floor_shape_settings(Vec3(100.0f, 1.0f, 100.0f));

+	// Transform the shape
+	RotatedTranslatedShapeSettings transformed_shape_settings(Vec3::sZero(), Quat::sIdentity(), &floor_shape_settings);
+
 	// Create the shape
-	ShapeSettings::ShapeResult floor_shape_result = floor_shape_settings.Create();
+	ShapeSettings::ShapeResult floor_shape_result = transformed_shape_settings.Create();
 	ShapeRefC floor_shape = floor_shape_result.Get(); // We don't expect an error here, but you can check floor_shape_result for HasError() / GetError()

 	// Create the settings for the body itself. Note that here you can also set other properties like the restitution / friction.

Running the program in Debug mode exits with the following:

HelloWorld(78064,0x1ec29bac0) malloc: *** error for object 0x16b86cb30: pointer being freed was not allocated
HelloWorld(78064,0x1ec29bac0) malloc: *** set a breakpoint in malloc_error_break to debug
fish: Job 1, './HelloWorld' terminated by signal SIGABRT (Abort)

I think the issue comes from the parent DecoratedShapeSettings object that takes a raw pointer as input:

JoltPhysics/Jolt/Physics/Collision/Shape/DecoratedShape.h

Lines 20 to 24 in 4deaf12

	explicit DecoratedShapeSettings(const ShapeSettings *inShape) : mInnerShape(inShape) { }
	explicit DecoratedShapeSettings(const Shape *inShape) : mInnerShapePtr(inShape) { }
	RefConst<ShapeSettings> mInnerShape; ///< Sub shape (either this or mShapePtr needs to be filled up)
	RefConst<Shape> mInnerShapePtr; ///< Sub shape (either this or mShape needs to be filled up)

The function signature suggests that it is safe to pass a raw pointer to a stack-allocated BoxShapeSettings for example. But because the object becomes refcounted, destroying the RotatedTranslatedShapeSettings will attempt to destroy the previously stack-allocated BoxShapeSettings.

I don't know if this design is intentional. I would argue that the DecoratedShapeSettings should take as argument a RefConst<> to prevent this kind of situation. At the very least the HelloWorld.cpp example suggests that it's ok to stack-allocate shape setting but this can lead to this kind of tricky situation. Let me know what are your thoughts on this. Thanks!

[jrouwe]

If you allocate a refcounted object on the stack and take a reference, you need to call object.SetEmbedded() to signal the reference counting system that you're responsible for cleaning it up.

So:

BoxShapeSettings floor_shape_settings(Vec3(100.0f, 1.0f, 100.0f));

should be

BoxShapeSettings floor_shape_settings(Vec3(100.0f, 1.0f, 100.0f));
floor_shape_settings.SetEmbedded();

in this case. In the original HelloWorld example, nothing takes a ref to the BoxShapeSettings object so it's fine in that case to skip the call.

[jdumas]

I see, thanks for the tip! It might be worth mentioning this use-case in the HelloWorld example maybe? I feel it's an easy mistake to make since the HelloWorld example make it look like it's ok to have those objects be stack-allocated without doing anything special.

Closing this issue since you've answer the initial question.

[jrouwe]

Done.