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

Bevy 0.15 support #138

Merged
merged 7 commits into from
Dec 7, 2024
Merged

Bevy 0.15 support #138

merged 7 commits into from
Dec 7, 2024

Conversation

rendaoer
Copy link
Contributor

@rendaoer rendaoer commented Dec 2, 2024

The main content is to support Bevy 0.15, switch from Style to Node, use TextColor for Text, etc., Migrating from the interpolation library to Bevy, modify the Bundle part of the example, check and optimize the code and documents

@rendaoer rendaoer mentioned this pull request Dec 2, 2024
Copy link

@mnmaita mnmaita left a comment

Choose a reason for hiding this comment

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

Really looking forward to get this one merged, great work! Leaving a few nits, suggestions and change requests. Thanks a lot!

src/plugin.rs Outdated
Comment on lines 32 to 36
/// [`Transform`]: https://docs.rs/bevy/0.12.0/bevy/transform/components/struct.Transform.html
/// [`Text`]: https://docs.rs/bevy/0.12.0/bevy/text/struct.Text.html
/// [`Style`]: https://docs.rs/bevy/0.12.0/bevy/ui/struct.Style.html
/// [`Node`]: https://docs.rs/bevy/0.15.0/bevy/ui/struct.Node.html
/// [`Sprite`]: https://docs.rs/bevy/0.12.0/bevy/sprite/struct.Sprite.html
/// [`ColorMaterial`]: https://docs.rs/bevy/0.12.0/bevy/sprite/struct.ColorMaterial.html
Copy link

Choose a reason for hiding this comment

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

Maybe these links should point to latest docs? And also I think Text should be swapped for TextColor here too.

Copy link
Owner

Choose a reason for hiding this comment

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

No they shouldn't. If they point to latest, then when you look at the docs for version N you get a link to the latest version, which is only N until N+1 is published, and will after that forever be wrong.

README.md Outdated Show resolved Hide resolved
@@ -168,7 +165,7 @@ Asset animation always requires the `bevy_asset` feature.

Copy link

Choose a reason for hiding this comment

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

I can't comment above this but in L155 Style should be replaced with Node and in L157 Text with TextColor probably?

examples/menu.rs Outdated Show resolved Hide resolved
examples/menu.rs Outdated Show resolved Hide resolved
examples/sequence.rs Outdated Show resolved Hide resolved
@@ -446,7 +446,7 @@ impl<T: 'static> Tween<T> {
/// # Example
/// ```
/// # use bevy_tweening::{lens::*, *};
/// # use bevy::math::*;
/// # use bevy::math::{*,curve::EaseFunction};
Copy link

Choose a reason for hiding this comment

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

Could you please format all these doc changes (i.e. adding a space after each comma, etc.)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@rendaoer
Copy link
Contributor Author

rendaoer commented Dec 2, 2024

@mnmaita Thank you for your code review

Copy link
Owner

@djeedai djeedai 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 the comprehensive PR. I'll try to review in details when I have more time, likely later this week or on the week-end, but it seems it's chewing a lot. There's the upgrade, and then there's unrelated things like adding a prelude (I think?), changing the easing functions, etc. In general I'd prefer to get standalone PRs for each bit, that not only makes it easier to review, but also makes it easier to bisect bugs in the future and potentially revert changes.

@rendaoer
Copy link
Contributor Author

rendaoer commented Dec 3, 2024

OK, I'll split them later.

@rendaoer
Copy link
Contributor Author

rendaoer commented Dec 3, 2024

But isn't EaseFunction considered included in the range of 0.15?

@rendaoer
Copy link
Contributor Author

rendaoer commented Dec 3, 2024

In places like this,

use bevy::prelude::*;
use bevy_tweening::{lens::*, *};

bevy::prelude::* exports bevy's EaseFunction, and then bevy_tweening::{lens::*, *} also exports interpolation's EaseFunction, and this happens in many places, and in the documentation, making it very cumbersome to modify. And I thought this was part of the migration to 0.15?

@djeedai
Copy link
Owner

djeedai commented Dec 7, 2024

Need to add x11 to Bevy features otherwise Linux won't build (winit needs either x11 or wayland; see rust-windowing/winit#3174).

@djeedai djeedai merged commit 0125aa3 into djeedai:main Dec 7, 2024
7 of 8 checks passed
@djeedai
Copy link
Owner

djeedai commented Dec 7, 2024

Thanks @rendaoer and @mnmaita for this. I merged the change now, I want to move ahead and release.

@musjj
Copy link

musjj commented Dec 7, 2024

@rendaoer
Are you planning to open another PR for the prelude module?

@djeedai
Copy link
Owner

djeedai commented Dec 7, 2024

@rendaoer Are you planning to open another PR for the prelude module?

This PR removed the dependency to interpolation so there's no issue with the prelude, is there @musjj ?

@musjj
Copy link

musjj commented Dec 7, 2024

Yeah, but it's more idiomatic to have a prelude (use bevy_tweening::{lens::*, *} vs. use bevy_tweening::prelude::*). Your other crate bevy_hanabi also uses a prelude, so I don't see a reason not to have one here too.

@djeedai
Copy link
Owner

djeedai commented Dec 7, 2024

@musjj ah sorry I thought you were talking about the symbol conflict on EaseFunction. I don't mind having a prelude, although the crate is fairly small so this is not really needed I think.

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.

4 participants