Details

Reviewers

Sybren A. Stüvel (sybren)
Jorge Bernal (lordloki)
Sergej Reich (sergof)
Mitchell Stokes (moguri)

Maniphest Tasks

T18831: DupliGroups Break Physics Setups

Commits

rBS0b4a71b07245: BGE: Add physics constraints replication
rB0b4a71b07245: BGE: Add physics constraints replication

Summary

This patch will add a physics constraints replication for group instances.

Initial patch T31443 from moerdn (Martin Sell).

Diff Detail

Repository: rB Blender

Event Timeline

This patch is only a update of my old group-constraints8_HG1.patch.
I also removed the ConstraintWrapper Documentation to D654.

Should I duplicate the source of the setup_constraint4object() to KX_Scene? This will allow us to reduce the amount of new includes.

In general the constraint handling seems overly complicated, but I guess this might be a simpler fix than over-hauling the constraints system. It's kind of silly how much constraint data we have to duplicate. For example, it would be nice if we could re-setup constraints based on Blender data, but since constraints can be changed at runtime (and we try to avoid modifying Blender data) we have to track constraints ourselves.

source/gameengine/Converter/BL_BlenderDataConversion.h
41 ↗	(On Diff #2130)	These includes can be moved to BL_BlenderDataConversion.cpp if you forward declare KX_GameObject and PHY_IPhysicsController.
45 ↗	(On Diff #2130)	If this is going to be public, it should have a better name such as: BL_SetupObjectConstraints().
source/gameengine/Ketsji/KX_GameObject.h
52	bRigidBodyJointConstraint can be forward declared to push this include into KX_GameObject.cpp.
77	I don't know if I like this in the KX_GameObject. Shouldn't constraint handling be in PHY_IPhysicsController?
80	Why bother storing the physics environment? Shouldn't it be simple enough to retrieve again?
source/gameengine/Ketsji/KX_PyConstraintBinding.cpp
480 ↗	(On Diff #2130)	Is this for this patch or another patch? It doesn't seem related. Maybe I'm just missing some context.
563 ↗	(On Diff #2130)	I'm currently not to fond of the extra constraints data code being added to KX_GameObject to handle this. Maybe there is a better way to do this? For example, can we get a list of KX_ConstraintWrappers from the KX_GameObject's PHY_IPhysicsController? This would probably work in a similar fashion as getting vehicle constraint wrappers.
source/gameengine/Ketsji/KX_Scene.cpp
703	Should the scene be handling this? Seems like something we should delegate to the physics controller.
730	Although, getting access to BL_BlenderDataConversion::setup_constraint4object() might be difficult from the physics controller (in relation to the comment on line 703). Perhaps this function should be moved from BL_BlenderDataConversion into the physics environment?
1058	Shouldn't this be handled in NewRemoveObject? I would think we could get a crash if something tries to reference constraints before the rest of the object is deleted at the end of the frame.
source/gameengine/Ketsji/KX_Scene.h
337 ↗	(On Diff #2130)	This also looks like it's from another patch.
541 ↗	(On Diff #2130)	I'm not really sure if we need this as a public function of KX_Scene. Maybe it could be rolled into KX_Scene::NewRemoveObject()?

Thomas Szepe (hg1) added inline comments.Jul 23 2014, 9:15 PM

source/gameengine/Ketsji/KX_GameObject.h
80	This is necessary to avoid blender from crashing, if getConstraints() is called. When the same pointer is use for the constraint and the constraint wrapper and the object was deleted Blender was crashing. Blender was also crashing if the the constraint delete method was in NewRemoveObject. Python seems still keep a reference on the object. I also got a wrong constraint ID from the generated constraint wrapper. I don't know but I think there is a general bug in the way how the constraint will be generated. Because there some other wired bugs in there.
source/gameengine/Ketsji/KX_PyConstraintBinding.cpp
480 ↗	(On Diff #2130)	No. I deliberately put this in this patch. I thought it is not necessary to make an extra patch for this.
563 ↗	(On Diff #2130)	This will cause a crash. (Edit on line 80)
source/gameengine/Ketsji/KX_Scene.cpp
1058	In a earlier version had done this. But this was producing some kind of bug. I can't remember exactly, but I think it was the bug that not all constraints were deleted. And after some starts of the BGE, Blender was crashing.

Thomas Szepe (hg1) added inline comments.Jul 30 2014, 6:50 PM

source/gameengine/Ketsji/KX_Scene.cpp
703	Moving ReplicateConstraints() into the physics controller is not so easy. To get the target object(s) I need to loop trough m_logicHierarchicalGameObjects. I think it would be the best to stare the target object and the dat into a list. So I can move this method into the physics controller and I don't need to loop trout all group objects to get the target object.

Changes:

Renamed setup_constraint4object() to SetupObjectConstraints().
Moved SetupObjectConstraints from KX_Scene into the CcdPhysicsControlle.
Moved BlenderDataConversion from BL_BlenderDataConversion into the CcdPhysicsEnvironment.
Moved createConstraint() changes into a separate patch D705, T41294.
Removed constraint deletion workaround. Patch now depends on D701.
Removed Python API GetConstraints() method for now. I will make a separate patch for this.

Looks good to me, do you have a test file?

This revision is now accepted and ready to land.Dec 3 2014, 4:31 AM

Yes, here are my test files.

trampoline_ragdoll_group_2.72.blend95 KBDownload

linked_group_constraints_2.72.blend72 KBDownload

Group-Cons-Regression.blend359 KBDownload

New feature, so I need a second reviewer.

Mitchell Stokes (moguri) added a reviewer: Sergej Reich (sergof).Feb 24 2015, 9:24 PM

I don't know enough about how we use Bullet constraints to give any feedback on the semantics of the code. However, I can suggest some changes that I consider code quality improvements.

source/gameengine/Converter/BL_BlenderDataConversion.cpp
2406	This could be replaced with `if (curcon->type != CONSTRAINT_TYPE_RIGIDBODYJOINT) continue;` to unindent the rest of the code block.
2409	The body of this `if` seems complicated enough to warrant its own method. Setting `skip = true` and then testing for that, it would simply be `return` when a separate method is used.
source/gameengine/Physics/Bullet/CcdPhysicsController.cpp
1678	Invert the condition to `(replica->GetConstraints().size() == 0 \|\| !replica->GetPhysicsController())`, and you can immediately `return`, unindenting the entire method a level.
source/gameengine/Physics/Bullet/CcdPhysicsEnvironment.cpp
3600	`if(!constraintId) return;` would also work here, and unindent a big chunk of code. It also fits the semantics of the method; when there is no constraint, the process of setting it up is done.
3602	There is a lot of code duplication in these `if`-bodies. Possibly by re-ordering some code and pushing the `if`s down, this can be avoided.
3610	"disabled limit" seems a strange concept to me, as in this case there is no limit to disable in the first place. "no limit" makes more sense to me. The same condition is also described differently in different comments. Also see my note about code de-duplication.

This revision now requires changes to proceed.Feb 26 2015, 9:50 PM

Inês Almeida (brita_) removed a reviewer: Inês Almeida (brita_).Feb 28 2015, 3:50 PM

Changes:

Done requested changes.
Add X limit to angular constraint.

The code is starting to look a lot better! It's certainly an improvement over the original. I've added inline comments, mostly about code style and missing comments.

source/gameengine/Converter/BL_BlenderDataConversion.cpp
1744	To me the name "findConstraint" suggests it finds a constraint and returns it. This seems more like an "isConstraintInList" kind of function. Also it needs a comment to describe its intended use.
1916	Please add a comment that explains in which situations it is possible that a constraint would be converted twice, as this won't be obvious to everybody. You've invested time to understand this topic -- ensure that your knowledge of why things are done the way they are done is saved for future developers (including future you) ;-)
2309	Why can these cases be skipped? Maybe add a comment that explains this.
2319	Style: Spaces around `=` and after commas.
2321	`((gotar->GetLayer()&activeLayerBitInfo) != 0)` can be written as `(gotar->GetLayer() & activeLayerBitInfo)`
2324	This can be moved outside the loop.
2333	Style: spaces around `=` and `!=`.
source/gameengine/Ketsji/KX_GameObject.cpp
294	Style: opening braces for functions/methods on the next line.
source/gameengine/Ketsji/KX_GameObject.h
198	Space before "used". Captialize sentences and end with a period.
source/gameengine/Ketsji/KX_Scene.cpp
850	Storing `(*git)->GetPhysicsController()` in a temporary variable may increase readability.
source/gameengine/Physics/Bullet/CcdPhysicsController.cpp
1754	Style: `bRigidBodyJointConstraint *dat`
1757	Style: opening bracket on same line as `for`
1761	`GetPhysicsEnvironment()` can be moved outside the loop.
source/gameengine/Physics/Bullet/CcdPhysicsController.h
722	Add comment that describes the function and its intended use.
source/gameengine/Physics/Bullet/CcdPhysicsEnvironment.cpp
3635	Wouldn't a `switch` statement be better here?
source/gameengine/Physics/Bullet/CcdPhysicsEnvironment.h
277	Add comment that describes the function and its intended use.
source/gameengine/Physics/common/PHY_IPhysicsController.h
139	This function should also be documented in a comment.
source/gameengine/Physics/common/PHY_IPhysicsEnvironment.h
217	This function should also be documented in a comment.

Done changes, except (*git)->GetPhysicsController() it is only used two times. In that case I think it don't improve readability.

About the double conversation.
Basically it is a bug fix (workaround) for an really old bug. Blender will crash when a group instance (dupli group) is made from a linked group instance and both are on the active layer.
Actually converting dupli group produces a lot of bugs. Maybe this is not necessary in future if we get the other dupli group bugs fixed. But for now, I don't have a better solution.

Getting better and better :)

source/gameengine/Converter/BL_BlenderDataConversion.cpp
2309	Skipp has one "p" ;-)
source/gameengine/Ketsji/KX_GameObject.h
200	Make sure that you end sentences with a period and start sentences with a capital letter. It now looks like this is one two-line sentence. Documentation generation tooling may also interpret it as such, and present it on a single line.
source/gameengine/Physics/Bullet/CcdPhysicsEnvironment.h
278	Typo "consrtaints "