[Merged by Bors] - bevy_scene: Use map for scene `components` #6345

MrGVSV · 2022-10-23T08:03:58Z

Objective

Currently scenes define components using a list:

[
  (
    entity: 0,
    components: [
      {
        "bevy_transform::components::transform::Transform": (
          translation: (
            x: 0.0,
            y: 0.0,
            z: 0.0
          ),
          rotation: (0.0, 0.0, 0.0, 1.0),
          scale: (
            x: 1.0,
            y: 1.0,
            z: 1.0
          ),
        ),
      },
      {
        "my_crate::Foo": (
          text: "Hello World",
        ),
      },
      {
        "my_crate::Bar": (
          baz: 123,
        ),
      },
    ],
  ),
]

However, this representation has some drawbacks (as pointed out by @Metadorius in this comment):

Increased nesting and more characters (minor effect on overall size)
More importantly, by definition, entities cannot have more than one instance of any given component. Therefore, such data is best stored as a map— where all values are meant to have unique keys.

Solution

Change components to store a map of components rather than a list:

[
  (
    entity: 0,
    components: {
      "bevy_transform::components::transform::Transform": (
        translation: (
          x: 0.0,
          y: 0.0,
          z: 0.0
        ),
        rotation: (0.0, 0.0, 0.0, 1.0),
        scale: (
          x: 1.0,
          y: 1.0,
          z: 1.0
        ),
      ),
      "my_crate::Foo": (
        text: "Hello World",
      ),
      "my_crate::Bar": (
        baz: 123
      ),
    },
  ),
]

Code Representation

This change only affects the scene format itself. DynamicEntity still stores its components as a list. The reason for this is that storing such data as a map is not really needed since:

The "key" of each value is easily found by just calling Reflect::type_name on it
We should be generating such structs using the World itself which upholds the one-component-per-entity rule

One could in theory create manually create a DynamicEntity with duplicate components, but this isn't something I think we should focus on in this PR. DynamicEntity can be broken in other ways (i.e. storing a non-component in the components list), and resolving its issues can be done in a separate PR.

Changelog

The scene format now uses a map to represent the collection of components rather than a list

Migration Guide

The scene format now uses a map to represent the collection of components. Scene files will need to update from the old list format.

Example Code

// OLD
[
  (
    entity: 0,
    components: [
      {
        "bevy_transform::components::transform::Transform": (
          translation: (
            x: 0.0,
            y: 0.0,
            z: 0.0
          ),
          rotation: (0.0, 0.0, 0.0, 1.0),
          scale: (
            x: 1.0,
            y: 1.0,
            z: 1.0
          ),
        ),
      },
      {
        "my_crate::Foo": (
          text: "Hello World",
        ),
      },
      {
        "my_crate::Bar": (
          baz: 123,
        ),
      },
    ],
  ),
]

// NEW
[
  (
    entity: 0,
    components: {
      "bevy_transform::components::transform::Transform": (
        translation: (
          x: 0.0,
          y: 0.0,
          z: 0.0
        ),
        rotation: (0.0, 0.0, 0.0, 1.0),
        scale: (
          x: 1.0,
          y: 1.0,
          z: 1.0
        ),
      ),
      "my_crate::Foo": (
        text: "Hello World",
      ),
      "my_crate::Bar": (
        baz: 123
      ),
    },
  ),
]

alice-i-cecile · 2022-10-27T01:46:16Z

bors r+

@Metadorius

# Objective Currently scenes define components using a list: ```rust [ ( entity: 0, components: [ { "bevy_transform::components::transform::Transform": ( translation: ( x: 0.0, y: 0.0, z: 0.0 ), rotation: (0.0, 0.0, 0.0, 1.0), scale: ( x: 1.0, y: 1.0, z: 1.0 ), ), }, { "my_crate::Foo": ( text: "Hello World", ), }, { "my_crate::Bar": ( baz: 123, ), }, ], ), ] ``` However, this representation has some drawbacks (as pointed out by @Metadorius in [this](#4561 (comment)) comment): 1. Increased nesting and more characters (minor effect on overall size) 2. More importantly, by definition, entities cannot have more than one instance of any given component. Therefore, such data is best stored as a map— where all values are meant to have unique keys. ## Solution Change `components` to store a map of components rather than a list: ```rust [ ( entity: 0, components: { "bevy_transform::components::transform::Transform": ( translation: ( x: 0.0, y: 0.0, z: 0.0 ), rotation: (0.0, 0.0, 0.0, 1.0), scale: ( x: 1.0, y: 1.0, z: 1.0 ), ), "my_crate::Foo": ( text: "Hello World", ), "my_crate::Bar": ( baz: 123 ), }, ), ] ``` #### Code Representation This change only affects the scene format itself. `DynamicEntity` still stores its components as a list. The reason for this is that storing such data as a map is not really needed since: 1. The "key" of each value is easily found by just calling `Reflect::type_name` on it 2. We should be generating such structs using the `World` itself which upholds the one-component-per-entity rule One could in theory create manually create a `DynamicEntity` with duplicate components, but this isn't something I think we should focus on in this PR. `DynamicEntity` can be broken in other ways (i.e. storing a non-component in the components list), and resolving its issues can be done in a separate PR. --- ## Changelog * The scene format now uses a map to represent the collection of components rather than a list ## Migration Guide The scene format now uses a map to represent the collection of components. Scene files will need to update from the old list format. <details> <summary>Example Code</summary> ```rust // OLD [ ( entity: 0, components: [ { "bevy_transform::components::transform::Transform": ( translation: ( x: 0.0, y: 0.0, z: 0.0 ), rotation: (0.0, 0.0, 0.0, 1.0), scale: ( x: 1.0, y: 1.0, z: 1.0 ), ), }, { "my_crate::Foo": ( text: "Hello World", ), }, { "my_crate::Bar": ( baz: 123, ), }, ], ), ] // NEW [ ( entity: 0, components: { "bevy_transform::components::transform::Transform": ( translation: ( x: 0.0, y: 0.0, z: 0.0 ), rotation: (0.0, 0.0, 0.0, 1.0), scale: ( x: 1.0, y: 1.0, z: 1.0 ), ), "my_crate::Foo": ( text: "Hello World", ), "my_crate::Bar": ( baz: 123 ), }, ), ] ``` </details>

bors · 2022-10-27T02:04:52Z

Pull request successfully merged into main.

Build succeeded:

@Metadorius

# Objective Currently scenes define components using a list: ```rust [ ( entity: 0, components: [ { "bevy_transform::components::transform::Transform": ( translation: ( x: 0.0, y: 0.0, z: 0.0 ), rotation: (0.0, 0.0, 0.0, 1.0), scale: ( x: 1.0, y: 1.0, z: 1.0 ), ), }, { "my_crate::Foo": ( text: "Hello World", ), }, { "my_crate::Bar": ( baz: 123, ), }, ], ), ] ``` However, this representation has some drawbacks (as pointed out by @Metadorius in [this](bevyengine#4561 (comment)) comment): 1. Increased nesting and more characters (minor effect on overall size) 2. More importantly, by definition, entities cannot have more than one instance of any given component. Therefore, such data is best stored as a map— where all values are meant to have unique keys. ## Solution Change `components` to store a map of components rather than a list: ```rust [ ( entity: 0, components: { "bevy_transform::components::transform::Transform": ( translation: ( x: 0.0, y: 0.0, z: 0.0 ), rotation: (0.0, 0.0, 0.0, 1.0), scale: ( x: 1.0, y: 1.0, z: 1.0 ), ), "my_crate::Foo": ( text: "Hello World", ), "my_crate::Bar": ( baz: 123 ), }, ), ] ``` #### Code Representation This change only affects the scene format itself. `DynamicEntity` still stores its components as a list. The reason for this is that storing such data as a map is not really needed since: 1. The "key" of each value is easily found by just calling `Reflect::type_name` on it 2. We should be generating such structs using the `World` itself which upholds the one-component-per-entity rule One could in theory create manually create a `DynamicEntity` with duplicate components, but this isn't something I think we should focus on in this PR. `DynamicEntity` can be broken in other ways (i.e. storing a non-component in the components list), and resolving its issues can be done in a separate PR. --- ## Changelog * The scene format now uses a map to represent the collection of components rather than a list ## Migration Guide The scene format now uses a map to represent the collection of components. Scene files will need to update from the old list format. <details> <summary>Example Code</summary> ```rust // OLD [ ( entity: 0, components: [ { "bevy_transform::components::transform::Transform": ( translation: ( x: 0.0, y: 0.0, z: 0.0 ), rotation: (0.0, 0.0, 0.0, 1.0), scale: ( x: 1.0, y: 1.0, z: 1.0 ), ), }, { "my_crate::Foo": ( text: "Hello World", ), }, { "my_crate::Bar": ( baz: 123, ), }, ], ), ] // NEW [ ( entity: 0, components: { "bevy_transform::components::transform::Transform": ( translation: ( x: 0.0, y: 0.0, z: 0.0 ), rotation: (0.0, 0.0, 0.0, 1.0), scale: ( x: 1.0, y: 1.0, z: 1.0 ), ), "my_crate::Foo": ( text: "Hello World", ), "my_crate::Bar": ( baz: 123 ), }, ), ] ``` </details>

@Metadorius

# Objective Currently scenes define components using a list: ```rust [ ( entity: 0, components: [ { "bevy_transform::components::transform::Transform": ( translation: ( x: 0.0, y: 0.0, z: 0.0 ), rotation: (0.0, 0.0, 0.0, 1.0), scale: ( x: 1.0, y: 1.0, z: 1.0 ), ), }, { "my_crate::Foo": ( text: "Hello World", ), }, { "my_crate::Bar": ( baz: 123, ), }, ], ), ] ``` However, this representation has some drawbacks (as pointed out by @Metadorius in [this](bevyengine#4561 (comment)) comment): 1. Increased nesting and more characters (minor effect on overall size) 2. More importantly, by definition, entities cannot have more than one instance of any given component. Therefore, such data is best stored as a map— where all values are meant to have unique keys. ## Solution Change `components` to store a map of components rather than a list: ```rust [ ( entity: 0, components: { "bevy_transform::components::transform::Transform": ( translation: ( x: 0.0, y: 0.0, z: 0.0 ), rotation: (0.0, 0.0, 0.0, 1.0), scale: ( x: 1.0, y: 1.0, z: 1.0 ), ), "my_crate::Foo": ( text: "Hello World", ), "my_crate::Bar": ( baz: 123 ), }, ), ] ``` #### Code Representation This change only affects the scene format itself. `DynamicEntity` still stores its components as a list. The reason for this is that storing such data as a map is not really needed since: 1. The "key" of each value is easily found by just calling `Reflect::type_name` on it 2. We should be generating such structs using the `World` itself which upholds the one-component-per-entity rule One could in theory create manually create a `DynamicEntity` with duplicate components, but this isn't something I think we should focus on in this PR. `DynamicEntity` can be broken in other ways (i.e. storing a non-component in the components list), and resolving its issues can be done in a separate PR. --- ## Changelog * The scene format now uses a map to represent the collection of components rather than a list ## Migration Guide The scene format now uses a map to represent the collection of components. Scene files will need to update from the old list format. <details> <summary>Example Code</summary> ```rust // OLD [ ( entity: 0, components: [ { "bevy_transform::components::transform::Transform": ( translation: ( x: 0.0, y: 0.0, z: 0.0 ), rotation: (0.0, 0.0, 0.0, 1.0), scale: ( x: 1.0, y: 1.0, z: 1.0 ), ), }, { "my_crate::Foo": ( text: "Hello World", ), }, { "my_crate::Bar": ( baz: 123, ), }, ], ), ] // NEW [ ( entity: 0, components: { "bevy_transform::components::transform::Transform": ( translation: ( x: 0.0, y: 0.0, z: 0.0 ), rotation: (0.0, 0.0, 0.0, 1.0), scale: ( x: 1.0, y: 1.0, z: 1.0 ), ), "my_crate::Foo": ( text: "Hello World", ), "my_crate::Bar": ( baz: 123 ), }, ), ] ``` </details>

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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Oct 23, 2022

MrGVSV added this to the Bevy 0.9 milestone Oct 23, 2022

MrGVSV force-pushed the scenes-component-map branch from 1f71756 to cd021cf Compare October 24, 2022 21:28

alice-i-cecile approved these changes Oct 26, 2022

View reviewed changes

alice-i-cecile requested review from mockersf and PROMETHIA-27 October 26, 2022 16:45

mockersf approved these changes Oct 27, 2022

View reviewed changes

mockersf 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 27, 2022

Use map for scene components

f0623ae

MrGVSV force-pushed the scenes-component-map branch from cd021cf to f0623ae Compare October 27, 2022 00:54

bors bot changed the title ~~bevy_scene: Use map for scene components~~ [Merged by Bors] - bevy_scene: Use map for scene components Oct 27, 2022

bors bot closed this Oct 27, 2022

mockersf added the hacktoberfest-accepted label Oct 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - bevy_scene: Use map for scene `components` #6345

[Merged by Bors] - bevy_scene: Use map for scene `components` #6345

MrGVSV commented Oct 23, 2022 •

edited

Loading

alice-i-cecile commented Oct 27, 2022

bors bot commented Oct 27, 2022

[Merged by Bors] - bevy_scene: Use map for scene components #6345

[Merged by Bors] - bevy_scene: Use map for scene components #6345

Conversation

MrGVSV commented Oct 23, 2022 • edited Loading

Objective

Solution

Code Representation

Changelog

Migration Guide

alice-i-cecile commented Oct 27, 2022

bors bot commented Oct 27, 2022

[Merged by Bors] - bevy_scene: Use map for scene `components` #6345

[Merged by Bors] - bevy_scene: Use map for scene `components` #6345

MrGVSV commented Oct 23, 2022 •

edited

Loading