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

assets.get can take any kind of handle #6948

Closed
wants to merge 1 commit into from

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