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

bevy_scene: Add ReflectBundle #6344

Closed
wants to merge 1 commit into from

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented Oct 23, 2022

Objective

Scenes do not currently support spawning entire bundles. This means that in order to simulate spawning a bundle, one must include all components within that bundle.

Given the following bundle:

#[derive(Reflect, Bundle)]
struct MyBundle {
  a: ComponentA,
  b: ComponentB,
  c: ComponentC,
  d: ComponentD,
  e: ComponentE,
  f: ComponentF,
  g: ComponentG,
}

We must then include the following in our scene file:

[
  {
    "entity": 0,
    "components": [
      {
        "my_crate::ComponentA": {
          "foo": "Hello World"
        }
      },
      {
        "my_crate::ComponentB": {}
      },
      {
        "my_crate::ComponentC": {}
      },
      {
        "my_crate::ComponentD": {}
      },
      {
        "my_crate::ComponentE": {}
      },
      {
        "my_crate::ComponentF": {}
      },
      {
        "my_crate::ComponentG": {}
      }
    ]
  }
]

We only needed to specify data for ComponentA. All other components in the bundle could have just as easily been left out since we didn't have to specify anything for them (or perhaps they're just markers).

Solution

Add ReflectBundle to allow scenes to spawn complete bundles.

With this change, we can now update our example bundle:

#[derive(Reflect, Bundle)]
#[reflect(Bundle)]
struct MyBundle {
  a: ComponentA,
  b: ComponentB,
  c: ComponentC,
  d: ComponentD,
  e: ComponentE,
  f: ComponentF,
  g: ComponentG,
}

And add a bundles section to our scene files:

[
  {
    "entity": 0,
    "components": []
    "bundles": {
      "a": {
        "foo": "Hello World"
      }
    }
  }
]

Much nicer!

Considerations

There are some caveats to this system we might need to consider:

  1. The bundles field will never be serialized by the DynamicScene serializer. This could be confusing to users who expect it to serialize. The problem is, we don't have a great way of knowing whether every necessary component exists to create a valid bundle and not lose any information. So for now it's a manually-written/deserialize-only feature.
  2. Now that Components also implement Bundle, it would be possible to register a ReflectBundle for a component. If this is done, then applying the "bundle" will result in each field of the component being inserted onto the entity. This is obviously not good. However, there's not a good way of knowing whether a Bundle is only a component or not. If we could add a Bundle::IS_COMPONENT constant or something, it might be possible to throw an error when attempting to create a ReflectBundle for a component.

Changelog

  • Added ReflectBundle
  • Added bundles field to DynamicEntity
    • Allows a bundles map to be optionally included per entity in scene files

@MrGVSV MrGVSV added C-Feature A new feature, making something new possible A-Scenes Serialized ECS data stored on the disk labels Oct 23, 2022
@MrGVSV MrGVSV force-pushed the reflect-scene-bundles branch 4 times, most recently from b9ec315 to 4029144 Compare October 28, 2022 07:28
@cart cart added the X-Controversial There is active debate or serious implications around merging this PR label Oct 28, 2022
@cart
Copy link
Member

cart commented Oct 28, 2022

First: I do think we need something functionally similar to this! Being able to specify a "logical group" of entity configuration and only specify components with non-default values is an important step for the scene system / this has always been the plan.

However this is controversial as its starting to encroach on "prefab space" and the concerns outlined in the "Components as Bundles" conversation: #2974 (comment), where we agreed that Bundles should just be "simple groups of components" (at least for now).

The main question is:

Will we support Bundles in scene editing scenarios? Supporting 3 concepts seems like too many:

  • Bundles (groups of components)
  • Prefabs (groups of components defined in code with arbitrary add/remove logic and probably the ability to spawn nested entities)
  • Scenes (arbitrary collections of entities / components / prefabs / bundles / nested scenes defined as an asset)

Will Bundles "become" prefabs? Is this even technically feasible? (this would require a number of internal changes to bevy_ecs / break existing assumptions). Does that mean components are also prefabs?

@MrGVSV
Copy link
Member Author

MrGVSV commented Oct 28, 2022

