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.

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

Update serialize flag for bevy_ecs #13740

Merged
merged 5 commits into from
Jun 10, 2024
Merged

Update serialize flag for bevy_ecs #13740

merged 5 commits into from
Jun 10, 2024

Conversation

cBournhonesque
Copy link
Contributor

@cBournhonesque cBournhonesque commented Jun 7, 2024 •
edited
Loading

Objective

There were some issues with the serialize feature:

  • bevy_app had a serialize feature and a dependency on serde even there is no usage of serde at all inside bevy_app
  • the bevy_app/serialize feature enabled bevy_ecs/serde, which is strange
  • bevy_internal/serialize did not enable bevy_app/serialize so there was no way of serializing an Entity in bevy 0.14

Solution

  • Remove serde and bevy_app/serialize
  • Add a serialize flag on bevy_ecs that enables serde
  • bevy_internal/serialize now enables bevy_ecs/serialize

@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Jun 7, 2024
@alice-i-cecile
Copy link
Member

bevy_internal/serialize did not enable bevy_app/serialize so there was no way of serializing an Entity in bevy 0.14

Because features are additive, you can actually get this by pulling a direct dependency on bevy_ecs and enabling the feature there. But this should still be fixed!

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events labels Jun 7, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 7, 2024

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

@alice-i-cecile alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Jun 7, 2024
@alice-i-cecile
Copy link
Member

CI seems very confused about your PR: I don't understand either the failures or the warning about missing examples.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 8, 2024

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

@cBournhonesque
Copy link
Contributor Author

@alice-i-cecile should be good now :)

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 10, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 10, 2024
Merged via the queue into bevyengine:main with commit 93f3432 Jun 10, 2024
32 checks passed
mockersf pushed a commit that referenced this pull request Jun 10, 2024
@cBournhonesque @mockersf
Update serialize flag for bevy_ecs (#13740)
effbcdf 
# Objective

There were some issues with the `serialize` feature:
- `bevy_app` had a `serialize` feature and a dependency on `serde` even
there is no usage of serde at all inside `bevy_app`
- the `bevy_app/serialize` feature enabled `bevy_ecs/serde`, which is
strange
- `bevy_internal/serialize` did not enable `bevy_app/serialize` so there
was no way of serializing an Entity in bevy 0.14

## Solution

- Remove `serde` and `bevy_app/serialize` 
- Add a `serialize` flag on `bevy_ecs` that enables `serde`
- ` bevy_internal/serialize` now enables `bevy_ecs/serialize`
@knutsoned
Copy link

Just tested this with leafwing-input-manager and rc.4. The bevy_ecs serde flag hack now causes an error. Removing it seems to fix the build error.

This was referenced Jul 1, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NiseVoid NiseVoid NiseVoid approved these changes

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

@james7132 james7132 Awaiting requested review from james7132

@mockersf mockersf Awaiting requested review from mockersf

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior 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
0.14
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@cBournhonesque @alice-i-cecile @knutsoned @NiseVoid