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

Added rotation using degrees; Redefined and extended vector constants; Minor clean-up #102

Merged
merged 11 commits into from
May 25, 2024

Conversation

Flashing-Blinkenlights
Copy link
Collaborator

I took the liberty of adding functions for rotating vectors using degrees (a little more intuitive in my opinion), and redefined a bunch of vectors allowing for greater flexibility and clarity while remaining backward-compatible.
Formatted with black, and ran a linter or two over it plus some tiny refactoring tweaks here and there.

Feel free to change anything at all, I'm up to discuss anything!

Copy link
Owner

@avdstaaij avdstaaij left a comment

Choose a reason for hiding this comment

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

Some nice additions here! I did leave a bunch of comments though, please take a look at them.

Please revert the formatting changes, though. I'm generally not a fan of most automatic formatters, especially super-strict ones like black, because I find that custom formatting here and there (mainly spacing) can make code much more readable. I see many examples of that here, most notably neighbors2D and neighbors3D. Furthermore, if I'm going to standardize the code formatting, I'd rather do it all at once - doing it in a PR like this makes it harder for me to see what the actual changes are.

src/gdpc/vector_tools.py Outdated Show resolved Hide resolved
src/gdpc/vector_tools.py Outdated Show resolved Hide resolved
src/gdpc/vector_tools.py Outdated Show resolved Hide resolved
src/gdpc/vector_tools.py Outdated Show resolved Hide resolved
src/gdpc/vector_tools.py Outdated Show resolved Hide resolved
src/gdpc/vector_tools.py Outdated Show resolved Hide resolved
src/gdpc/vector_tools.py Outdated Show resolved Hide resolved
src/gdpc/vector_tools.py Outdated Show resolved Hide resolved
src/gdpc/vector_tools.py Outdated Show resolved Hide resolved
src/gdpc/vector_tools.py Outdated Show resolved Hide resolved
@avdstaaij
Copy link
Owner

Looks like I forgot to submit one of my comments:

I quite like the DIAGONALS_2D/3D aliases, please move them up to the 2D/3D categories.
There is one issue though, since the original DIAGONALS constants were tuples, not sets, this is technically a breaking change. This may be over-careful, but please wrap them in tuple() and add a comment about updating them to sets on the next major version release.

@Flashing-Blinkenlights
Copy link
Collaborator Author

Looks like I forgot to submit one of my comments:

I quite like the DIAGONALS_2D/3D aliases, please move them up to the 2D/3D categories. There is one issue though, since the original DIAGONALS constants were tuples, not sets, this is technically a breaking change. This may be over-careful, but please wrap them in tuple() and add a comment about updating them to sets on the next major version release.

I can't think of a use-case where a tuple would be beneficial to use over a set here. The only difference I could see is that a set is mutable, but that's still reverse-compatible
On the other hand, having these vectors as sets allows for operations such as unions and intersects with other sets of vectors, which would not be possible with a tuple, hence I think converting them is unnecessary and may prevent the use of its full functionality.

@Flashing-Blinkenlights
Copy link
Collaborator Author

Some nice additions here! I did leave a bunch of comments though, please take a look at them.

Please revert the formatting changes, though. I'm generally not a fan of most automatic formatters, especially super-strict ones like black, because I find that custom formatting here and there (mainly spacing) can make code much more readable. I see many examples of that here, most notably neighbors2D and neighbors3D. Furthermore, if I'm going to standardize the code formatting, I'd rather do it all at once - doing it in a PR like this makes it harder for me to see what the actual changes are.

Should I revert all non-functional changes, including isort, newlines, spacing between operators, commas at the end of listings and type annotations?

@avdstaaij
Copy link
Owner

(Discussion continued on Discord)

Copy link
Owner

@avdstaaij avdstaaij left a comment

Choose a reason for hiding this comment

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

I've got a few more comments, mostly formatting nitpicks but a few functional comments as well.

I've ignored most formatting changes, but I did comment on the ones that I found actively detrimental. I'll look into putting up an autoformatter soon! Maybe I need to let go of custom formatting for the sake of consistency... but I'm not quite ready yet. I do want any color I like sometimes.

src/gdpc/vector_tools.py Outdated Show resolved Hide resolved
src/gdpc/vector_tools.py Outdated Show resolved Hide resolved
src/gdpc/vector_tools.py Outdated Show resolved Hide resolved
src/gdpc/vector_tools.py Outdated Show resolved Hide resolved
src/gdpc/vector_tools.py Show resolved Hide resolved
src/gdpc/vector_tools.py Show resolved Hide resolved
src/gdpc/vector_tools.py Outdated Show resolved Hide resolved
src/gdpc/vector_tools.py Outdated Show resolved Hide resolved
src/gdpc/vector_tools.py Outdated Show resolved Hide resolved
src/gdpc/vector_tools.py Outdated Show resolved Hide resolved
@avdstaaij avdstaaij merged commit 2f2c24b into avdstaaij:master May 25, 2024
avdstaaij added a commit that referenced this pull request May 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants