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

[Merged by Bors] - 0.7 migration guide #332

Closed
wants to merge 22 commits into from

Conversation

IceSentry
Copy link
Contributor

@IceSentry IceSentry commented Apr 9, 2022

This is currently an empty PR. I'll be adding new sections tonight and over the weekend, Feel free to suggest anything missing.

If anyone wants to help I will gladly accept PR to my branch on my fork at https://github.com/IceSentry/bevy-website/tree/0.7-migration-guide

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Please don't make me migrate more short codes T_T

If at all possible, use links for this doc.

@IceSentry
Copy link
Contributor Author

IceSentry commented Apr 9, 2022

Oh, sorry, about the shortcodes, I was just looking at the previous migration guide😅. I wasn't sure what the agreed upon solution was.

@alice-i-cecile
Copy link
Member

Use reference style links, with ` inside of the link :) Make sure to link to specific releases too!

E.g.. [`World`]

[`World`]: https://docs.rs/bevy/0.6.1/bevy/ecs/world/struct.World.html

@ramirezmike
Copy link
Contributor

Here are a couple things I've run into so far that I think are missing

@rparrett
Copy link
Contributor

rparrett commented Apr 9, 2022

I ran into this as well, and had to do some some_translation + Vec3::from(aabb.center) etc which seemed less than ideal. Is that the best we can do when migrating?

Comment on lines 96 to 101
### Fix mul_vec3 tranformation order

<https://github.com/bevyengine/bevy/pull/3811>

Transforms are now consistently applied in the standard scale -> rotate -> translate. This doesn't require any code changes, but it means SpriteBundle will behave as expected when rotating.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### Fix mul_vec3 tranformation order
<https://github.com/bevyengine/bevy/pull/3811>
Transforms are now consistently applied in the standard scale -> rotate -> translate. This doesn't require any code changes, but it means SpriteBundle will behave as expected when rotating.

Would love to hear other opinions on this, but if this isn't actionable, I don't think it needs to be here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it technically affected how things were transformed, so if somebody hacked something on top, they can remove it, but I guess the changelog is enough for this.

@ramirezmike
Copy link
Contributor

I ran into this as well, and had to do some some_translation + Vec3::from(aabb.center) etc which seemed less than ideal. Is that the best we can do when migrating?

Vec3A and Vec3 implement the From trait so you should be able to do

aabb. center. into()

@IceSentry
Copy link
Contributor Author

@ramirezmike is the event being moved something that requires a migration guide? Is it not just stuff already in the prelude?

@alice-i-cecile
Copy link
Member

@ramirezmike is the event being moved something that requires a migration guide? Is it not just stuff already in the prelude?

Users who are importing it directly will have to refactor that.

@IceSentry
Copy link
Contributor Author

So, I originally added the PR links to each migration because I wanted to refer to it while writing it, but should I keep it there for the final migration guide? I'm not sure if it's too much noise, but personally I like keeping those reference to dig a bit deeper in the change without having to manually search for it.

@rparrett
Copy link
Contributor

rparrett commented Apr 12, 2022

I agree. Some of these migration suggestions just skim the surface and users may need to look deeper themselves or otherwise might just appreciate the extra context.

I'm just not sure how to format them, exactly.

@alice-i-cecile
Copy link
Member

Perhaps we could link to the PRs in each heading?

@IceSentry
Copy link
Contributor Author

Alright, I tried that. I'm not sure how I feel about it. It's less clutter, which is nice, but it's a bit less obvious that it links to a github PR

@IceSentry
Copy link
Contributor Author

IceSentry commented Apr 13, 2022

The last one left with no description is bevyengine/bevy#3633 which is the config api stuff, but I don't really understand that API enough to come up with a good migration example.

@IceSentry
Copy link
Contributor Author

If anyone is curious how it looks there's a preview version hosted by cloudflare pages at https://0-7-migration-guide.bevy-website.pages.dev/learn/book/migration-guides/0.6-0.7/

@cart
Copy link
Member

cart commented Apr 13, 2022

Just hopping in now for the first time. Y'all have been productive. Nice work 😄

@cart
Copy link
Member

cart commented Apr 15, 2022

bors r+

bors bot pushed a commit that referenced this pull request Apr 15, 2022
This is currently an empty PR. I'll be adding new sections tonight and over the weekend, Feel free to suggest anything missing.

If anyone wants to help I will gladly accept PR to my branch on my fork at https://github.com/IceSentry/bevy-website/tree/0.7-migration-guide

Co-authored-by: Charles <[email protected]>
@bors
Copy link

bors bot commented Apr 15, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title 0.7 migration guide [Merged by Bors] - 0.7 migration guide Apr 15, 2022
@bors bors bot closed this Apr 15, 2022
@IceSentry IceSentry deleted the 0.7-migration-guide branch April 15, 2022 04:58
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.

7 participants