Skip to content

[Merged by Bors] - [Fixes #6030] Bevy scene optional serde #6076

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 32 commits into from
Closed

[Merged by Bors] - [Fixes #6030] Bevy scene optional serde #6076

wants to merge 32 commits into from

Conversation

LinusKall
Copy link
Contributor

Objective

Fixes #6030, making serde optional.

Solution

This was solved by making a serialize feature that can activate serde, which is now optional.

When serialize is deactivated, the Plugin implementation for ScenePlugin does nothing.

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Scenes Serialized ECS data stored on the disk labels Sep 24, 2022
@alice-i-cecile
Copy link
Member

The basic strategy LGTM here :) What do you need to do still (other than resolve merge conflicts)?

afonsolage
afonsolage reviewed Sep 25, 2022
View reviewed changes
@LinusKall
Copy link
Contributor Author

LinusKall commented Sep 26, 2022 •
edited
Loading

@alice-i-cecile

The basic strategy LGTM here :) What do you need to do still (other than resolve merge conflicts)?

I'm a little unsure, but I believe I should:

  1. Fix all issues that occur when running cargo run -p ci,
  2. make sure there are no warnings, and
  3. make sure it's possible to run cargo run --example scene.

Am I on the right track? 😅

Edit: It seems like I'm missing something else, because it feels like the serde feature is not enabled. Should I activate it somewhere?

@alice-i-cecile
Copy link
Member

Yep, seems like a solid checklist.

It seems like I'm missing something else, because it feels like the serde feature is not enabled. Should I activate it somewhere?

You forgot to add the serde feature to the default features for bevy_scene :)

@LinusKall
Copy link
Contributor Author

You forgot to add the serde feature to the default features for bevy_scene :)

Aaah of course, thank you very much! 😄

@LinusKall
Copy link
Contributor Author

I fixed most of the issue that occurred when running cargo run -p ci,
and there are no warnings or errors when running cargo run --example scene either.

The checks here on GitHub also seem to check out.

Is there anything else I should look at? :)

@alice-i-cecile
Copy link
Member

I think we're good now :) Can you move this PR out of draft status?

@alice-i-cecile alice-i-cecile requested review from afonsolage and mockersf and removed request for afonsolage September 26, 2022 15:48
@LinusKall
Copy link
Contributor Author

@alice-i-cecile

Yep! :)

@LinusKall LinusKall marked this pull request as ready for review September 26, 2022 15:49
MinerSebas
MinerSebas reviewed Sep 26, 2022
View reviewed changes
mockersf
mockersf reviewed Sep 26, 2022
View reviewed changes
@alice-i-cecile
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Nov 2, 2022
@bors
Try #6076:
002712f
@alice-i-cecile
Copy link
Member

(Talk to Cart about being added to the team on Github to get the basic tier of bors and issue labelling permissions :D)

alice-i-cecile
alice-i-cecile approved these changes Nov 2, 2022
View reviewed changes
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.

Alright, I'm happy with this now. I think it fills the role of "let users use bevy_scene with their own serialization libraries".

@LinusKall
Copy link
Contributor Author

Thank you, Alice! 😄

But lastly, the Dependencies / check-bans (pull request) failure does not seem to give any useful information?

I tried to run cargo deny check bans locally and got no errors. Would you have any tips?

@bors
Copy link
Contributor

bors bot commented Nov 2, 2022

try

Build succeeded:

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Nov 2, 2022 •
edited
Loading

It's not your PRs fault: we have a duplicate dependency in tree that we need to clean up. I think it might be spurious for old PRs due to weird git branching.

@LinusKall
Copy link
Contributor Author

Okay!

Just out of curiosity, do you know what weird git branching would be in this case?

@alice-i-cecile
Copy link
Member

Just out of curiosity, do you know what weird git branching would be in this case?

My suspicion is that older PRs are using a cached version of the dependencies for bevy_audio, generated during a time when our transitive dependencies broke due to semver gnarliness: diwic/alsa-rs#90.

New PRs aren't getting this, but I think the dependencies check in CI isn't eagerly recomputing whatever cache it had, because the PRs themselves haven't done anything that should fix it (the offending release was yanked).

So now that I think about it, not actually git's fault: just weird caching problems.

@LinusKall
Copy link
Contributor Author

That sounds similar to the issues that can occur in the target folder sometimes,

hope that it is as easily resolved as well! 😅

mockersf
mockersf reviewed Nov 11, 2022
View reviewed changes
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Nov 11, 2022
@cart
Copy link
Member

cart commented Nov 14, 2022

bors r+

bors bot pushed a commit that referenced this pull request Nov 14, 2022
@LinusKall
[Fixes #6030] Bevy scene optional serde (#6076)
110c82c 
# Objective

Fixes #6030, making ``serde`` optional.

## Solution

This was solved by making a ``serialize`` feature that can activate ``serde``, which is now optional. 

When ``serialize`` is deactivated, the ``Plugin`` implementation for ``ScenePlugin`` does nothing.


Co-authored-by: Linus Käll <[email protected]>
@bors
Copy link
Contributor

bors bot commented Nov 14, 2022

Build failed (retrying...):

@bors
Copy link
Contributor

bors bot commented Nov 14, 2022

Canceled.

@LinusKall
Copy link
Contributor Author

Sorry @cart, I synchronized the wrong branch...

@cart
Copy link
Member

cart commented Nov 14, 2022

bors r+

bors bot pushed a commit that referenced this pull request Nov 14, 2022
@LinusKall
[Fixes #6030] Bevy scene optional serde (#6076)
af2a199 
# Objective

Fixes #6030, making ``serde`` optional.

## Solution

This was solved by making a ``serialize`` feature that can activate ``serde``, which is now optional. 

When ``serialize`` is deactivated, the ``Plugin`` implementation for ``ScenePlugin`` does nothing.


Co-authored-by: Linus Käll <[email protected]>
@bors
Copy link
Contributor

bors bot commented Nov 14, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title [Fixes #6030] Bevy scene optional serde [Merged by Bors] - [Fixes #6030] Bevy scene optional serde Nov 14, 2022
@bors bors bot closed this Nov 14, 2022
@LinusKall LinusKall deleted the bevy_scene_optional_serde branch November 22, 2022 11:02
taiyoungjang pushed a commit to taiyoungjang/bevy that referenced this pull request Dec 15, 2022
@LinusKall @taiyoungjang
[Fixes bevyengine#6030] Bevy scene optional serde (bevyengine#6076)
a11cd7f 
# Objective

Fixes bevyengine#6030, making ``serde`` optional.

## Solution

This was solved by making a ``serialize`` feature that can activate ``serde``, which is now optional. 

When ``serialize`` is deactivated, the ``Plugin`` implementation for ``ScenePlugin`` does nothing.


Co-authored-by: Linus Käll <[email protected]>
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
@LinusKall @alradish
[Fixes bevyengine#6030] Bevy scene optional serde (bevyengine#6076)
49571cc 
# Objective

Fixes bevyengine#6030, making ``serde`` optional.

## Solution

This was solved by making a ``serialize`` feature that can activate ``serde``, which is now optional. 

When ``serialize`` is deactivated, the ``Plugin`` implementation for ``ScenePlugin`` does nothing.


Co-authored-by: Linus Käll <[email protected]>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@LinusKall @ItsDoot
[Fixes bevyengine#6030] Bevy scene optional serde (bevyengine#6076)
429db0d 
# Objective

Fixes bevyengine#6030, making ``serde`` optional.

## Solution

This was solved by making a ``serialize`` feature that can activate ``serde``, which is now optional. 

When ``serialize`` is deactivated, the ``Plugin`` implementation for ``ScenePlugin`` does nothing.


Co-authored-by: Linus Käll <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afonsolage afonsolage afonsolage left review comments

@cart cart cart left review comments

@MinerSebas MinerSebas MinerSebas left review comments

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@mockersf mockersf mockersf approved these changes

Assignees
No one assigned
Labels
A-Scenes Serialized ECS data stored on the disk C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make bevy_scene dependency on serde optional
6 participants
@LinusKall @alice-i-cecile @cart @afonsolage @mockersf @MinerSebas