Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change misleading comment in 'Your first 3D game' #9382

Merged
merged 1 commit into from
Nov 17, 2024

Conversation

fmnjose
Copy link
Contributor

@fmnjose fmnjose commented May 15, 2024

On the collision processing code (Chapter 6 - Jumping and squashing monsters) there is a misleading comment saying that the collision.get_collider() == null has the purpose of checking for a collision with the ground, while the purpose is to prevent a null pointer that happens when there are multiple collisions with a mob in a single frame.

This addresses the following open issue: #9355

On the collision processing code (Chapter 6 - Jumping and squashing monsters) there is a misleading comment saying that the collision.get_collider() == null has the purpose of checking for a collision with the ground.

This addresses the following open issue: godotengine#9355
@Bamarin
Copy link
Contributor

Bamarin commented May 30, 2024

Not against this change, but rather curious: how can the collider be null?

You mentioned this happens after we squash the mob and delete the object (for subsequent collisions with the same mob, the collider will be null).
But we are breaking out of the look right after the squash, so that null check shouldn't be necessary.

Indeed, it's missing in the C# counterpart.
Is it because the break statement acts differently in GDscript?

@raulsntos
Copy link
Member

Indeed, it's missing in the C# counterpart.
Is it because the break statement acts differently in GDscript?

No, the check also exists in C#:

if (collision.GetCollider() is Mob mob)

This checks that the collider instance is of type Mob and that it's not null.

@raulsntos raulsntos added the area:getting started Issues and PRs related to the Getting Started section of the documentation label May 30, 2024
@fmnjose
Copy link
Contributor Author

fmnjose commented May 30, 2024

Not against this change, but rather curious: how can the collider be null?

You mentioned this happens after we squash the mob and delete the object (for subsequent collisions with the same mob, the collider will be null). But we are breaking out of the look right after the squash, so that null check shouldn't be necessary.

Indeed, it's missing in the C# counterpart. Is it because the break statement acts differently in GDscript?

I've tried removing the code block, and I started getting null pointers when hitting the mobs from certain angles.
I printed the collision counters and noticed that in those cases there were multiple collisions with the mob, and the first collision processed would delete the object, inducing a null pointer for the next ones being processed.

Either way the block of code does not serve the purpose of checking for a collision with the ground, as it will never have a null collider, and removing this code doesn't affect the collision with the ground.

@mhilbrunner mhilbrunner merged commit 0a6c134 into godotengine:master Nov 17, 2024
1 check passed
@mhilbrunner
Copy link
Member

Merged. Thanks and congrats on your first merged contribution! Sorry this took a while :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:getting started Issues and PRs related to the Getting Started section of the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please remove this code block, or add some explanation to it
4 participants