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 fill_region method to the AStarGrid2D #79495

Merged
merged 1 commit into from
Aug 4, 2023

Conversation

Chaosus
Copy link
Member

@Chaosus Chaosus commented Jul 15, 2023

Closes godotengine/godot-proposals#7276 by adding a property default_solid_enabled to the AStarGrid2D - after the user set it to true, the next call of update method will make all points solid. Adds the fill_region method to assign the solid flag and weight_scale to the specified region on the gird.

@dalexeev
Copy link
Member

Maybe it should be an optional argument to the update() method instead?

Or we can add a fill_region() method that would fill the specified region (must be a sub-region of the current region) with the specified value. In my opinion, this is better, as it can be used in more cases (for batch operations). The main complaint of the linked proposal is that you have to use a loop, but that's just 3 lines of code. So there is a concern about GDScript performance?

@Chaosus Chaosus force-pushed the astargrid_all_solid branch 2 times, most recently from 86897bd to e895865 Compare July 15, 2023 07:55
@Chaosus Chaosus changed the title Add a property to initialize all points as solid in AStarGrid2D Add a fill_region method to the AStarGrid2D Jul 15, 2023
@Chaosus
Copy link
Member Author

Chaosus commented Jul 15, 2023

@dalexeev I think fill_region is the neat idea! Thanks!

@Chaosus Chaosus force-pushed the astargrid_all_solid branch 2 times, most recently from 82a78a3 to 2a60114 Compare July 15, 2023 08:37
core/math/a_star_grid_2d.cpp Outdated Show resolved Hide resolved
core/math/a_star_grid_2d.cpp Outdated Show resolved Hide resolved
Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

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

I forgot to remind you about this check, sorry. The rest of the code and approach looks good to me. Not sure how much the GDScript loop is less performant though. Probably worth asking the proposal author, perhaps they meant some other problem?

core/math/a_star_grid_2d.cpp Show resolved Hide resolved
core/math/a_star_grid_2d.cpp Show resolved Hide resolved
@dalexeev
Copy link
Member

For consistency, the following should be added to the description of the methods:

[b]Note:[/b] Calling [method update] is not needed after the call of this function.

Also I'm a bit worried that there is no error/warning if the specified region is outside the grid region boundaries.

Copy link
Contributor

@smix8 smix8 left a comment

Choose a reason for hiding this comment

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

I am not really involved with the AStar classes but looks useful and fine.

@akien-mga akien-mga merged commit b8fa19c into godotengine:master Aug 4, 2023
13 checks passed
@akien-mga
Copy link
Member

Thanks!

@Chaosus Chaosus deleted the astargrid_all_solid branch August 5, 2023 05:50
@2fd5
Copy link

2fd5 commented Aug 13, 2023

Thank you for implementing this and everyone involved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants