Replace unwraps with expects in bevy_pbr #4046

omarbassam88 · 2022-02-26T17:27:25Z

Objective

Replacing all the unwraps with either expect or unwrap_or_else .
Fixes Tracking issue: Replace unwrap with expect #3899 .

Solution

Replacing all the unwraps with more helpful error messages.

ghost · 2022-02-26T17:40:17Z

Something went wrong here. You have two commits (Fix mesh2d_manual example and Fix unnecessary casts) that have nothing to do with this PR. Could you please remove those commits so that you only have your Replaced unwraps for bevy_pbr crate in there? Thanks.

omarbassam88 · 2022-02-26T18:05:14Z

Something went wrong here. You have two commits (Fix mesh2d_manual example and Fix unnecessary casts) that have nothing to do with this PR. Could you please remove those commits so that you only have your Replaced unwraps for bevy_pbr crate in there? Thanks.

I did fix it now. Sorry, this is my first pull request.

ghost · 2022-02-26T18:17:03Z

I did fix it now. Sorry, this is my first pull request.

Perfect, thanks! Don't worry about it it's all good :)

crates/bevy_pbr/src/lib.rs

crates/bevy_pbr/src/material.rs

crates/bevy_pbr/src/render/light.rs

crates/bevy_pbr/src/render/mesh.rs

superdump · 2022-02-28T08:19:15Z

Good stuff by the way! I'm just clarifying some of the error messages a little. :)

Co-authored-by: Robert Swain <robert.swain@gmail.com>

omarbassam88 · 2022-02-28T13:51:18Z

Good stuff by the way! I'm just clarifying some of the error messages a little. :)

Thank you for your suggestions. I did commit your suggestions except for the one about the display of primitive_topology_bits. I am not really sure how can I test it?

superdump · 2022-02-28T13:54:34Z

Good stuff by the way! I'm just clarifying some of the error messages a little. :)

Thank you for your suggestions. I did commit your suggestions except for the one about the display of primitive_topology_bits. I am not really sure how can I test it?

I guess try println!(“{}”, primitive_topology_bits); and see what it prints and whether it makes sense. If that doesn’t work due to a missing Display trait implementation, then you can try ”{:?}” instead, which I am pretty confident works as I think I’ve used it for wgpu::Features which are also bitflags.

omarbassam88 · 2022-02-28T14:07:42Z

Good stuff by the way! I'm just clarifying some of the error messages a little. :)

Thank you for your suggestions. I did commit your suggestions except for the one about the display of primitive_topology_bits. I am not really sure how can I test it?

I guess try println!(“{}”, primitive_topology_bits); and see what it prints and whether it makes sense. If that doesn’t work due to a missing Display trait implementation, then you can try ”{:?}” instead, which I am pretty confident works as I think I’ve used it for wgpu::Features which are also bitflags.

Yes, what I actually meant was is there's a specific example or test that I can run that calls this function just to make sure that it displays correctly?

superdump · 2022-02-28T16:24:33Z

Yes, what I actually meant was is there's a specific example or test that I can run that calls this function just to make sure that it displays correctly?

I don’t think this should fail so I unfortunately have no suggestion how to trigger the error case. That’s why I’m suggesting just testing separately that a print or panic with a particular format string works.

omarbassam88 · 2022-02-28T16:41:28Z

Yes, what I actually meant was is there's a specific example or test that I can run that calls this function just to make sure that it displays correctly?

I don’t think this should fail so I unfortunately have no suggestion how to trigger the error case. That’s why I’m suggesting just testing separately that a print or panic with a particular format string works.

Ok , I just tested it now with load_gltf example using println! and just {} and it works and prints a u8 number. so. I guess it should work fine for the panic! as well. right?

superdump · 2022-02-28T19:05:54Z

Ok , I just tested it now with load_gltf example using println! and just {} and it works and prints a u8 number. so. I guess it should work fine for the panic! as well. right?

Ohhhhh, I'm sorry! primitive_topology_bits is a u32! D'oh. That was a thinko on my part. Consider it resolved.

superdump · 2022-02-28T19:07:07Z

So, now it just needs updating on top of main.

omarbassam88 · 2022-02-28T20:27:01Z

So, now it just needs updating on top of main.

What do you mean by on top of main?

omarbassam88 · 2022-02-28T20:28:07Z

Ah you mean to update it in sync with the main branch?

cart · 2022-02-28T20:36:31Z

crates/bevy_pbr/src/material.rs

-        let asset_server = world.get_resource::<AssetServer>().unwrap();
-        let render_device = world.get_resource::<RenderDevice>().unwrap();
+        let asset_server = world
+            .get_resource::<AssetServer>()


We should replace these with the new world.resource::<AssetServer>() methods to avoid the need for duplicate panic messages / improve ergonomics.

OK, Shall I do it in this pull request? or should this issue be resolved independently and across all other crates as well?

Personally I feel like there's no harm in merging this PR and then going through and changing get_resource() to resource() if that is the new(?) API to use. But it's up to @cart .

ghost · 2022-02-28T20:57:54Z

Your latest commit (merging main into your branch) went wrong. You didn't resolve the merge conflicts the right way. #4047 removed a lot of unwraps which probably caused most of the merge conflicts. Like cart already suggested, we should use world.resource now instead of world.get_resource in most cases, because this version panics with a good error message.

omarbassam88 · 2022-02-28T20:58:51Z

Your latest commit (merging main into your branch) went wrong. You didn't resolve the merge conflicts the right way. #4047 removed a lot of unwraps which probably caused most of the merge conflicts. Like cart already suggested, we should use world.resource now instead of world.get_resource in most cases, because this version panics with a good error message.

Yes, got it I am working on updating it to main branch now.

alice-i-cecile · 2022-04-11T03:59:41Z

Closing per discussion in #3899 <3

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Feb 26, 2022

ghost mentioned this pull request Feb 26, 2022

Tracking issue: Replace unwrap with expect #3899

Closed

31 tasks

ghost added 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 Feb 26, 2022

omarbassam88 force-pushed the replace_unwraps branch from 3d33632 to 5164bbe Compare February 26, 2022 18:01

Replaced unwraps for bevy_pbr crate

cd070ad

omarbassam88 force-pushed the replace_unwraps branch from 5164bbe to cd070ad Compare February 26, 2022 18:03

superdump requested changes Feb 28, 2022

View reviewed changes

omarbassam88 and others added 11 commits February 28, 2022 15:42

Update crates/bevy_pbr/src/lib.rs

ad34eee

Co-authored-by: Robert Swain <robert.swain@gmail.com>

Update crates/bevy_pbr/src/lib.rs

dfd83a5

Co-authored-by: Robert Swain <robert.swain@gmail.com>

Update crates/bevy_pbr/src/lib.rs

8391494

Co-authored-by: Robert Swain <robert.swain@gmail.com>

Update crates/bevy_pbr/src/material.rs

58fdfba

Co-authored-by: Robert Swain <robert.swain@gmail.com>

Update crates/bevy_pbr/src/render/light.rs

ceb338f

Co-authored-by: Robert Swain <robert.swain@gmail.com>

Update crates/bevy_pbr/src/render/mesh.rs

7534ea1

Co-authored-by: Robert Swain <robert.swain@gmail.com>

Update crates/bevy_pbr/src/render/mesh.rs

bea7f9c

Co-authored-by: Robert Swain <robert.swain@gmail.com>

Update crates/bevy_pbr/src/render/mesh.rs

8c25b04

Co-authored-by: Robert Swain <robert.swain@gmail.com>

Update crates/bevy_pbr/src/render/mesh.rs

26099ba

Co-authored-by: Robert Swain <robert.swain@gmail.com>

Update crates/bevy_pbr/src/render/mesh.rs

8ba3cdc

Co-authored-by: Robert Swain <robert.swain@gmail.com>

Update crates/bevy_pbr/src/render/mesh.rs

0a6276f

Co-authored-by: Robert Swain <robert.swain@gmail.com>

Merge remote-tracking branch 'upstream/main' into replace_unwraps

b45b988

cart reviewed Feb 28, 2022

View reviewed changes

resolving conflicts with main branch

fc5b0a2

omarbassam88 requested a review from superdump February 28, 2022 21:52

superdump approved these changes Feb 28, 2022

View reviewed changes

alice-i-cecile closed this Apr 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace unwraps with expects in bevy_pbr #4046

Replace unwraps with expects in bevy_pbr #4046

omarbassam88 commented Feb 26, 2022

ghost commented Feb 26, 2022

omarbassam88 commented Feb 26, 2022

ghost commented Feb 26, 2022

superdump commented Feb 28, 2022

omarbassam88 commented Feb 28, 2022

superdump commented Feb 28, 2022

omarbassam88 commented Feb 28, 2022

superdump commented Feb 28, 2022

omarbassam88 commented Feb 28, 2022

superdump commented Feb 28, 2022

superdump commented Feb 28, 2022

omarbassam88 commented Feb 28, 2022

omarbassam88 commented Feb 28, 2022

cart Feb 28, 2022

omarbassam88 Feb 28, 2022

omarbassam88 Feb 28, 2022

superdump Feb 28, 2022

ghost commented Feb 28, 2022

omarbassam88 commented Feb 28, 2022

alice-i-cecile commented Apr 11, 2022

Replace unwraps with expects in bevy_pbr #4046

Replace unwraps with expects in bevy_pbr #4046

Conversation

omarbassam88 commented Feb 26, 2022

Objective

Solution

ghost commented Feb 26, 2022

omarbassam88 commented Feb 26, 2022

ghost commented Feb 26, 2022

superdump commented Feb 28, 2022

omarbassam88 commented Feb 28, 2022

superdump commented Feb 28, 2022

omarbassam88 commented Feb 28, 2022

superdump commented Feb 28, 2022

omarbassam88 commented Feb 28, 2022

superdump commented Feb 28, 2022

superdump commented Feb 28, 2022

omarbassam88 commented Feb 28, 2022

omarbassam88 commented Feb 28, 2022

cart Feb 28, 2022

Choose a reason for hiding this comment

omarbassam88 Feb 28, 2022

Choose a reason for hiding this comment

omarbassam88 Feb 28, 2022

Choose a reason for hiding this comment

superdump Feb 28, 2022

Choose a reason for hiding this comment

ghost commented Feb 28, 2022

omarbassam88 commented Feb 28, 2022

alice-i-cecile commented Apr 11, 2022