This patch will add a physics constraints replication for group instances.
Initial patch T31443 from moerdn (Martin Sell).
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
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.
Comment Actions
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 | These includes can be moved to BL_BlenderDataConversion.cpp if you forward declare KX_GameObject and PHY_IPhysicsController. | |
| 45 | 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 | Is this for this patch or another patch? It doesn't seem related. Maybe I'm just missing some context. | |
| 563 | 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 | This also looks like it's from another patch. | |
| 541 | 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()? | |
| source/gameengine/Ketsji/KX_GameObject.h | ||
|---|---|---|
| 80 | This is necessary to avoid blender from crashing, if getConstraints() is called. | |
| source/gameengine/Ketsji/KX_PyConstraintBinding.cpp | ||
| 480 | No. I deliberately put this in this patch. I thought it is not necessary to make an extra patch for this. | |
| 563 | 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. | |
| 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. | |
Comment Actions
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.
Comment Actions
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 ↗ | (On Diff #2545) | 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 ↗ | (On Diff #2545) | 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 ↗ | (On Diff #2545) | There is a lot of code duplication in these if-bodies. Possibly by re-ordering some code and pushing the ifs down, this can be avoided. |
| 3610 ↗ | (On Diff #2545) | "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. |
Comment Actions
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 ↗ | (On Diff #3746) | Style: bRigidBodyJointConstraint *dat |
| 1757 ↗ | (On Diff #3746) | Style: opening bracket on same line as for |
| 1761 ↗ | (On Diff #3746) | GetPhysicsEnvironment() can be moved outside the loop. |
| source/gameengine/Physics/Bullet/CcdPhysicsController.h | ||
| 722 ↗ | (On Diff #3746) | Add comment that describes the function and its intended use. |
| source/gameengine/Physics/Bullet/CcdPhysicsEnvironment.cpp | ||
| 3635 ↗ | (On Diff #3746) | Wouldn't a switch statement be better here? |
| source/gameengine/Physics/Bullet/CcdPhysicsEnvironment.h | ||
| 277 ↗ | (On Diff #3746) | Add comment that describes the function and its intended use. |
| source/gameengine/Physics/common/PHY_IPhysicsController.h | ||
| 139 ↗ | (On Diff #3746) | This function should also be documented in a comment. |
| source/gameengine/Physics/common/PHY_IPhysicsEnvironment.h | ||
| 217 ↗ | (On Diff #3746) | This function should also be documented in a comment. |
Comment Actions
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.
Comment Actions
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 ↗ | (On Diff #3797) | Typo "consrtaints " |
| source/gameengine/Ketsji/KX_GameObject.h | ||
|---|---|---|
| 200 | Actually it is a single line sentence (seems comma is missing). But I will change it to a two two-line sentence. Looks better. | |