Fix for the replace physics mesh bug
Needs ReviewPublic

Authored by Max Voss (max12345) on Aug 16 2015, 1:19 PM.

Details

Summary

Creates a new physics shape instance when the mesh is replaced, so that all other linked objects remain unaffected.

The bug in question is this one:

https://developer.blender.org/T40955

You can find some example files there.

Detailed description of the bug from the link.

"The use of replace mesh , do something with mesh , reinstance physics mesh doesn't work because replace mesh switches out the graphics mesh without regard whether it was shared or inherited or not. But the physics mesh gets inherited from whatever mesh the shape was inherited from when the object was created. Which is also why this new demofile https://developer.blender.org/F110777 demonstrates another bug, this time with the replace mesh actuator:

Creating an object and then a linked duplicate, giving the original but not the linked duplicate a replace mesh actuator and using it with a cylinder will switch out shapes for the original object but also because of it's linked nature the physics shape for the linked duplicate - but not the duplcates graphics mesh.

So really when anything gets replaced, things break. Either the graphics mesh gets replaced and the physics mesh doesn't or both do and then the physics meshes on all relative objects break."

me in that linked bug report

You can use the linked demofile in a fairly straightforward way, there are 3 objects with the described setup and an always sensor executes the mesh replacement. You can then see how the physics mesh gets switched out when it shouldn't be. Or the graphics mesh when it shouldn't be etc. Since the behavior of all objects spawned with scene.addObject are linked copies, this behavior applies to all objects that are spawned while the game runs. Changing one changes all of them.

This patch "fixes" that by introducing a flag which if enabled creates a new instance for the physics mesh. This physics mesh can then be replaced or written to without affecting the original mesh or it's linked copies.

This is good because this way I can create mesh shapes while the engine runs, instead of just spawning previously created mesh shapes.

Diff Detail

Max Voss (max12345) retitled this revision from to Fix for the replace physics mesh bug.Aug 16 2015, 1:19 PM
Max Voss (max12345) updated this object.

removed some useless comments

I don't tested the code, I only added some code style notes.

source/gameengine/Ketsji/KX_GameObject.cpp
2071–2074

Correcting spaces.

if (!PyArg_ParseTuple(args, "|OOi:reinstancePhysicsMesh", &gameobj_py, &mesh_py, &dupli) || 
    (gameobj_py && !ConvertPythonToGameObject(gameobj_py, &gameobj, true, "gameOb.reinstancePhysicsMesh(obj, mesh, dupli) : KX_GameObject")) ||
    (mesh_py && !ConvertPythonToMesh(mesh_py, &mesh, true, "gameOb.reinstancePhysicsMesh(obj, mesh, dupli ): KX_GameObject")))
{
source/gameengine/Ketsji/KX_Scene.cpp
1291

Spaces
gameobj->GetPhysicsController()->ReinstancePhysicsShape(NULL, use_gfx ? NULL : mesh, true);

source/gameengine/Physics/Bullet/CcdPhysicsController.cpp
1712

Space
bool CcdPhysicsController::ReinstancePhysicsShape(KX_GameObject *from_gameobj, RAS_MeshObject *from_meshobj, bool dupli)

1715

No empty spaces after semi colon.
Old line 1715 and 1716 are correct. So leave it unchanged.

2483

Delete this new empty line.

source/gameengine/Physics/Bullet/CcdPhysicsController.h
798

Asterisk
virtual bool ReinstancePhysicsShape(KX_GameObject *from_gameobj, RAS_MeshObjec *from_meshobj, bool dupli = false);

source/gameengine/Physics/common/PHY_IPhysicsController.h
144

Asterisk
virtual bool ReinstancePhysicsShape(KX_GameObject *from_gameobj, RAS_MeshObject *from_meshobj, bool dupli = false) = 0;

Can I just hit "done" to accept the suggested change with spaces?

Can I just hit "done" to accept the suggested change with spaces?

To accept the changes you need to select "done" and then you need press "Submit". But this is only to inform us that you want to do or have done this changes.
It don't update the patch.
To change the patch you still need to upload an updated diff.

Max Voss (max12345) marked 7 inline comments as done.

I updated the suggested changes (finally).

Please update the summary to give us some more information. Right now, you don't describe any problem with the existing code. You just explain what the new code does, but not why.

Furthermore, please include an example file, including an explanation on how to use it to see the improvement(s) made by this patch.

Sell your patch! Make it easy for us to understand why those changes are necessary. Make us want it!

Max Voss (max12345) updated this object.

Once I fixed the compiler error, the patch seems to work. There are a few things missing, though: clear documentation. I know that the Blender code isn't the end-all-be-all of well-documented code, but IMO we should at least properly document new stuff.

source/gameengine/Physics/Bullet/CcdPhysicsController.cpp
1715

Can you remove this line from the patch? It doesn't seem to change anything.

1719

The fact that it only duplicates the shape when it is shared is not documented in the reinstancePhysicsMesh docstring.

1848

Apparently, the copy constructor creates a shallow copy; the copy still shares data with the original. This should be documented in its declaration!

source/gameengine/Physics/Bullet/CcdPhysicsController.h
798

This doesn't compile. RAS_MeshObjec doesn't exist, it needs an extra t.
Furthermore, some documentation comment would be nice.

source/gameengine/Physics/common/PHY_IPhysicsController.h
144

This method should be documented.

This revision now requires changes to proceed.Dec 16 2015, 12:52 PM

Fair point I'll get to it on the weekend.

Ok so my local git diff shows something different than the diff I have here, not really related changes I think, it's just a lot longer. What can I do about that? I mean if it's unrelated you'll probably not want it in the patch.

But updating my build and giving you the up to date diff might break my build and I need that.

source/gameengine/Physics/common/PHY_IPhysicsController.h
144

Where? As a comment or is there some other place where that would go?

Ok so my local git diff shows something different than the diff I have here, not really related changes I think, it's just a lot longer. What can I do about that? I mean if it's unrelated you'll probably not want it in the patch.

Make it two (or more) commits, then send in just one commit as patch. That way you can test and recompile just one change, without affecting the others.

source/gameengine/Physics/common/PHY_IPhysicsController.h
144

Yes, as a doxygen-compatible comment.

Max Voss (max12345) updated this revision to Diff 5604.EditedDec 21 2015, 1:07 PM

I updated everything so the diff is relative to master so hopefully changing the diff is easier now.

The already previously made changes applied again.

Added some comments documenting the additions.

Max Voss (max12345) marked an inline comment as done.Apr 9 2016, 1:06 PM

Can I get an update on what's still missing?

Overall, this patch looks good to me. I like that it extends the API, which will help keep compatibility (as opposed to making changes deep in Physics code). As long as CcdShapeConstructionInfo's copy constructor was not getting called before, I do not see how this can break existing games.

@Max Voss (max12345), what kind of testing have you done? Do you have some test files demonstrating the usage of this new API?

@Sybren A. Stüvel (sybren), Do you still have issues with this patch? I'm not too concerned about this patch adding comments to already existing functions.

This is a simple file showing the creation of a new mesh and the proper replacement of both the graphics and physics mesh.

Apparently I had an unsubmitted comment still laying around. I haven't done anything with the GE since the start of the year, so I trust @Mitchell Stokes (moguri)'s opinion of this patch.

source/gameengine/Physics/Bullet/CcdPhysicsController.h
808

What does "the bool specifies whether the the physics information of the game object should be duplicated or not to either create a new physics mesh or write to the old one, changing it and that way all the physics meshes of instances or linked copies of the game object" mean? Maybe split this up in more sentences?

source/gameengine/Physics/Bullet/CcdPhysicsController.h
808

I haven't touched this in a while so I'll assume you haven't either. I don't care what the comment reads in detail.

The root of this problem is that it's an unclear behavior around pointers and physics data.

The bge is setup to spawn new objects, those are "linked" objects in terms of blender and the bge, and what happens on the code level is that their data is actually the data of an "Inactive object" that's pointed to.

The problem that comes up is that when you want to change *one* object, the function that changes stuff can only change the data that's pointed to. So by changing that data, all objects with the same class/inactive object get changed.

BUT this behavior can be useful and sometimes intended.

So what this bool does is switch between whether you want the information to be duplicated and then changed, which leaves the original data intact and the result is two objects that no longer share the same data or if you want the data to not be duplicated and be changed, which results in the two objects still sharing the same data.

So the bool allows you to pick which behavior you want, modify all data or just the data of one object.