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

Change HandleUntyped::typed to not panic #2536

Closed
wants to merge 3 commits into from
Closed

Change HandleUntyped::typed to not panic #2536

wants to merge 3 commits into from

Conversation

kroltan
Copy link

@kroltan kroltan commented Jul 24, 2021

Objective

Fixes #2534

Solution

Changed the signature and implementation of HandleUntyped::typed to return a Result instead of panicking.

Adjusted a bunch of engine crate code that uses that function, but I sense a code smell. All usages are creating HandleUntypeds statically, and converting to Handle<T> on use. This could be avoided by just creating a Handle<T> in the first place.

I'll open another PR with that change.

Fixes #2534
Introduces a new FIXME: Naughty pipeline handle should be typed for
a strange pattern encountered in internal uses of `typed`.
@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jul 24, 2021
@alice-i-cecile
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Jul 24, 2021
@alice-i-cecile alice-i-cecile added A-Scenes Serialized ECS data stored on the disk C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Jul 24, 2021
@bors
Copy link
Contributor

bors bot commented Jul 24, 2021

try

Build failed:

@alice-i-cecile
Copy link
Member

Adjusted a bunch of engine crate code that uses that function, but I sense a code smell. All usages are creating HandleUntypeds statically, and converting to Handle on use. This could be avoided by just creating a Handle in the first place.

@cart do you have insight on this? Will the new renderer changes alter this?

@kroltan
Copy link
Author

kroltan commented Jul 24, 2021

Hmm seems I done a silly one and forgot to run cargo fmt.

@kroltan
Copy link
Author

kroltan commented Jul 24, 2021

Actually I forgot to run the whole CI. Still some elbow grease to go on this one!

@kroltan
Copy link
Author

kroltan commented Jul 24, 2021

@alice-i-cecile With this change, AssetServer::load changes to propagate the Result.

However this raises a further question, how come it wasn't a Result before? It seems that internally it goes to load_untracked that just yells a warning if the loading fails.

This feels like a big API change, so should I do the same and output a warning instead of changing the return type of AssetServer::load (leaving this interface change to another issue), or bite the bullet and change the interface?

(I didn't notice this earlier since I was being a silly man and forgot to run the CI that showed the actual scope of the change 😅)

@alice-i-cecile
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Jul 24, 2021
@bors
Copy link
Contributor

bors bot commented Jul 24, 2021

try

Build failed:

@alice-i-cecile
Copy link
Member

However this raises a further question, how come it wasn't a Result before? It seems that internally it goes to load_untracked that just yells a warning if the loading fails.

Hmm. I'm genuinely unsure. I know asset loading is deliberately async; maybe it's to avoid propagating results?

In any case, I think we should split this work out and create a new issue. The asset pipeline needs some serious design work in general (see #1701, #708) so it's better to tackle that more holistically.

@kroltan
Copy link
Author

kroltan commented Jul 24, 2021

Actually, upon a bit more careful investigation, currently the error path would never trigger anyways. typed can only fail for HandleId::Id, not HandleId::AssetPathId, which is what the AssetServer code path uses.

Anyways, fixed the CI errors and reverted the AssetServer::load signature.

@mockersf
Copy link
Member

This could be avoided by just creating a Handle<T> in the first place.

There isn't yet a const way to make a Handle<T>, but you can probably adapt

pub const fn weak_from_u64(uuid: Uuid, id: u64) -> Self {

@kroltan
Copy link
Author

kroltan commented Jul 25, 2021

There isn't yet a const way to make a Handle<T>

Yeah, you're right, I scratched up this change but it requires #![feature(const_fn_trait_bound)].

It could be revisited once that language feature get stabilized.


Regarding the contents of this PR, this should be ready for review.

@mockersf
Copy link
Member

Regarding the contents of this PR, this should be ready for review.

Not a fan of the FIXME comments, could you reword them?

@kroltan
Copy link
Author

kroltan commented Jul 25, 2021

How about this?

Handle is always converted to typed, see discussion in PR #2536

@mockersf
Copy link
Member

Good for me! Thanks

@NathanSWard
Copy link
Contributor

Curious, I'm pretty sure handle used to return a Result/Option type and then it got changed to use expect.
Do we know the reason for the inital change?

@NathanSWard
Copy link
Contributor

Could we add unit tests to bind the HandleUntyped::typed behavior?

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Aug 5, 2021

Curious, I'm pretty sure handle used to return a Result/Option type and then it got changed to use expect.
Do we know the reason for the inital change?

Poking around, it looks like this got changed for 0.3, from https://docs.rs/bevy/0.2.0/bevy/prelude/struct.Handle.html to https://docs.rs/bevy/0.3.0/bevy/asset/struct.HandleUntyped.html#method.typed. Here's the line / commit to blame: c32e637#diff-cffa1459c07909ff31710f4d23192ae34a30938951b42076f162702f04c3f601L79

The PR description has no justification of this change; @cart did you have some Extremely Good Argument against making this return a Result/Option that we've overlooked? I suspect it was just overlooked :)

@cart
Copy link
Member

cart commented Aug 13, 2021

No "extremely good argument". Just the argument that the implicit panic is more ergonomic. I'm down to change it, given how uncommon this scenario is.

@alice-i-cecile
Copy link
Member

Closing this out, as I don't think it's relevant any more after #4794 :)

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-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HandleUntyped::typed should return a Result
5 participants