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

Refactor of code #22

Merged
merged 26 commits into from
Nov 19, 2024
Merged

Refactor of code #22

merged 26 commits into from
Nov 19, 2024

Conversation

salam99823
Copy link
Collaborator

Changes:

Added:

  • feature parallel for fast work on large data.
  • pedantic lint params.
  • private utils module.

Moved:

  • collider::rapier2d and collider::avian2d to root of crate.
  • logic of public functions to utils for simply.

Removed:

  • thiserror from dependencies.
  • collider module.

Other

  • dependency on bevy replaced with bevy_math.

@salam99823
Copy link
Collaborator Author

Hi, what do you think about the latest commit to dev branch.

@shnewto
Copy link
Owner

shnewto commented Nov 13, 2024

Hello @salam99823! There's a lot of really useful refactors here and thank you for updating the rapier2d example to use linear velocity instead of just moving the sprite to new pixels 🙃

Regarding the changes in your PR here (your main branch). I'm on totally board, they're good changes. My initial implementation had in mind a kind of "drop a new physics framework into the collider directory" and be up and running, but it only adds noise, it's just as easy to drop a new framework into the src directory.

Regarding the changes in your dev branch, those are good changes too but they took me a minute to think about. Looking at your code, it's clear that using generate_multi_collider and generate_collider instead of individual symbols for every collider makes the code cleaner and more light weight. Initially I liked the experience of being able to write generate_ and tab complete in the editor the collider you were looking for. But again, it's not harder to use ColliderType, it actually makes it much clearer to the user what colliders are supported.

All that said, both branches are changes I'll be very glad to incorporate into this project. Thank you for all the time you've given to this and the edges project.

I'll hold off on merging in case you're thinking you'll update your main branch with your dev changes.

@shnewto
Copy link
Owner

shnewto commented Nov 17, 2024

@salam99823 is this ready for review?

@salam99823
Copy link
Collaborator Author

Hello @shnewto! Yes, I think it can be released as a new version.

@shnewto shnewto merged commit 2077b1a into shnewto:main Nov 19, 2024
7 checks passed
@shnewto
Copy link
Owner

shnewto commented Nov 19, 2024

@salam99823 thank you again for these substantial and very positive refactors!

@salam99823
Copy link
Collaborator Author

I'm glad you liked it.

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