It fix T46381. Normally BL_Action::Update (manage action time, end, loop…) should be called the same number of times as BL_Action::UpdateIPO (update action position, scale ect… in the game object).
But the bug report shows that UpdateIPO is called one less time than Update. To fix it i revert the commit 362b25b38287cb75e4d22b30bdbc7f47e8eb3fdf and implement a mutex in BL_Action::Update.
Example file :
Details
Details
- Reviewers
Campbell Barton (campbellbarton) Sybren A. Stüvel (sybren) Ulysse Martin (youle) Daniel Stokes (kupoman) Jorge Bernal (lordloki) Mitchell Stokes (moguri) - Maniphest Tasks
- T39928: Blender crash/freeze when game engine is started with animation played directly on camera object with parents.
T46381: The animation is not played to the end - Commits
- rBS728d1ec50464: BGE: Fix T46381 : last action frame not updated.
rBbedc58ac4e5e: BGE: Fix T46381 : last action frame not updated.
rB728d1ec50464: BGE: Fix T46381 : last action frame not updated.
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- ge_action_end_frame
Event Timeline
| source/gameengine/Ketsji/BL_ActionManager.cpp | ||
|---|---|---|
| 160 | I let BL_Action::Update manage all. | |
Comment Actions
I feel like this is a hack on top of a hack. If we clean up the hack introduced by rB362b25 and move UpdateIPO back into BL_Action::Update(), the issue should be fixed.
Comment Actions
- Revert "Fix T39928: Blender crash/freeze when game engine is started with animation played directly on camera object with parents."
- Use mutex instead of separate function UpdateIPO
Comment Actions
This approach is much better. I'm not a huge fan of BL_Action::InitMutux() and EndMutux() since this exposes details of BL_Action to the "outside world," but it is acceptable for now. In the future we might want to consider a utility class we can statically initialize to handle mutux init/end.
Comment Actions
Nice work; actually solving the thread-safety issue is much better than the hack that was there. I agree with @Mitchell Stokes (moguri) that the initialization/finalization might need some cleanup. However, I'm also concerned that the lock might be too broad -- see my inline comments for that.
| source/gameengine/Ketsji/BL_Action.cpp | ||
|---|---|---|
| 61 | There should be comments explaining what this mutex is for. | |
| 515 | Where is the thread-unsafety of UpdateIPO? Is it also unsafe to call UpdateIPO on multiple distinct objects in parallel? Maybe this lock is too defensive, and a lock at the object level would solve the thread-safefty issue as well, and increase performance. | |
| source/gameengine/Ketsji/BL_Action.h | ||
| 148 | All functions should have a comment that explains what it does and why it is needed. IMO this is more important than the code itself, as once that's there the majority of the work is done (the reasoning about what the code should do), and implementation from then on is straight forward. Code style: use Blender code style. | |
| source/gameengine/Ketsji/BL_Action.cpp | ||
|---|---|---|
| 61 | Please also include an explanation of why a spinlock is appropriate here. | |
| 515 | "children" is already plural, so "childrens" is doubly plural. The comment also doesn't explain why it is unsafe, and even causes more questions, like "if the update is recursive, isn't this lock going to cause deadlock?" | |
| source/gameengine/Ketsji/BL_Action.h | ||
| 148 | "animations" should be "animation". | |
| source/gameengine/Ketsji/KX_GameObject.cpp | ||
| 948 ↗ | (On Diff #5222) | Unclear what "it" is in this sentence. The closing */ can be on the end of the line above it. |
| source/gameengine/Ketsji/KX_KetsjiEngine.cpp | ||
| 78 ↗ | (On Diff #5222) | "for manage" is incorrect. It's either "for managing" or "to manage". |
| 187 ↗ | (On Diff #5222) | This comment isn't necessary, as it only repeats the name of the function call. |
| 209 ↗ | (On Diff #5222) | This comment isn't necessary, as it only repeats the name of the function call. |
Comment Actions
The patch looks good to me, nice work on the comments. Do you have an example blend file that allows us to properly test possible threading issues?
Comment Actions
@Sybren A. Stüvel (sybren) : The examples files are in T46381 and in the moguri's commit reverted here.