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

Add serialize feature to bevy_color #12527

Closed
pablo-lua opened this issue Mar 17, 2024 · 9 comments · Fixed by #12666
Closed

Add serialize feature to bevy_color #12527

pablo-lua opened this issue Mar 17, 2024 · 9 comments · Fixed by #12666
Labels
A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy

Comments

@pablo-lua
Copy link
Contributor

pablo-lua commented Mar 17, 2024

What problem does this solve or what need does it fill?

Make serde optional for users. If they don't want, they not require it

What solution would you like?

Create the feature serialize for bevy_color and remove serde::Serialize and Deserialize bound from trait StandardColor (or recreate if feature is enabled)

What alternative(s) have you considered?

Leave it as it is

Additional context

Notice that bevy_render depends on bevy_color, and as bevy_render doesn't flag serde by now, bevy_color can't too (unless of course we add the feature by default in bevy_render importing of bevy_color).

@pablo-lua pablo-lua added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Mar 17, 2024
@alice-i-cecile alice-i-cecile added the D-Trivial Nice and easy! A great choice to get started with Bevy label Mar 17, 2024
@alice-i-cecile
Copy link
Member

We should add a serde flag for both crates, but there's no need to block. We can add it to bevy_color first, and just enable it during import initially.

@mghildiy
Copy link
Contributor

I may give it a try. Please let me know.

@pablo-lua
Copy link
Contributor Author

I may give it a try. Please let me know.

Feel free to solve this :D

@mghildiy
Copy link
Contributor

So to summarize my understaning:

  • bevy_color is a crate for which I need to create a new feature 'serialize'
  • remove serde::Serialize and Deserialize from StandardColor(a traint defined in bevy_color)
  • then use this new feature 'serialize' for StandardColor if enabled by user

Actually I am java pogrammer by experience and have just started learning Rust, so want to ensure things are clear to me.
Please bear with me.

@alice-i-cecile
Copy link
Member

Yep: you'll also want to make sure that the serialize feature flag in bevy_color is toggled on if the corresponding feature on bevy is enabled.

@pablo-lua
Copy link
Contributor Author

You can, too, have a look on prior work with similar tasks if that help: #11188
About the thing with StandardColor, that can be strange in design decision: We can't have type bounds being configurable by feature AFAIK, so you'll have to literally recreate the struct if feature is on
something like:

#[cfg(feature = "serialize")]
struct StandardColor 
where // . . .
{
 // . . .
}

#[cfg(not(feature = "serialize"))]
struct StandardColor 
where // . . .
{
// . . .
}

Thats probably the gist of it.

@alice-i-cecile
Copy link
Member

I think we should just remove the Serialize + Deserialize bounds from StandardColor: I don't think the complexity is worth it for an internal helper.

@mghildiy
Copy link
Contributor

PR

@mghildiy
Copy link
Contributor

Build is failing due to some IO error. Is it down to code changes?

github-merge-queue bot pushed a commit that referenced this issue Mar 24, 2024
# Objective

- Add serialize feature to bevy_color
- "Fixes #12527".

## Solution

- Added feature for serialization

---

## Changelog

- Serde serialization is now optional, with flag 'serialize'

## Migration Guide

- If user wants color data structures to be serializable, then
application needs to be build with flag 'serialize'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use 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.

3 participants