Skip to content

assets.get can take any kind of handle#6948

Closed
mockersf wants to merge 1 commit intobevyengine:mainfrom
mockersf:assets-get-using-handleid
Closed

assets.get can take any kind of handle#6948
mockersf wants to merge 1 commit intobevyengine:mainfrom
mockersf:assets-get-using-handleid

Conversation

@mockersf
Copy link
Member

Objective

  • assets.get can take an Handle<T> or an HandleId

Solution

  • it's using the value as a HandleId anyway, so just expose it as needing the trait

@mockersf mockersf added the A-Assets Load files from disk to use for things like images, models, and sounds label Dec 13, 2022
@mockersf mockersf force-pushed the assets-get-using-handleid branch from ecbf158 to 5d93974 Compare December 14, 2022 00:18
Copy link

@HoNile HoNile left a comment

Choose a reason for hiding this comment

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

Good but if get change I think get_mut should do the same, and I guess contains too.

@tim-blackbird
Copy link
Contributor

tim-blackbird commented Dec 14, 2022

I'm not sure about this. This removes a type check that prevents Handle<Mesh> from being passed into Assets<Image>::get for example.

Then again, a lot of the other methods already take Into<HandleId> :(

@IceSentry
Copy link
Contributor

Yeah, I'm with ira on that one, the typecheck seems valuable here.

@james7132
Copy link
Member

Doesn't HandleId carry type/asset path information in it already? If we pass a Handle<Mesh> into a Assets<Image> it will always fail. Though, I guess that's a silent failure that should be caught at compile time, if possible.

@infmagic2047
Copy link
Contributor

It was actually changed from Into<HandleId> to Handle<T> in #4794 for the typecheck 😄

@mockersf mockersf closed this Jan 3, 2023
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants