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

Migrate to Bevy 0.15 #113

Merged
merged 2 commits into from
Dec 19, 2024
Merged

Migrate to Bevy 0.15 #113

merged 2 commits into from
Dec 19, 2024

Conversation

haihala
Copy link
Contributor

@haihala haihala commented Dec 4, 2024

Hey so I don't know what I'm doing. Followed rust-analyzer, clippy, the tests and what little intuition I have, I got it to compile and the examples to run. I have a wonky setup and can't be a 100% sure even the box game works, but the synctest seems to, so I'm relying on that.

For the love of everything good and holy, please do read through this and see if you can make it better. Especially the places where bevy::utils::get_short_name were used, as I never figured out if that was necessary and/or how to do it now that get_short_name no longer exists. Maybe this link to the migration guide helps, but the ShortName type doesn't seem to exist.

Copy link
Collaborator

@johanhelsing johanhelsing left a comment

Choose a reason for hiding this comment

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

Thanks for porting! We should clarify what happened to get_short_name, and if that image handle is moved somewhere else that we should rollback, but otherwise looks good :)

This is what bevy 0.15 migration docs suggest
@haihala
Copy link
Contributor Author

haihala commented Dec 6, 2024

I consulted several discord channels, and they claim that the disqualified crate was split off from Bevy and we should use it here. I then did just that.

Copy link
Collaborator

@johanhelsing johanhelsing left a comment

Choose a reason for hiding this comment

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

Looks good to me! This pr was a lot cleaner than i expected reading the description. Thanks for doing all the digging after short names.

@haihala
Copy link
Contributor Author

haihala commented Dec 6, 2024

Looks good to me! This pr was a lot cleaner than i expected reading the description. Thanks for doing all the digging after short names.

Thanks for the kind words. I don't have much expertise with contributing to open source and don't know the crates, so I was guessing a log. Really speaks to the quality of rust tools that I even got there in the end.

I don't appear to have rights to merge and I'm almost certain the pipeline doesn't deploy to crates.io. Do you know how to proceed?

@johanhelsing
Copy link
Collaborator

@gschup publishes manually. I have merge rights, but I'll wait a bit for him, and maybe his review, since he needs to do the publishing anyway.

@johanhelsing
Copy link
Collaborator

I think this will need some extra changes when gschup/ggrs#82 is merged

@johanhelsing johanhelsing mentioned this pull request Dec 15, 2024
1 task
@johanhelsing
Copy link
Collaborator

@haihala I pushed some fixes for latest ggrs changes in #114

@gschup gschup merged commit f70a56d into gschup:main Dec 19, 2024
1 check passed
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.

3 participants