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

[Merged by Bors] - bevy_scene: Replace root list with struct #6354

Closed
wants to merge 2 commits into from

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented Oct 24, 2022

Objective

Scenes are currently represented as a list of entities. This is all we need currently, but we may want to add more data to this format in the future (metadata, asset lists, etc.).

It would be nice to update the format in preparation of possible future changes. Doing so now (i.e., before 0.9) could mean reduced1 breakage for things added in 0.10.

Solution

Made the scene root a struct rather than a list.

(
  entities: [
    // Entity data here...
  ]
)

Changelog

  • The scene format now puts the entity list in a newly added entities field, rather than having it be the root object

Migration Guide

The scene file format now uses a struct as the root object rather than a list of entities. The list of entities is now found in the entities field of this struct.

// OLD
[
  (
    entity: 0,
    components: [
      // Components...
    ]
  ),
]

// NEW
(
  entities: [
    (
      entity: 0,
      components: [
        // Components...
      ]
    ),
  ]
)

Footnotes

  1. Obviously, adding features runs the risk of breaking things regardless. But if all features added are for whatever reason optional or well-contained, then users should at least have an easier time updating.

@MrGVSV MrGVSV 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 Oct 24, 2022
@MrGVSV MrGVSV added this to the Bevy 0.9 milestone Oct 24, 2022
Copy link
Member

@Vrixyz Vrixyz left a comment

Choose a reason for hiding this comment

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

I feel like it's a complex implementation of the serialization/deserialization ?
I would have expected to see a SerializedScene struct we can refer to rather than rely on other docs/examples ?

In any way, it's probably helpful to have a custom deserializer, to iterate over assets or entities I guess, but right now it doesn't seem to be the point ?

Other than that (and the minor space nitpick), LGTM 👍 but I'm not comfortable enough with scenes to commit to an approve though, good luck :)

assets/scenes/load_scene_example.scn.ron Outdated Show resolved Hide resolved
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.

I'm on board. I think it's very unlikely that we never want to add other data to scenes, whether that's assets, meta-data, resources or something else entirely.

Co-authored-by: Thierry Berger <contact@thierryberger.com>
@MrGVSV
Copy link
Member Author

MrGVSV commented Oct 24, 2022

I feel like it's a complex implementation of the serialization/deserialization ? I would have expected to see a SerializedScene struct we can refer to rather than rely on other docs/examples ?

Yeah the documentation could be improved to bevy_scene overall. As for a SerializedScene struct, you can sorta look at DynamicScene to get an idea of the layout. Thankfully, with this PR it actually matches up rather than being the value of DynamicScene::entities.

In any way, it's probably helpful to have a custom deserializer, to iterate over assets or entities I guess, but right now it doesn't seem to be the point ?

The reason for the custom deserializer is we need access to the TypeRegistry (we use DeserializeSeed instead of Deserialize to achieve this). So we can't just derive Serialize/Deserialize unfortunately.

@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 Oct 24, 2022
Copy link
Contributor

@jakobhellermann jakobhellermann left a comment

Choose a reason for hiding this comment

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

I agree with the decision to change the scene schema, it is more understandable and easier to extend.
It's unfortunate that we can't just derive the serialization code but that is nothing new. I don't exactly understand how the #[seride(field_identifier)] stuff works but other that that the implementation looks good.

Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

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

I haven't written serde code in a long time, but nothing stands out as bad 👍

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Oct 24, 2022
# Objective

Scenes are currently represented as a list of entities. This is all we need currently, but we may want to add more data to this format in the future (metadata, asset lists, etc.). 

It would be nice to update the format in preparation of possible future changes. Doing so now (i.e., before 0.9) could mean reduced[^1] breakage for things added in 0.10.

[^1]: Obviously, adding features runs the risk of breaking things regardless. But if all features added are for whatever reason optional or well-contained, then users should at least have an easier time updating.

## Solution

Made the scene root a struct rather than a list.

```rust
(
  entities: [
    // Entity data here...
  ]
)
```

---

## Changelog

