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 static types in 2D Navigation A Star demo #1008

Merged
merged 5 commits into from
Aug 31, 2024

Conversation

MatrixFrog
Copy link
Contributor

@MatrixFrog MatrixFrog commented Jan 3, 2024

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

@MatrixFrog MatrixFrog marked this pull request as ready for review January 3, 2024 05:54
@MatrixFrog MatrixFrog changed the title Add some static types Add static types in 2D Navigation A Star demo Jan 3, 2024
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.

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().

2d/navigation_astar/character.gd Outdated Show resolved Hide resolved
@MatrixFrog MatrixFrog requested a review from dalexeev January 8, 2024 01:43
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.

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).

Copy link
Member

@aaronfranke aaronfranke left a 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.

2d/navigation_astar/character.gd Outdated Show resolved Hide resolved
2d/navigation_astar/character.gd Outdated Show resolved Hide resolved
2d/navigation_astar/pathfind_astar.gd Outdated Show resolved Hide resolved
2d/navigation_astar/pathfind_astar.gd Outdated Show resolved Hide resolved
2d/navigation_astar/pathfind_astar.gd Outdated Show resolved Hide resolved
@MatrixFrog
Copy link
Contributor Author

Thanks. I should have time to test this out in a couple days.

2d/navigation_astar/pathfind_astar.gd Outdated Show resolved Hide resolved
2d/navigation_astar/pathfind_astar.gd Outdated Show resolved Hide resolved
2d/navigation_astar/pathfind_astar.gd Show resolved Hide resolved
2d/navigation_astar/character.gd Show resolved Hide resolved
2d/navigation_astar/character.gd Show resolved Hide resolved
2d/navigation_astar/character.gd Show resolved Hide resolved
@MatrixFrog
Copy link
Contributor Author

ok just tested it out, this seems to be working well and should be ready to merge! thanks!

@MatrixFrog MatrixFrog requested a review from aaronfranke August 29, 2024 23:23
Copy link
Member

@aaronfranke aaronfranke left a 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.

Copy link
Member

@Calinou Calinou left a 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!

@Calinou Calinou merged commit fa6061c into godotengine:master Aug 31, 2024
1 check passed
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