-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 static types in 2D Navigation A Star demo #1008
Conversation
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.
Please add void
type hints for all functions in character.gd
and 2 functions in pathfind_astar.gd
, float
for unused parameter _delta
. Also please add type hints or at least type inference for local variables arrived_to_next_point
in _process()
, last_point
and current_point
in _draw()
.
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.
The changes look good to me, but I'm not sure we've reached consensus on #868. Personally, I support static typing in demo projects and find it improves readability. However, I recognize the concerns and think we should first define the rules for using static typing in demo projects.
There are also questions about type inference if the initializer does not contain the type name and whether it is necessary to specify the type for for
loop variables (a new feature in 4.2).
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.
When the type is not written on the same line, it should be explicitly specified. https://docs.godotengine.org/en/stable/tutorials/scripting/gdscript/gdscript_styleguide.html#inferred-types
Also this PR has conflicts and needs to be rebased.
Thanks. I should have time to test this out in a couple days. |
ok just tested it out, this seems to be working well and should be ready to merge! thanks! |
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 didn't test this but it looks correct.
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.
Tested locally, it works as expected.
Thanks!
I was just looking at this demo and realized that jumping to definitions of things would work a little better if the types were filled in, so I went ahead and did it.
Part of #868