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

Remove Component trait impl from Handle<T> #15716

Closed
tim-blackbird opened this issue Oct 7, 2024 · 4 comments · Fixed by #15796
Closed

Remove Component trait impl from Handle<T> #15716

tim-blackbird opened this issue Oct 7, 2024 · 4 comments · Fixed by #15796
Labels
A-Assets Load files from disk to use for things like images, models, and sounds A-Cross-Cutting Impacts the entire engine C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers
Milestone

Comments

@tim-blackbird
Copy link
Contributor

Objective

Remove the Component trait implementation from Handle<T> as its usage is now considered an anti-pattern.

The move to required components has notably made porting error prone both internally and for the code of early adopters. Due a lack of errors on the usage of high-profile but now no longer relevant Handle components like Handle<Mesh> and Handle<StandardMaterial>.

@tim-blackbird tim-blackbird added A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers labels Oct 7, 2024
@tim-blackbird tim-blackbird added this to the 0.15 milestone Oct 7, 2024
@benfrankel
Copy link
Contributor

benfrankel commented Oct 7, 2024

This will also resolve #14124 (migrating entirely to Handle-as-field).

@tim-blackbird
Copy link
Contributor Author

tim-blackbird commented Oct 8, 2024

I thought I remembered there being an issue related to this already :)

I haven't created an issue for Handle<Image> since the sprite migration to required components is well on it's way

@mgi388
Copy link
Contributor

mgi388 commented Oct 8, 2024

@tim-blackbird I think the new handle wrapper components need to be registered in the type registry. I tried locally myself changing one of my own Handle<Foo>s to FooHandle<Foo> and confirmed in egui inspector if we don't app.register_type::<FooHandle>(), then you cannot inspect the component.

Separately, I was also wondering if the engine should have a macro for this boilerplate?

impl From<Handle<AnimationGraph>> for AnimationGraphHandle {
fn from(handle: Handle<AnimationGraph>) -> Self {
Self(handle)
}
}
impl From<AnimationGraphHandle> for AssetId<AnimationGraph> {
fn from(handle: AnimationGraphHandle) -> Self {
handle.id()
}
}
impl From<&AnimationGraphHandle> for AssetId<AnimationGraph> {
fn from(handle: &AnimationGraphHandle) -> Self {
handle.id()
}
}
Fine if not (or if it's not generic enough to be a macro generated for every component wrapper), or if it could come later, but I just wondered if it would help the ecosystem migrate.

@tim-blackbird
Copy link
Contributor Author

I always forget to add those calls to register_type 😔

We could potentially make use of the derive_more crate to cut down on some boilerplate

github-merge-queue bot pushed a commit that referenced this issue Oct 9, 2024
…nent` (#15749)

# Objective

- Another step towards #15716
- Remove trait implementations that are dependent on `Handle<T>` being a
`Component`

## Solution

- Remove unused `ExtractComponent` trait implementation for `Handle<T>`
- Remove unused `ExtractInstance` trait implementation for `AssetId`
- Although the `ExtractInstance` trait wasn't used, the `AssetId`s were
being stored inside of `ExtractedInstances` which has an
`ExtractInstance` trait bound on its contents.
I've upgraded the `RenderMaterialInstances` type alias to be its own
resource, identical to `ExtractedInstances<AssetId<M>>` to get around
that with minimal breakage.
## Testing

Tested `many_cubes`, rendering did not explode
github-merge-queue bot pushed a commit that referenced this issue Oct 9, 2024
# Objective

- Closes #15716
- Closes #15718

## Solution

- Replace `Handle<MeshletMesh>` with a new `MeshletMesh3d` component
- As expected there were some random things that needed fixing:
- A couple tests were storing handles just to prevent them from being
dropped I believe, which seems to have been unnecessary in some.
- The `SpriteBundle` still had a `Handle<Image>` field. I've removed
this.
- Tests in `bevy_sprite` incorrectly added a `Handle<Image>` field
outside of the `Sprite` component.
- A few examples were still inserting `Handle`s, switched those to their
corresponding wrappers.
- 2 examples that were still querying for `Handle<Image>` were changed
to query `Sprite`

## Testing

- I've verified that the changed example work now

## Migration Guide

`Handle` can no longer be used as a `Component`. All existing Bevy types
using this pattern have been wrapped in their own semantically
meaningful type. You should do the same for any custom `Handle`
components your project needs.

The `Handle<MeshletMesh>` component is now `MeshletMesh3d`.

The `WithMeshletMesh` type alias has been removed. Use
`With<MeshletMesh3d>` instead.
bas-ie added a commit to bas-ie/bevy_ecs_tilemap that referenced this issue Oct 24, 2024
rparrett pushed a commit to rparrett/bevy_ecs_tilemap that referenced this issue Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds A-Cross-Cutting Impacts the entire engine C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants