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

Update to Bevy 0.14 #232

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from
Draft

Update to Bevy 0.14 #232

wants to merge 17 commits into from

Conversation

luca-della-vedova
Copy link
Member

New feature implementation

Implemented feature

As per description. We technically can go up to 0.14 but it is currently stuck on #231 (comment), so this gets us closer (the 0.13 -> 0.14 breakage is minimal, so not a lot of duplicated work).

Implementation description

Will comment on the lines themselves for migrations that were not trivial

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
@luca-della-vedova luca-della-vedova marked this pull request as draft July 25, 2024 05:15
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Copy link
Member Author

@luca-della-vedova luca-della-vedova left a comment

Choose a reason for hiding this comment

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

It is sadly a pickle. Ran into this issue while testing. Closing the camera preview window creates a panic in winit that seems to have been fixed with 0.14.

However, 0.14 migration is still stuck on #231 (comment).

Will let this sit for a while, if 0.14 doesn't get unstuck I would propose temporarily commenting out the Preview button to remove the possible panic

- {pose: {trans: [1973, 633, 2.6]}, kind: {type: point, color: [0.5, 0.5, 1.0, 1.0]}}
- {pose: {trans: [1973, 633, 2.6]}, kind: {type: point, color: [0.5, 0.5, 1.0]}}
Copy link
Member Author

Choose a reason for hiding this comment

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

Lights in the bevy renderer itself don't use the alpha channel (and it's unclear what alpha even means for a light color), hence changed them to be RGB

bevy_mod_outline = "0.7"
bevy_infinite_grid = "0.12"
bevy_gltf_export = { git = "https://github.com/luca-della-vedova/bevy_gltf_export", branch = "bevy_0.13"}
bevy_polyline = { git = "https://github.com/luca-della-vedova/bevy_polyline", branch = "luca/bevy_0.13_panic" }
Copy link
Member Author

Choose a reason for hiding this comment

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

bevy_polyline in 0.13 panics, this is fixed in their 0.14 release but was not backported, once we update we won't need a custom branch anymore.

Comment on lines -264 to +258
Circle {
OffsetCircle {
Copy link
Member Author

Choose a reason for hiding this comment

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

Circle cannot be used anymore since it was added to the prelude as part of the primitive shape support effort.
I renamed this OffsetCircle since it's a circle with a Z offset but any other solution is also OK.

Comment on lines 351 to 353
exposure: Exposure {
ev100: Exposure::EV100_INDOOR,
},
Copy link
Member Author

Choose a reason for hiding this comment

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

As part of the work on lighting and cameras, a new component was added for exposure. I'm defaulting it to indoor exposure and changing the light default accordingly.

Query<((), With<LiftDoormat>)>,
Query<(), With<LiftDoormat>>,
Copy link
Member Author

Choose a reason for hiding this comment

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

This is very interesting, the query was actually wrong (it was querying for a tuple of empty component and a With<> component, while it should have queried for the entity component with a With<> condition).
In 0.13 they made it a syntax error, so this was great

let mut ambient_light = world.get_resource_mut::<AmbientLight>().expect(
"Make sure bevy's PbrPlugin is initialized before the cameras");

ambient_light.brightness = 2.0;
Copy link
Member Author

Choose a reason for hiding this comment

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

Since we made everything a lot darker than default bevy, I also had to reduce the ambient light otherwise its effect would be overwhelming the rendering and everything would look washed out.

Comment on lines 60 to 61
# Disable the manage_clipboard feature, depends on nightly web features
bevy_egui = { version = "0.27", features = ["open_url", "default_fonts", "render"] }
Copy link
Member Author

Choose a reason for hiding this comment

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

Ref vladbat00/bevy_egui#270, manage_clipboard only works on nightly web APIs

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
@luca-della-vedova
Copy link
Member Author

Second block on web, bevyengine/bevy#11278 is only available in 0.14 and the 0.13 behavior regressed with the workaround not really working, will just wait for 0.14 now.

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
@luca-della-vedova luca-della-vedova changed the title Update to Bevy 0.13 Update to Bevy 0.14 Jan 13, 2025
@luca-della-vedova
Copy link
Member Author

Note da1f574 should be reverted, I commented out the line for now since I couldn't find a clean migration strategy. This PR in egui added a new LayerId parameter to the function and we might need to do a bit of refactoring to get it from the UI element.

48d832d is an interesting one that should be investigated and probably reverted / reworked. Without it bevy currently panics, it seems that the system ordering we were enforcing was actually wrong, we were asking for systems to run between visibility propagation and transform propagation (I'm guessing so we can set the pose before transform is propagated). However, in Bevy the transform runs after visibility, so when we ask Bevy to "please run visibility, then our system set then transform", while internally it has a constraint of "run transform then visibility" it creates an unresolvable condition and panics at startup time.
This system ordering, again opposite of what we expect, is present in:

  • 0.14.2 (Version being migrated to in this PR)
  • 0.12.1 (our current version).
  • 0.10 (our previous version)
  • 0.8 Our previous previous version, I'll skip for brevity but I found it all the way going back.

So my current guess is that this condition has always been undefined but it only became a panic in the latest version of Bevy, which is actually great, since we can now fix it.
Again I don't think the fix I pushed is a good one, if I understand the intention behind the system set we only care that our systems run before transform propagation (so we can make sure to set all the Pose components and update bevy's Transform components before they are propagated), but I'm not 100% sure about that.

Furthermore 0a6ec94 should be reverted (specifically, bump to bevy impulse 0.2.1 once it's re-released).
We need a feature in bevy_impulse that was only merged in the 0.0.2 version but this branch will be using 0.2.x. I opened an upstream PR to forward merge 0.0.x to 0.1.x, once it is merged we should do a 0.1.x to 0.2.x, trigger a 0.2.1 release on crates.io and change the dependency to bevy_impulse = 0.2.1. The git dependency has my branch doing this forward port.

Finally, there are a lot of warnings because of deprecated APIs / unused variables. Those are not as critical so I left them until we solve all pending items here.

@mxgrey mxgrey self-assigned this Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants