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

Fix _shape_shape typos in TileData collision shape API #49940

Closed
wants to merge 1 commit into from
Closed

Fix _shape_shape typos in TileData collision shape API #49940

wants to merge 1 commit into from

Conversation

Xrayez
Copy link
Contributor

@Xrayez Xrayez commented Jun 27, 2021

According to existing add_collision_shape() and remove_collision_shape(), the renamed methods should be set/get_collision_shape(), and not set/get_collision_shape_shape().


This PR is generously donated by Goost.

According to existing `add_collision_shape()` and `remove_collision_shape()`, the renamed methods should be `set/get_collision_shape()`, and not `set/get_collision_shape_shape()`.
@Xrayez Xrayez requested a review from a team as a code owner June 27, 2021 11:08
@Chaosus Chaosus added this to the 4.0 milestone Jun 27, 2021
@groud
Copy link
Member

groud commented Jun 28, 2021

This is superseded by #49859 where I got rid of those functions. Shapes are getting replaced by a polygon instead, so I removed those functions.

But no, this was not a typo anyway, add_collision_shape and remove_collision_shape removes also the data associated with the shape (one_way and one_way_margin), while set/get_collision_shape_shape() only affect the Shape2D resource. I would have liked to find a better name but well, I did not.

@Xrayez
Copy link
Contributor Author

Xrayez commented Jun 28, 2021

The issue with method naming in #49859 still persists: _polygon_polygon(). If you really want to be explicit with this kind of API, most of the time, there are better alternatives, like: _polygon_points(), or _polygon_vertices(), or even with shape it would be _shape_resource().

@groud
Copy link
Member

groud commented Jun 28, 2021

The issue with method naming in #49859 still persists: _polygon_polygon(). If you really want to be explicit with this kind of API, most of the time, there are better alternatives, like: _polygon_points(), or _polygon_vertices(), or even with shape it would be _shape_resource().

Ah my bad you are right, I can change that then. Though I'm not sure there's a real problem with it TBH.

@Xrayez
Copy link
Contributor Author

Xrayez commented Jun 28, 2021

Though I'm not sure there's a real problem with it TBH.

To be honest, I submitted this PR immediately because I really thought there was an obvious copy-paste mistake regardless. Even Chaosus assigned the "bug" label to this PR. We should give good example for everyone reading the Godot's codebase, especially for new contributors, and especially when people tend to take existing Godot code as a reference for their own code.

@akien-mga
Copy link
Member

akien-mga commented Jun 29, 2021

Superseded by #49859.

@akien-mga akien-mga closed this Jun 29, 2021
@Xrayez Xrayez deleted the tileset-fix-collision-shape-api branch June 29, 2021 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants