Skip to content

Make bevy_scene dependency on serde optional #6030

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

Closed
alice-i-cecile opened this issue Sep 20, 2022 · 9 comments
Closed

Make bevy_scene dependency on serde optional #6030

alice-i-cecile opened this issue Sep 20, 2022 · 9 comments
Labels
A-Scenes Serialized ECS data stored on the disk C-Code-Quality A section of code that is hard to understand or change D-Trivial Nice and easy! A great choice to get started with Bevy

Comments

@alice-i-cecile
Copy link
Member

In bevy_scene, the dependency to serde is not optional and the code is always enabled... but it would be nice to make it optional. maybe another PR 🙂

Originally posted by @mockersf in #6023 (comment)

@alice-i-cecile alice-i-cecile added D-Trivial Nice and easy! A great choice to get started with Bevy C-Code-Quality A section of code that is hard to understand or change A-Scenes Serialized ECS data stored on the disk labels Sep 20, 2022
@LinusKall
Copy link
Contributor

LinusKall commented Sep 20, 2022

I've never contributed before, but I would like to give this a shot!

@LinusKall
Copy link
Contributor

Would it be a good idea to make custom SceneSerializer and SceneDeserializer implementations in a new module or make variants within the serde.rs module?

@alice-i-cecile
Copy link
Member Author

Thanks! For this particular issue I think we can probably do a very scoped change :) This line is the suspicious one:

serde = { version = "1.0", features = ["derive"] }

Here's the reference page I use when mucking about with dependencies.

@LinusKall
Copy link
Contributor

Thank you!

I've changed serde to be optional, but is that enough? Wouldn't it also be necessary to define a feature that we can use to "activate" serde again?

I'm also uncertain because wouldn't the case of this serde feature being "deactivated" have to be handled? That is why I'm wondering about custom implementations.

Perhaps I'm just missing something? 😅

@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented Sep 20, 2022

Oh right! You need to add a serde feature, then add it to the top-level bevy feature. Check out what was done in these commits: https://github.com/bevyengine/bevy/pull/6023/commits

I'm also uncertain because wouldn't the case of this serde feature being "deactivated" have to be handled? That is why I'm wondering about custom implementations.

If the serde feature is deactivated, users don't care about implementing serde's traits one way or another :)

@LinusKall
Copy link
Contributor

I see, thank you for your patience!

I will try it out 😄

@LinusKall
Copy link
Contributor

Hi,

I've taken a look and it seems like most things could be solved by using #[cfg(feature = "serialize)].

The only thing that needs to be handled differently is the AssetLoader implementation for the SceneLoader struct, this is because it uses the SceneDeserializer struct.

I'm thinking that perhaps this implementation and the current Plugin implementation for the ScenePlugin could be connected to the serialize feature.

Then I could make a "blank" Plugin implementation that does nothing when the serialize feature isn't activated.

Would this be reasonable?

@alice-i-cecile
Copy link
Member Author

Yep, that seems basically correct. Feel free to open PRs early (and stick them in draft mode); it's often easier to figure out the exact technical details when there's a change set in front of you :)

@LinusKall
Copy link
Contributor

LinusKall commented Sep 21, 2022

Okay, I'll try opening a PR tomorrow! :)

Edit: sorry, I meant on friday

bors bot pushed a commit that referenced this issue Nov 14, 2022
# 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 bors bot closed this as completed in af2a199 Nov 14, 2022
taiyoungjang pushed a commit to taiyoungjang/bevy that referenced this issue Dec 15, 2022
# 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 issue Jan 22, 2023
# 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 issue Feb 1, 2023
# 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
Labels
A-Scenes Serialized ECS data stored on the disk C-Code-Quality A section of code that is hard to understand or change D-Trivial Nice and easy! A great choice to get started with Bevy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants