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 method to retrieve all points within a region to AStarGrid2D #91868

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Chaosus
Copy link
Member

@Chaosus Chaosus commented May 12, 2024

Closes godotengine/godot-proposals#6276

(Not sure whether It's useful or not)

@Chaosus Chaosus requested review from a team as code owners May 12, 2024 08:35
@Chaosus Chaosus added this to the 4.x milestone May 12, 2024
@MMUTFX2053
Copy link

is there a down side to adding more utility functions ? or even classes ? you question its usefulness but i think if it helps the developers to make games and apps more easily, it should be included, unless it increases the binary size too much or slows down performance

@AThousandShips
Copy link
Member

is there a down side to adding more utility functions ? or even classes ?

There's such a thing as engine bloat, this feature is trivial to do yourself, the only difference is a slight loss of performance

@MMUTFX2053
Copy link

is there a down side to adding more utility functions ? or even classes ?

There's such a thing as engine bloat, this feature is trivial to do yourself, the only difference is a slight loss of performance

there were talks about officially maintained gdextension plugins when 4.0 hit stable, maybe an officially maintained "boost" like gdextension plugin could help offload all the bloat, and the devs still get to have tons of utility

@AThousandShips
Copy link
Member

You couldn't really do this part as an extension, not necessarily, unless you add an extended type, you can't add methods to existing classes, but that's off topic for this PR

@Chaosus
Copy link
Member Author

Chaosus commented May 12, 2024

Sometimes its not simply to understand whether its necessary addition or not. If this considered as a bloat, I'm fine to close this PR without problem (at least its was easy to do and I need some trainings to do not forget skills).

@AThousandShips
Copy link
Member

I think this could be useful, but the question is if it's enough of a convenience function considering the ease at which you can accomplish it in scripting

One detail I'd suggest changing is forgoing the _get_point_unchecked and instead just performing the lookup manually using an offset, we already have the range so it'd be easier to replace that call and the subtractions with just iterating over an offset from the actual size

@Chaosus Chaosus force-pushed the astargrid_get_point_positions_in_region branch 2 times, most recently from 001ca5e to 7dea8bc Compare May 12, 2024 13:46
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.

I think the code and everything make sense, though I haven't tested it, but I'm not convinced this is necessary to put in core, I'm not sure I see the specific need to be able to fetch all the points directly, I'd probably expect any usage of this for things like drawing to just iterate over the positions and using the get_point_position or similar, I'm less sure about the usefulness of fetching all the points at once, especially since it doesn't consider solid points so any such checking would require further testing (which couldn't be done with this output anyway as the solid check checks for the grid points)

Also any user who might be interested in the points in any other ordering, like by column, or right to left instead etc. couldn't use this

So I'm not sure about the actual scope where this would be used directly, and the only benefit would be performance for fetching this particular case, but I'm not sure that performance would be critical if one already iterates over the output and processes it, so I'm unsure about the specific use case for this

But very interesting and nice work either way, would 100% approve if I wasn't unsure about the necessity

Edit: What I think might be really interesting though would be to return an array of dictionaries containing point information, like {id, pos, solid, weight_scale}, that I feel would be very useful, and would greatly help with performance etc.

@Chaosus Chaosus force-pushed the astargrid_get_point_positions_in_region branch from 7dea8bc to a7f6b96 Compare May 12, 2024 14:30
@Chaosus
Copy link
Member Author

Chaosus commented May 12, 2024

@AThousandShips Yeah, it's good idea - I've changed it to return a TypedDictionary (with id, position, solid and weight_scale members):

изображение

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.

LGTM, makes sense and should be useful! 🎉

@Chaosus Chaosus force-pushed the astargrid_get_point_positions_in_region branch from a7f6b96 to c222cf9 Compare May 12, 2024 14:33
doc/classes/AStarGrid2D.xml Outdated Show resolved Hide resolved
doc/classes/AStarGrid2D.xml Outdated Show resolved Hide resolved
@Chaosus Chaosus force-pushed the astargrid_get_point_positions_in_region branch from c222cf9 to db2e09e Compare May 12, 2024 15:14
@akien-mga akien-mga modified the milestones: 4.x, 4.3 May 13, 2024
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.

Add a method to get all points from an AStarGrid2D
5 participants