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

Update using_character_body_2d.rst #10675

Closed
wants to merge 4 commits into from
Closed

Conversation

Nio03
Copy link

@Nio03 Nio03 commented Feb 12, 2025

I added an example of an if statement on move_and_slide with collision.get_collider().name to detect collisions with specific object types.

I added an example of an if statement on move_and_slide with collision.get_collider().name to detect collisions with specific object types.
@tetrapod00 tetrapod00 added enhancement area:manual Issues and PRs related to the Manual/Tutorials section of the documentation topic:physics labels Feb 12, 2025
@AThousandShips
Copy link
Member

Looks good, but the C# example needs to be updated to match

@Nio03
Copy link
Author

Nio03 commented Feb 12, 2025

Looks good, but the C# example needs to be updated to match

done

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but needs someone to verify the C# code as I don't use C#

Copy link
Contributor

@tetrapod00 tetrapod00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As is I think the example teaches bad practice. If we want to include an example like this, I left some different directions that the example could go in a comment.

Edit: and to be clear I'm still a casual user when to comes to best practice for GDScript, but the example here seems fragile even to me

Comment on lines +142 to +143
if collision.get_collider().name == "TileMapLayer" or collision.get_collider().name == "RigidBody2D":
print("I collided with ", collision.get_collider().name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the intention with this whole example?

Is the intention to check if we are colliding with any TileMapLayer or any RigidBody2D? If so, doing so by checking the name of the collider seems fragile and likely to teach bad habits. RigidBodies and TileMapLayers are unlikely to be named exactly "RigidBody2D" or "TileMapLayer". Something like the following is much more robust:

		if collision.get_collider() is TileMapLayer or collision.get_collider() is RigidBody2D:

But I'm not sure if we're currently supposed to use type casting in examples in general at this time.

I believe groups are also reasonably idiomatic for this:

		if collision.get_collider().is_in_group("Player"):

If the intention is merely to teach that you can use some sort of filtering on the results using the names, I would try filtering by something like "Wall" or "Enemy". But that is really not very robust, since any duplicated/instantiated nodes will have names like "Wall2" or "Wall3".

@skyace65
Copy link
Contributor

skyace65 commented Mar 4, 2025

Closing due to the lack of a response to tetrapod's review. @Nio03 if you want to keep working on this in the future feel free to ping me and I'll re-open this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
archived area:manual Issues and PRs related to the Manual/Tutorials section of the documentation enhancement topic:physics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants