-
-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Add a method to retrieve all points within a region to AStarGrid2D
#91868
Conversation
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 |
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 |
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 |
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). |
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 |
001ca5e
to
7dea8bc
Compare
There was a problem hiding this 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.
7dea8bc
to
a7f6b96
Compare
@AThousandShips Yeah, it's good idea - I've changed it to return a |
There was a problem hiding this 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! 🎉
a7f6b96
to
c222cf9
Compare
c222cf9
to
db2e09e
Compare
Thanks! |
Closes godotengine/godot-proposals#6276
(Not sure whether It's useful or not)