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

chore(): Bevy 0.11 migration #45

Merged
merged 24 commits into from
Jul 28, 2023
Merged

chore(): Bevy 0.11 migration #45

merged 24 commits into from
Jul 28, 2023

Conversation

kaosat-dev
Copy link
Contributor

@kaosat-dev kaosat-dev commented Jul 17, 2023

Hi !
Started working on the migration to Bevy 0.11 , not quite done yet :)

@hafiidz
Copy link
Contributor

hafiidz commented Jul 18, 2023

Looking forward to this. Not sure how to help, I made an attempt to do the same recently but stuck at a few errors that I couldn't really understand.

Sharing the link here for "reference" https://github.com/hafiidz/bevy_proto/tree/upgrade_bevy_0_11

@MrGVSV
Copy link
Owner

MrGVSV commented Jul 18, 2023

Thanks for helping out! Could you put this in draft mode until you’re ready for review?

@kaosat-dev kaosat-dev marked this pull request as draft July 18, 2023 09:03
@kaosat-dev
Copy link
Contributor Author

Thanks for helping out! Could you put this in draft mode until you’re ready for review?

Sure ! Sorry about that, it is now in draft mode

@kaosat-dev
Copy link
Contributor Author

kaosat-dev commented Jul 18, 2023

Looking forward to this. Not sure how to help, I made an attempt to do the same recently but stuck at a few errors that I couldn't really understand.

Sharing the link here for "reference" https://github.com/hafiidz/bevy_proto/tree/upgrade_bevy_0_11

Wow , you have already done pretty much all the work !
Perhaps it would make more sense that you create a PR and we can help to solve the remaining issues ? Or I can continue with this PR

@hafiidz
Copy link
Contributor

hafiidz commented Jul 19, 2023

I think we can just continue here. When I did mine it wasn't with the idea to publish it, so I didn't pay attention to be sticking to proper formatting. Just doing things as I learn from error reports. Maybe you can cherry pick changes as necessary. I'm not sure how to add changes here though. Should I send a PR to your branch separately? Based on brief Google search, it seems to show only the PR requester and repo owner can make changes here.

@MrGVSV
Copy link
Owner

MrGVSV commented Jul 19, 2023

Should I send a PR to your branch separately? Based on brief Google search, it seems to show only the PR requester and repo owner can make changes here.

Yeah generally in these cases you'd make a PR to their branch.

I'll also try to help out with the migration if I find time this week.

@hafiidz
Copy link
Contributor

hafiidz commented Jul 20, 2023

Added a PR kaosat-dev#2 to @kaosat-dev v0.11 draft branch

@kaosat-dev
Copy link
Contributor Author

Added a PR kaosat-dev#2 to @kaosat-dev v0.11 draft branch

Thanks a lot ! Went over the changes, and it looks good and matches the needed changes outlined in the migration guide !

@kaosat-dev
Copy link
Contributor Author

@MrGVSV & @hafiidz Sorry for the delay, I was swamped this week, I will finalize this PR tomorrow :)

@hafiidz
Copy link
Contributor

hafiidz commented Jul 22, 2023

@MrGVSV & @hafiidz Sorry for the delay, I was swamped this week, I will finalize this PR tomorrow :)

No worries, all good. Take your time, it's volunteer work anyway. I am just taking opportunity to use Rust in actual project and contributing back to the community since I love using this bevy add on in my personal project.

@MrGVSV
Copy link
Owner

MrGVSV commented Jul 22, 2023

@MrGVSV & @hafiidz Sorry for the delay, I was swamped this week, I will finalize this PR tomorrow :)

No worries, all good. Take your time, it's volunteer work anyway. I am just taking opportunity to use Rust in actual project and contributing back to the community since I love using this bevy add on in my personal project.

Agreed, don't feel rushed to do anything. Like @hafiidz said, this is all on a volunteer basis. You both are already doing more than enough by even taking the time to help out, so thank you! 😄

@kaosat-dev
Copy link
Contributor Author

@MrGVSV & @hafiidz thanks for the kind words, also glad to help !
There are a few remaining issues (all within bevy_impls) that I have not been able to figure out :
conflicting implementations of trait 'FromReflect' for type 'EnvironmentMapLightInput' (and similarly for a few others), it seems to be macro related, but even after a bit of googling, I have not found a way to resolve those issues.
(I am guessing these where the ones you mentionned @hafiidz ?).

@MrGVSV
Copy link
Owner

MrGVSV commented Jul 22, 2023

@MrGVSV & @hafiidz thanks for the kind words, also glad to help ! There are a few remaining issues (all within bevy_impls) that I have not been able to figure out : conflicting implementations of trait 'FromReflect' for type 'EnvironmentMapLightInput' (and similarly for a few others), it seems to be macro related, but even after a bit of googling, I have not found a way to resolve those issues. (I am guessing these where the ones you mentionned @hafiidz ?).

No worries, taking a look now!

@MrGVSV
Copy link
Owner

MrGVSV commented Jul 22, 2023

Okay here's a pretty major problem: generic types that require an input.

Currently, the macro will generate an input that looks like this:

#[derive(Schematic)]
struct MyStruct<T, U: Asset> {
  foo: T,
  #[schematic(asset)]
  bar: Handle<U>
}

// Generates:
#[derive(Reflect)]
struct MyStructInput<T> {
  foo: T,
  #[reflect(ignore, default)]
  __phantom_ty__: ::core::marker::PhantomData<fn() -> ( T, U )>,
}

By adding the PhantomData, we're able to easily support any generic type without being forced to parse each field's type and attributes. Unfortunately, PhantomData does not implement FromReflect, so we get a compile error.

bevyengine/bevy#9046 would hopefully fix this, but I think it won't be until Bevy 0.12.

In the meantime, I think we might be able to solve this by adding a temporary attribute to indicate which generics are needed on the input (if one is to be generated):

// Struct where attribute needed:
#[derive(Schematic)]
#[schematic(input(generics(T)))] // We don't need `U`
struct MyStruct<T, U: Asset> {
  foo: T,
  #[schematic(asset)]
  bar: Handle<U>
}

// Struct where no attribute needed:
#[derive(Schematic)]
struct AnotherStruct<T> {
  foo: T,
}

Thoughts on this?

@hafiidz
Copy link
Contributor

hafiidz commented Jul 23, 2023

@MrGVSV & @hafiidz thanks for the kind words, also glad to help ! There are a few remaining issues (all within bevy_impls) that I have not been able to figure out : conflicting implementations of trait 'FromReflect' for type 'EnvironmentMapLightInput' (and similarly for a few others), it seems to be macro related, but even after a bit of googling, I have not found a way to resolve those issues. (I am guessing these where the ones you mentionned @hafiidz ?).

Haha, yup. A lot of Rust macros really felt like magic to me still.

@MrGVSV, unfortunately, I don't have a firm grasp on Rust/Bevy yet to give a strong opinion. It does looks good & especially simple when no attribute needed.

@hafiidz
Copy link
Contributor

hafiidz commented Jul 24, 2023

Not sure if this is the right place, but I think the bevy dependency might need to be more specific, i.e. only compatible with version 0.11.x

The following line in, might need changes from >= to = or to specify via * or ^ as per semver. I recently tried to create a new simple showcase for another issue (#44) with bevy 0.10 and bevy_proto 0.10 but this result in version conflict since cargo is trying to use bevy 0.11 on top of bevy 0.10 specified.

bevy = { version = ">=0.11.0", default-features = false, features = ["bevy_asset"] }

image

@MrGVSV
Copy link
Owner

MrGVSV commented Jul 28, 2023

Okay I think I have it fixed. Turns out the issue was with doing #[reflect(ignore, default)]. That pattern must have broke in 0.11 as it was adding the FromReflect bound even though it's not needed. Removing default allows it to work as normal!

@MrGVSV
Copy link
Owner

MrGVSV commented Jul 28, 2023

Could one or both of you test the branch now? Hopefully it should work in your own projects again!

@kaosat-dev
Copy link
Contributor Author

Could one or both of you test the branch now? Hopefully it should work in your own projects again!

Thanks ! I will take it for a spin now :)

@kaosat-dev
Copy link
Contributor Author

Okay I think I have it fixed. Turns out the issue was with doing #[reflect(ignore, default)]. That pattern must have broke in 0.11 as it was adding the FromReflect bound even though it's not needed. Removing default allows it to work as normal!

ok, wow, so was it a dual issue ? Ie the one with the generic types and the one with defaults ?

@kaosat-dev
Copy link
Contributor Author

Okay here's a pretty major problem: generic types that require an input.

Currently, the macro will generate an input that looks like this:

#[derive(Schematic)]
struct MyStruct<T, U: Asset> {
  foo: T,
  #[schematic(asset)]
  bar: Handle<U>
}

// Generates:
#[derive(Reflect)]
struct MyStructInput<T> {
  foo: T,
  #[reflect(ignore, default)]
  __phantom_ty__: ::core::marker::PhantomData<fn() -> ( T, U )>,
}

By adding the PhantomData, we're able to easily support any generic type without being forced to parse each field's type and attributes. Unfortunately, PhantomData does not implement FromReflect, so we get a compile error.

bevyengine/bevy#9046 would hopefully fix this, but I think it won't be until Bevy 0.12.

In the meantime, I think we might be able to solve this by adding a temporary attribute to indicate which generics are needed on the input (if one is to be generated):

// Struct where attribute needed:
#[derive(Schematic)]
#[schematic(input(generics(T)))] // We don't need `U`
struct MyStruct<T, U: Asset> {
  foo: T,
  #[schematic(asset)]
  bar: Handle<U>
}

// Struct where no attribute needed:
#[derive(Schematic)]
struct AnotherStruct<T> {
  foo: T,
}

Thoughts on this?

Same as @hafiidz , my understanding of Rust & Bevy is not yet good enough to make an informed decision, (PhantomData for example still goes a bit over my head :D ) but it seems like a good stop gap solution ?

@kaosat-dev
Copy link
Contributor Author

kaosat-dev commented Jul 28, 2023

@MrGVSV tested out the branch with your changes, it all works great ! (tried out all examples from the repo+ one of my own) well done 👍 !!

@hafiidz
Copy link
Contributor

hafiidz commented Jul 28, 2023

Could one or both of you test the branch now? Hopefully it should work in your own projects again!

My personal project might take a day to complete migration, will update tomorrow.

At the same time, I have just performed a quick simple project to test, it seems to be working great. However, there is a separate issue with button bundle that might need your help to see later, that still remain persistent even with bevy 0.11 (#44).

@hafiidz
Copy link
Contributor

hafiidz commented Jul 28, 2023

Just take the time to complete the migration of my personal project, much faster than expected. It is running really well now.

Thank you very much! Awesome work @MrGVSV.

@hafiidz
Copy link
Contributor

hafiidz commented Jul 28, 2023

Everything seems to be working well now. The error with button bundle is resolved.

@MrGVSV MrGVSV marked this pull request as ready for review July 28, 2023 23:22
@MrGVSV
Copy link
Owner

MrGVSV commented Jul 28, 2023

Awesome, I'm going to go ahead and merge this then! Thank you both for your help!

@MrGVSV MrGVSV merged commit 5e721b6 into MrGVSV:main Jul 28, 2023
2 checks 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