chore(): Bevy 0.11 migration

[kaosat-dev]

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

[hafiidz]

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]

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

[kaosat-dev]

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]

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]

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]

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]

Added a PR https://github.com/kaosat-dev/bevy_proto/pull/2 to @kaosat-dev v0.11 draft branch

[kaosat-dev]

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]

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

[hafiidz]

@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]

@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]

@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]

@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]

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.

https://github.com/bevyengine/bevy/pull/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]

@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]

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 (https://github.com/MrGVSV/bevy_proto/issues/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.

https://github.com/MrGVSV/bevy_proto/blob/e899c10884ca7e1de07e80c9e2461ec24b7961c8/Cargo.toml#L103

[MrGVSV]

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]

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

[kaosat-dev]

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]

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]

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]

@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 :+1: !!

[hafiidz]

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 (https://github.com/MrGVSV/bevy_proto/issues/44).

[hafiidz]

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]

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

[MrGVSV]

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