Fix duplicate mob squash logic in 3D tutorial (Your first 3D game)

[jjaxu]

fixes #7592

Hi there! 👋

I was following the Godot 3D tutorial (Your first 3D game) and believe to have discovered a bug regarding squashing the mobs.

The collision detection section mentions "Godot makes the body move sometimes multiple times in a row to smooth out the character's motion. So we have to loop over all collisions that may have happened." and thus checks for collisions in a loop.

This current logic however will sometimes result in duplicate calls to mob.squash(), which will double or triple count the score for one mob kill. The simple fix is to break out of the loop after handling the first squash occurrence to prevent further duplicate calls.

Demo

Here's the demo, notice how the first two kills increased the score by two, then the last two by one. I've seen anywhere between 1-3

I've updated all occurrences of this code snippet that appear for both GDScript and C# to include the break statement, as well as an explanation of the logic. The files changed are:

• 06.jump_and_squash
• 07.killing_player
• 09.adding_animations

Preview

GDScript

C#

Explanation

Please help verify whether this is the correct solution. If it is, I'm also more than happy to apply the fix to the godot-3d-dodge-the-creeps repo as well.

Thanks and have a great day!

• Bugsquad edit, alternative to: 3d game tutorial bug on squashing #8333

[jjaxu]

Updated the PR with periods in comments, as well as adjusted the wording of the explanation. Screenshots updated to reflect the changes

[jjaxu]

I believe this fixes: #7592

[AThousandShips]

To link it add the line "Fixes: #7592" to your first message (the one at the very top)

This code also now more closely matches the code in the demo project

[jjaxu]

Linked the issue. Glad to see using break was the intended solution. What about the standalone godot-3d-dodge-the-creeps repo? Seems to me that it's no longer maintained and the demos have since been migrated to godot-demo-projects

[AThousandShips]

That does seem to be the case yes

[jjaxu]

@AThousandShips Is there anything else that needs to be done before this can be approved and merged? There is nothing else from my side.

[AThousandShips]

You'll have to see what further reviewers say

[sdlllllll]

I'm a beginner and also noticed this bug in the tutorial. Is that mean if I use collision detection in any calculation, a "break" or "return" is necessary in order to prevent a duplicate calculation.

[george-lim]

This would be a very helpful for onboarding new people, I also ran into this bug.

@AThousandShips any chance we could get another pair of eyes from the team to review? It's definitely a nice to have and the changes seem harmless / trivial :)

[AThousandShips]

You can go join https://chat.godotengine.org and post it there for attention

[AThousandShips]

Looks good to me, haven't verified the behavior but is the same logic as the 2D case

[AThousandShips]

Please squash your commits into one, see here

[jjaxu]

Thanks @AThousandShips, I have squashed my commits and rebased the changes onto the latest master.

[jjaxu]

@AThousandShips Commit 426c83b added some whitespace changes which caused merge conflicts. I've rebased my changes again to resolve. Will require approval again (for the pipeline). Thanks!

[Calinou]

Thanks!

[J-John1]

Thanks for your help everyone!