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

Add a check to prevent user to call AStarGrid2D::update when its not needed #93993

Merged
merged 1 commit into from
Jul 7, 2024

Conversation

Chaosus
Copy link
Member

@Chaosus Chaosus commented Jul 6, 2024

I think it's a good small fix to softly prevent user to call it in unappropriated place/time.

@Chaosus Chaosus requested a review from a team as a code owner July 6, 2024 09:26
@Chaosus Chaosus added this to the 4.3 milestone Jul 6, 2024
@AThousandShips
Copy link
Member

I'd just make it an early exit instead of an error

@AThousandShips
Copy link
Member

I'd also say this should be noted in the documentation

@Chaosus
Copy link
Member Author

Chaosus commented Jul 6, 2024

I'd just make it an early exit instead of an error

I disagree - the error will give a user a proper sense of where and when to use this method. Otherwise, the user may spam this method and not even understand that because it returns silently.

@AThousandShips
Copy link
Member

But calling it at the "wrong" time with an early exit doesn't come with any cost, and having a check in GDScript for is_dirty will be slower than just calling update and have it just deal with it

@Chaosus Chaosus force-pushed the astargrid2d_update_fix branch from 998ad0c to 26edc90 Compare July 6, 2024 09:53
@Chaosus
Copy link
Member Author

Chaosus commented Jul 6, 2024

But calling it at the "wrong" time with an early exit doesn't come with any cost, and having a check in GDScript for is_dirty will be slower than just calling update and have it just deal with it

Hm, I didn't think about the GDScript speed, maybe you've right, changed

core/math/a_star_grid_2d.cpp Show resolved Hide resolved
@Chaosus Chaosus force-pushed the astargrid2d_update_fix branch from 26edc90 to 453c875 Compare July 6, 2024 11:59
@akien-mga akien-mga merged commit 5bc5ea8 into godotengine:master Jul 7, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@Chaosus Chaosus deleted the astargrid2d_update_fix branch July 7, 2024 11:51
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