bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
35.04k stars 3.44k forks source link

API inconsistency with `Handle` as component or field of component #14124

Open benfrankel opened 2 months ago

benfrankel commented 2 months ago

Question

Should components with associated assets (e.g. Sprite + Handle<Image>) include their Handle as a field, or as a separate component on the same entity, or something else?

From the Discord discussion, this is blocked on Bevy scenes before a design decision can be made.

Status quo

Bevy's answer to this question is inconsistent (as of March 6th, 2024):

Component with a wrapped Handle field:

  • UiImage::texture: Handle<Image>
  • TextureAtlas::layout: Handle<TextureAtlasLayout>
  • Skybox::image: Handle<Image>
  • Mesh2dHandle::0: Handle<Mesh>
  • SkinnedMesh::inverse_bindposes: Handle<SkinnedMeshInverseBindposes>
  • Lightmap::image: Handle<Image>
  • IrradianceVolume::voxels: Handle<Image>
  • EnvironmentMapLight::diffuse_map: Handle<Image>
  • EnvironmentMapLight::specular_map: Handle<Image>

Component with an associated Handle-as-component:

  • Sprite + Handle<Image>
  • more? hard to ripgrep for this

Bundle with a Handle-as-component field:

  • SpriteBundle::texture: Handle<Image>
  • SceneBundle::scene: Handle<Scene>
  • DynamicSceneBundle::scene: Handle<DynamicScene>
  • MaterialMesh2dBundle<M>::material: Handle<M>
  • MaterialMeshBundle<M>::material: Handle<M>
  • MaterialMeshBundle<M>::mesh: Handle<Mesh>
  • MaterialNodeBundle<M>::material: Handle<M>
  • AudioSourceBundle<Source>::source: Handle<Source>
alice-i-cecile commented 2 months ago

I agree; this design has annoyed me since the Component trait was implemented. I think these should all be newtyped personally.

benfrankel commented 1 week ago

This seems to be unblocked now, with required components landed and the scene overhaul generally being underway.

alice-i-cecile commented 1 week ago

This was discussed informally today, with both @cart and I being broadly on board with moving to wrapper components: https://discordapp.com/channels/691052431525675048/1264881140007702558/1278098028536008827