However this is controversial as its starting to encroach on "prefab space" and the concerns outlined in the "Components as Bundles" conversation: #2974 (comment), where we agreed that Bundles should just be "simple groups of components" (at least for now).

Makes sense, I don't have an issue with marking this as controversial.

Will we support Bundles in scene editing scenarios? Supporting 3 concepts seems like too many:

  • Bundles (groups of components)
  • Prefabs (groups of components defined in code with arbitrary add/remove logic and probably the ability to spawn nested entities)
  • Scenes (arbitrary collections of entities / components / prefabs / bundles / nested scenes defined as an asset)

Will Bundles "become" prefabs? Is this even technically feasible? (this would require a number of internal changes to bevy_ecs / break existing assumptions). Does that mean components are also prefabs?

Yeah there are a lot of vaguely similar concepts that are liable to confuse users. Each one kinda blends into the next (especially when coming from something like Unity or another engine).

I don't think bundles should be prefabs, but they definitely can be viewed that way. They encapsulate a set of components, can define data for those components (think constructors or Default impls), and can even be "composed" to an extent (for components, not entities).

So in my eyes, and I'm sure others, bundles are prefab-like. The main differences1 are that they don't provide any other prefab-related functionality and they don't really "exist" in the world (as they're just sugar for a group of components2).

And the same could be said for components (they're basically just a bundle with one item).

All that to say, I think we could go ahead with ReflectBundle.

My arguments for it:

  • As mentioned above, users might already be treating bundles as crude prefabs, and might see bundles-in-scenes as the logical next step
  • Prefabs are still in development and this benefits people now while they wait (assuming they ship with serialized asset formats)
  • The scene format is young and can expect to have lots of changes made to it, so we shouldn't be afraid to add/remove features as the rest of the scene/prefab story improves

My arguments against it:

  • This might confuse users who think bundles are actually prefabs when they're not, thus making future efforts to distinguish the two possibly a little more difficult
  • Because bundles don't really "exist", we can't serialize an entity with all its bundles (not without a few assumptions anyways) which could add to the confusion
  • While I do think we should be flexible with the scene format while we figure things out, I'm confident prefabs will come at some point. If they're expected within the next release or two, then maybe its better to just wait
  • And sorta like what you mentioned, adding ReflectBundle could put certain limitations on the bevy_ecs side of things that we're not fully aware of

Footnotes

  1. I could be wrong in my understanding of what prefabs are according to Bevy so these "differences" might not actually be true or slightly off base.

  2. I'm sure one of the ECS devs is screaming something about archetypes at me haha

@Shatur
Copy link
Contributor

Shatur commented Jul 14, 2023

@MrGVSV maybe we can have ReflectBundle without scene changes? Like ReflectComponent, could be useful for networking.

github-merge-queue bot pushed a commit that referenced this pull request Aug 28, 2023
# Objective

Similar to #6344, but contains only `ReflectBundle` changes. Useful for
scripting. The implementation has also been updated to look exactly like
`ReflectComponent`.

---

## Changelog

### Added

- Reflection for bundles.

---------

Co-authored-by: Gino Valente <[email protected]>
Shatur added a commit to projectharmonia/bevy that referenced this pull request Aug 30, 2023
# Objective

Similar to bevyengine#6344, but contains only `ReflectBundle` changes. Useful for
scripting. The implementation has also been updated to look exactly like
`ReflectComponent`.

---

## Changelog

### Added

- Reflection for bundles.

---------

Co-authored-by: Gino Valente <[email protected]>
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

Similar to bevyengine#6344, but contains only `ReflectBundle` changes. Useful for
scripting. The implementation has also been updated to look exactly like
`ReflectComponent`.

---

## Changelog

### Added

- Reflection for bundles.

---------

Co-authored-by: Gino Valente <[email protected]>
@richchurcher
Copy link
Contributor

Backlog cleanup: closing due to inactivity, and because it seems like once bsn! lands this may not be quite as relevant? (If I've misjudged that, please let me know!)

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-Feature A new feature, making something new possible X-Controversial There is active debate or serious implications around merging this PR
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants