Page MenuHome
Differential D1467

Fix for the replace physics mesh bug
AbandonedPublic
Actions

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

Details

Reviewers
Sergej Reich (sergof)
Mitchell Stokes (moguri)
Angus Hollands (agoose77)
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

Event Timeline

Max Voss (max12345) updated this revision to Diff 4866.Aug 16 2015, 1:19 PM
Max Voss (max12345) retitled this revision from to Fix for the replace physics mesh bug.
Max Voss (max12345) updated this object.
Max Voss (max12345) added a project: Game Engine.
Max Voss (max12345) updated this revision to Diff 5020.Sep 6 2015, 10:48 PM

removed some useless comments

Mitchell Stokes (moguri) added reviewers: Game Engine, Game Physics, Angus Hollands (agoose77), Sergej Reich (sergof).Sep 10 2015, 7:27 PM
Mitchell Stokes (moguri) added a project: Game Physics.
Thomas Szepe (hg1) added a subscriber: Thomas Szepe (hg1).Sep 11 2015, 3:41 PM

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;

Max Voss (max12345) added a comment.Oct 6 2015, 3:45 PM

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

Thomas Szepe (hg1) added a comment.Oct 6 2015, 4:19 PM

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) updated this revision to Diff 5570.Dec 15 2015, 8:11 AM
Max Voss (max12345) marked 7 inline comments as done.

I updated the suggested changes (finally).

Sybren A. Stüvel (sybren) added a subscriber: Sybren A. Stüvel (sybren).Dec 15 2015, 9:28 AM

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.Dec 15 2015, 5:09 PM
Max Voss (max12345) updated this object.Dec 15 2015, 5:11 PM
Max Voss (max12345) updated this object.
Max Voss (max12345) updated this object.Dec 15 2015, 5:15 PM
Sybren A. Stüvel (sybren) added a comment.Dec 16 2015, 12:51 PM

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.

Sybren A. Stüvel (sybren) requested changes to this revision.Dec 16 2015, 12:52 PM
Sybren A. Stüvel (sybren) added a reviewer: Sybren A. Stüvel (sybren).
This revision now requires changes to proceed.Dec 16 2015, 12:52 PM
Max Voss (max12345) added a comment.Dec 16 2015, 11:05 PM

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

Max Voss (max12345) added a comment.Dec 21 2015, 12:05 PM

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?

Sybren A. Stüvel (sybren) added a comment.Dec 21 2015, 12:28 PM
In D1467#37101, @Max Voss (max12345) wrote:

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
Max Voss (max12345) edited edge metadata.

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

Max Voss (max12345) updated this revision to Diff 5605.Dec 21 2015, 1:11 PM
Max Voss (max12345) edited edge metadata.

The already previously made changes applied again.

Max Voss (max12345) updated this revision to Diff 5607.Dec 21 2015, 1:58 PM

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?

Mitchell Stokes (moguri) added a reviewer: Mitchell Stokes (moguri).Aug 16 2016, 5:14 PM
Mitchell Stokes (moguri) edited edge metadata.Aug 22 2016, 4:34 AM

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.

Max Voss (max12345) added a comment.Aug 22 2016, 5:13 PM

meshcreatetest.blend561 KBDownload

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

Sybren A. Stüvel (sybren) resigned from this revision.Sep 2 2016, 10:56 AM
Sybren A. Stüvel (sybren) removed a reviewer: Sybren A. Stüvel (sybren).

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?

Max Voss (max12345) added inline comments.Feb 13 2017, 10:24 AM
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.

Sybren A. Stüvel (sybren) removed reviewers: Game Physics, Game Engine.Apr 7 2017, 11:58 PM
Aaron Carlisle (Blendify) abandoned this revision.Dec 29 2019, 2:27 AM
Aaron Carlisle (Blendify) added a subscriber: Aaron Carlisle (Blendify).

BGE was removed in Blender 2.80 this patch is being closed as a result.

Revision Contents
Changeset List

PathSize
doc/
python_api/
rst/
bge_types/
4 lines
source/
gameengine/
Ketsji/
11 lines
2 lines
Physics/
Bullet/
12 lines
27 lines
common/
2 lines

Diff 5020

View Options

doc/python_api/rst/bge_types/bge.types.KX_GameObject.rst

Loading...
View Options

source/gameengine/Ketsji/KX_GameObject.cpp

Loading...
View Options

source/gameengine/Ketsji/KX_Scene.cpp

Loading...
View Options

source/gameengine/Physics/Bullet/CcdPhysicsController.h

Loading...
View Options

source/gameengine/Physics/Bullet/CcdPhysicsController.cpp

Loading...
View Options

source/gameengine/Physics/common/PHY_IPhysicsController.h

Loading...