* The scene format now puts the entity list in a newly added `entities` field, rather than having it be the root object

## Migration Guide

The scene file format now uses a struct as the root object rather than a list of entities. The list of entities is now found in the `entities` field of this struct.

```rust
// OLD
[
  (
    entity: 0,
    components: [
      // Components...
    ]
  ),
]

// NEW
(
  entities: [
    (
      entity: 0,
      components: [
        // Components...
      ]
    ),
  ]
)
```


Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
@bors bors bot changed the title bevy_scene: Replace root list with struct [Merged by Bors] - bevy_scene: Replace root list with struct Oct 24, 2022
@bors bors bot closed this Oct 24, 2022
@MrGVSV MrGVSV deleted the scene-root branch October 24, 2022 21:39
@ickk ickk added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Oct 25, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

Scenes are currently represented as a list of entities. This is all we need currently, but we may want to add more data to this format in the future (metadata, asset lists, etc.). 

It would be nice to update the format in preparation of possible future changes. Doing so now (i.e., before 0.9) could mean reduced[^1] breakage for things added in 0.10.

[^1]: Obviously, adding features runs the risk of breaking things regardless. But if all features added are for whatever reason optional or well-contained, then users should at least have an easier time updating.

## Solution

Made the scene root a struct rather than a list.

```rust
(
  entities: [
    // Entity data here...
  ]
)
```

---

## Changelog

* The scene format now puts the entity list in a newly added `entities` field, rather than having it be the root object

## Migration Guide

The scene file format now uses a struct as the root object rather than a list of entities. The list of entities is now found in the `entities` field of this struct.

```rust
// OLD
[
  (
    entity: 0,
    components: [
      // Components...
    ]
  ),
]

// NEW
(
  entities: [
    (
      entity: 0,
      components: [
        // Components...
      ]
    ),
  ]
)
```


Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
Pietrek14 pushed a commit to Pietrek14/bevy that referenced this pull request Dec 17, 2022
# Objective

Scenes are currently represented as a list of entities. This is all we need currently, but we may want to add more data to this format in the future (metadata, asset lists, etc.). 

It would be nice to update the format in preparation of possible future changes. Doing so now (i.e., before 0.9) could mean reduced[^1] breakage for things added in 0.10.

[^1]: Obviously, adding features runs the risk of breaking things regardless. But if all features added are for whatever reason optional or well-contained, then users should at least have an easier time updating.

## Solution

Made the scene root a struct rather than a list.

```rust
(
  entities: [
    // Entity data here...
  ]
)
```

---

## Changelog

* The scene format now puts the entity list in a newly added `entities` field, rather than having it be the root object

## Migration Guide

The scene file format now uses a struct as the root object rather than a list of entities. The list of entities is now found in the `entities` field of this struct.

```rust
// OLD
[
  (
    entity: 0,
    components: [
      // Components...
    ]
  ),
]

// NEW
(
  entities: [
    (
      entity: 0,
      components: [
        // Components...
      ]
    ),
  ]
)
```


Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

Scenes are currently represented as a list of entities. This is all we need currently, but we may want to add more data to this format in the future (metadata, asset lists, etc.). 

It would be nice to update the format in preparation of possible future changes. Doing so now (i.e., before 0.9) could mean reduced[^1] breakage for things added in 0.10.

[^1]: Obviously, adding features runs the risk of breaking things regardless. But if all features added are for whatever reason optional or well-contained, then users should at least have an easier time updating.

## Solution

Made the scene root a struct rather than a list.

```rust
(
  entities: [
    // Entity data here...
  ]
)
```

---

## Changelog

* The scene format now puts the entity list in a newly added `entities` field, rather than having it be the root object

## Migration Guide

The scene file format now uses a struct as the root object rather than a list of entities. The list of entities is now found in the `entities` field of this struct.

```rust
// OLD
[
  (
    entity: 0,
    components: [
      // Components...
    ]
  ),
]

// NEW
(
  entities: [
    (
      entity: 0,
      components: [
        // Components...
      ]
    ),
  ]
)
```


Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
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-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants