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

Replace unwraps with expects in bevy_app, bevy_audio, bevy_core, and bevy_core_pipeline #3913

Closed
wants to merge 12 commits into from
2 changes: 1 addition & 1 deletion crates/bevy_app/src/schedule_runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ impl Plugin for ScheduleRunnerPlugin {
{
fn set_timeout(f: &Closure<dyn FnMut()>, dur: Duration) {
web_sys::window()
.unwrap()
.expect("A browser is required to run wasm, but none was found")
.set_timeout_with_callback_and_timeout_and_arguments_0(
f.as_ref().unchecked_ref(),
dur.as_millis() as i32,
Expand Down
10 changes: 7 additions & 3 deletions crates/bevy_audio/src/audio_output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ where
{
fn play_source(&self, audio_source: &Source) {
if let Some(stream_handle) = &self.stream_handle {
let sink = Sink::try_new(stream_handle).unwrap();
let sink = Sink::try_new(stream_handle).expect("Could not play audio");
sink.append(audio_source.decoder());
sink.detach();
}
Expand Down Expand Up @@ -72,8 +72,12 @@ where
Source: Decodable,
{
let world = world.cell();
let audio_output = world.get_non_send::<AudioOutput<Source>>().unwrap();
let mut audio = world.get_resource_mut::<Audio<Source>>().unwrap();
let audio_output = world
.get_non_send::<AudioOutput<Source>>()
.expect("Could not find `AudioOutput` in the `World`.");
let mut audio = world
.get_resource_mut::<Audio<Source>>()
.expect("Could not find `Audio` in the `World`.");
Comment on lines +75 to +80
Copy link
Member

Choose a reason for hiding this comment

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

In both those cases, the issue would be either:

  • the user is calling this system without adding the AudioPlugin
  • the user is calling this system on a custom Source without having it set up first like in
    app.init_non_send_resource::<AudioOutput<AudioSource>>()
    .add_asset::<AudioSource>()
    .init_resource::<Audio<AudioSource>>()

It could be helpful to explain how to fix the issue

Copy link
Author

Choose a reason for hiding this comment

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

Would this be a case where adding to the error codes is best for an explanation?


if let Some(audio_sources) = world.get_resource::<Assets<Source>>() {
audio_output.try_play_queued(&*audio_sources, &mut *audio);
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_audio/src/audio_source.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,6 @@ impl Decodable for AudioSource {
type DecoderItem = <rodio::Decoder<Cursor<AudioSource>> as Iterator>::Item;

fn decoder(&self) -> Self::Decoder {
rodio::Decoder::new(Cursor::new(self.clone())).unwrap()
rodio::Decoder::new(Cursor::new(self.clone())).expect("Could not create `Decoder`. Did you forget to enable the appropriate audio feature or are you using an unsupported audio file format?")
}
}
45 changes: 36 additions & 9 deletions crates/bevy_core/src/time/fixed_timestep.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,10 @@ impl FixedTimestep {
) -> ShouldRun {
let should_run = state.update(&time);
if let Some(ref label) = state.label {
let res_state = fixed_timesteps.fixed_timesteps.get_mut(label).unwrap();
let res_state = fixed_timesteps
.fixed_timesteps
.get_mut(label)
.unwrap_or_else(|| panic!("Could not find a FixedTimestep labeled {label}"));
res_state.step = state.step;
res_state.accumulator = state.accumulator;
}
Expand Down Expand Up @@ -206,7 +209,9 @@ impl System for FixedTimestep {
Box::new(Self::prepare_system.config(|c| c.0 = Some(self.state.clone())));
self.internal_system.initialize(world);
if let Some(ref label) = self.state.label {
let mut fixed_timesteps = world.get_resource_mut::<FixedTimesteps>().unwrap();
let mut fixed_timesteps = world
.get_resource_mut::<FixedTimesteps>()
.expect("Could not find resource FixedTimesteps. It can be added with CorePlugin, DefaultPlugins, or MinimalPlugins");
Copy link

Choose a reason for hiding this comment

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

Suggested change
.expect("Could not find resource FixedTimesteps. It can be added with CorePlugin, DefaultPlugins, or MinimalPlugins");
.expect("Could not get the `FixedTimesteps` resource from the `World`. It can be added using the `CorePlugin`, the `DefaultPlugins`, or the `MinimalPlugins`");

fixed_timesteps.fixed_timesteps.insert(
label.clone(),
FixedTimestepState {
Expand Down Expand Up @@ -254,25 +259,45 @@ mod test {
// if time does not progress, the step does not run
schedule.run(&mut world);
schedule.run(&mut world);
assert_eq!(0, *world.get_resource::<Count>().unwrap());
assert_eq!(
0,
*world
.get_resource::<Count>()
.expect("Could not get the `Count` resource from the `World`")
);
assert_eq!(0., get_accumulator_deciseconds(&world));

// let's progress less than one step
advance_time(&mut world, instance, 0.4);
schedule.run(&mut world);
assert_eq!(0, *world.get_resource::<Count>().unwrap());
assert_eq!(
0,
*world
.get_resource::<Count>()
.expect("Could not find `Count` in the `World`.")
Copy link

Choose a reason for hiding this comment

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

Suggested change
.expect("Could not find `Count` in the `World`.")
.expect("Could not get the `Count` resource from the `World`")

);
assert_eq!(4., get_accumulator_deciseconds(&world));

// finish the first step with 0.1s above the step length
advance_time(&mut world, instance, 0.6);
schedule.run(&mut world);
assert_eq!(1, *world.get_resource::<Count>().unwrap());
assert_eq!(
1,
*world
.get_resource::<Count>()
.expect("Could not find `Count` in the `World`.")
Copy link

Choose a reason for hiding this comment

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

Suggested change
.expect("Could not find `Count` in the `World`.")
.expect("Could not get the `Count` resource from the `World`")

);
assert_eq!(1., get_accumulator_deciseconds(&world));

// runs multiple times if the delta is multiple step lengths
advance_time(&mut world, instance, 1.7);
schedule.run(&mut world);
assert_eq!(3, *world.get_resource::<Count>().unwrap());
assert_eq!(
3,
*world
.get_resource::<Count>()
.expect("Could not find `Count` in the `World`.")
Copy link

Choose a reason for hiding this comment

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

Suggested change
.expect("Could not find `Count` in the `World`.")
.expect("Could not get the `Count` resource from the `World`")

);
assert_eq!(2., get_accumulator_deciseconds(&world));
}

Expand All @@ -283,16 +308,18 @@ mod test {
fn advance_time(world: &mut World, instance: Instant, seconds: f32) {
world
.get_resource_mut::<Time>()
.unwrap()
.expect("Could not find `Time` in the `World`.")
Copy link

Choose a reason for hiding this comment

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

Suggested change
.expect("Could not find `Time` in the `World`.")
.expect("Could not get the `Time` resource from the `World`")

.update_with_instant(instance.add(Duration::from_secs_f32(seconds)));
}

fn get_accumulator_deciseconds(world: &World) -> f64 {
world
.get_resource::<FixedTimesteps>()
.unwrap()
.expect("Could not find `FixedTimesteps` in the 'World'.")
Copy link

Choose a reason for hiding this comment

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

Suggested change
.expect("Could not find `FixedTimesteps` in the 'World'.")
.expect("Could not get the `FixedTimesteps` resource from the `World`")

.get(LABEL)
.unwrap()
.unwrap_or_else(|| {
panic!("Could not find a resource of the appropriate label: {LABEL}",)
Copy link

Choose a reason for hiding this comment

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

Suggested change
panic!("Could not find a resource of the appropriate label: {LABEL}",)
panic!("Could not get the `FixedTimestepState`for the label: {LABEL}",)

})
.accumulator
.mul(10.)
.round()
Expand Down
12 changes: 9 additions & 3 deletions crates/bevy_core_pipeline/src/clear_pass.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ impl Node for ClearPassNode {
) -> Result<(), NodeRunError> {
let mut cleared_targets = HashSet::new();
let clear_color = world.get_resource::<ClearColor>().unwrap();
let render_target_clear_colors = world.get_resource::<RenderTargetClearColors>().unwrap();
let render_target_clear_colors = world
.get_resource::<RenderTargetClearColors>()
.expect("Could not find `ClearColor` resource in the `World`.");
Copy link

Choose a reason for hiding this comment

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

Suggested change
.expect("Could not find `ClearColor` resource in the `World`.");
.expect("Could not get the `RenderTargetClearColors` resource from the `World`. It can be added using the `CorePipelinePlugin`, or the `DefaultPlugins`");


// This gets all ViewTargets and ViewDepthTextures and clears its attachments
// TODO: This has the potential to clear the same target multiple times, if there
Expand Down Expand Up @@ -89,8 +91,12 @@ impl Node for ClearPassNode {
// TODO: This is a hack to ensure we don't call present() on frames without any work,
// which will cause panics. The real fix here is to clear "render targets" directly
// instead of "views". This should be removed once full RenderTargets are implemented.
let windows = world.get_resource::<ExtractedWindows>().unwrap();
let images = world.get_resource::<RenderAssets<Image>>().unwrap();
let windows = world
.get_resource::<ExtractedWindows>()
.expect("Could not find `ExtractedWindows` resource in `World`.");
Copy link

Choose a reason for hiding this comment

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

Suggested change
.expect("Could not find `ExtractedWindows` resource in `World`.");
.expect("Could not get the `ExtractedWindows` resource from the `World`. It can be added using the `WindowRenderPlugin`, the `RenderPlugin`, or the `DefaultPlugins`");

let images = world
.get_resource::<RenderAssets<Image>>()
.expect("Could not find `RenderAssets` resource in `World`.");
Copy link

Choose a reason for hiding this comment

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

Suggested change
.expect("Could not find `RenderAssets` resource in `World`.");
.expect("Could not get the `RenderAssets<Image>` resource from the `World`. It can be added using the `ImagePlugin`, the `RenderAssetPlugin::<Image>`, the `RenderPlugin` or the `DefaultPlugins`");

for target in render_target_clear_colors.colors.keys().cloned().chain(
windows
.values()
Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_core_pipeline/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ impl Plugin for CorePipelinePlugin {
draw_3d_graph::node::MAIN_PASS,
MainPass3dNode::IN_VIEW,
)
.unwrap();
.expect("Could not add slot edge to `RenderGraph`.");
Copy link

Choose a reason for hiding this comment

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

Suggested change
.expect("Could not add slot edge to `RenderGraph`.");
.expect("Could not add slot edge to `RenderGraph`");

Copy link
Member

Choose a reason for hiding this comment

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

Something to consider before throwing expects onto more "internal" non-user facing code: static strings are compiled into the binary. I don't think we want to bloat bevy apps with a bunch of "internal error messages", especially in cases like these where the context of the code is as good (if not better) for debugging than the message in the expect (for a Bevy Engine dev debugging a user-reported error).

graph.add_sub_graph(draw_3d_graph::NAME, draw_3d_graph);

let mut clear_graph = RenderGraph::default();
Expand All @@ -187,11 +187,11 @@ impl Plugin for CorePipelinePlugin {
graph.add_node(node::MAIN_PASS_DRIVER, MainPassDriverNode);
graph
.add_node_edge(node::MAIN_PASS_DEPENDENCIES, node::MAIN_PASS_DRIVER)
.unwrap();
.expect("Could not add node edge to `RenderGraph`.");
Copy link

Choose a reason for hiding this comment

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

Suggested change
.expect("Could not add node edge to `RenderGraph`.");
.expect("Could not add node edge to `RenderGraph`");

graph.add_node(node::CLEAR_PASS_DRIVER, ClearPassDriverNode);
graph
.add_node_edge(node::CLEAR_PASS_DRIVER, node::MAIN_PASS_DRIVER)
.unwrap();
.expect("Could not add node edge to `RenderGraph`.");
Copy link

Choose a reason for hiding this comment

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

Suggested change
.expect("Could not add node edge to `RenderGraph`.");
.expect("Could not add node edge to `RenderGraph`");

}
}

Expand Down
6 changes: 4 additions & 2 deletions crates/bevy_core_pipeline/src/main_pass_2d.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ impl Node for MainPass2dNode {

let draw_functions = world
.get_resource::<DrawFunctions<Transparent2d>>()
.unwrap();
.expect("Could not find `DrawFunctions` resource in the `World`.");
Copy link

Choose a reason for hiding this comment

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

Suggested change
.expect("Could not find `DrawFunctions` resource in the `World`.");
.expect("Could not get the `DrawFunctions<Transparent2d>` resource from the `World`. It can be added using the `CorePipelinePlugin`, or the `DefaultPlugins`");


let render_pass = render_context
.command_encoder
Expand All @@ -64,7 +64,9 @@ impl Node for MainPass2dNode {
let mut draw_functions = draw_functions.write();
let mut tracked_pass = TrackedRenderPass::new(render_pass);
for item in &transparent_phase.items {
let draw_function = draw_functions.get_mut(item.draw_function).unwrap();
let draw_function = draw_functions
.get_mut(item.draw_function)
.expect("Could not get draw function.");
Copy link

Choose a reason for hiding this comment

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

Suggested change
.expect("Could not get draw function.");
.unwrap_or_else(|| {
panic!("Could not get the draw function for id: {}", item.draw_function.0);
});

draw_function.draw(world, &mut tracked_pass, view_entity, item);
}
Ok(())
Expand Down
18 changes: 13 additions & 5 deletions crates/bevy_core_pipeline/src/main_pass_3d.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,9 @@ impl Node for MainPass3dNode {
let mut draw_functions = draw_functions.write();
let mut tracked_pass = TrackedRenderPass::new(render_pass);
for item in &opaque_phase.items {
let draw_function = draw_functions.get_mut(item.draw_function).unwrap();
let draw_function = draw_functions
.get_mut(item.draw_function)
.expect("Could not get draw function.");
Copy link

Choose a reason for hiding this comment

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

Suggested change
.expect("Could not get draw function.");
.unwrap_or_else(|| {
panic!("Could not get the draw function for id: {}", item.draw_function.0);
});

draw_function.draw(world, &mut tracked_pass, view_entity, item);
}
}
Expand All @@ -109,15 +111,19 @@ impl Node for MainPass3dNode {
}),
};

let draw_functions = world.get_resource::<DrawFunctions<AlphaMask3d>>().unwrap();
let draw_functions = world
.get_resource::<DrawFunctions<AlphaMask3d>>()
.expect("Could not get `DrawFunctions` resource from the `World`.");
Copy link

Choose a reason for hiding this comment

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

Suggested change
.expect("Could not get `DrawFunctions` resource from the `World`.");
.expect("Could not get the `DrawFunctions<AlphaMask3d>` resource from the `World`. It can be added using the `CorePipelinePlugin`, or the `DefaultPlugins`");


let render_pass = render_context
.command_encoder
.begin_render_pass(&pass_descriptor);
let mut draw_functions = draw_functions.write();
let mut tracked_pass = TrackedRenderPass::new(render_pass);
for item in &alpha_mask_phase.items {
let draw_function = draw_functions.get_mut(item.draw_function).unwrap();
let draw_function = draw_functions
.get_mut(item.draw_function)
.expect("Could not get draw function.");
Copy link

Choose a reason for hiding this comment

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

Suggested change
.expect("Could not get draw function.");
.unwrap_or_else(|| {
panic!("Could not get the draw function for id: {}", item.draw_function.0);
});

draw_function.draw(world, &mut tracked_pass, view_entity, item);
}
}
Expand Down Expand Up @@ -147,15 +153,17 @@ impl Node for MainPass3dNode {

let draw_functions = world
.get_resource::<DrawFunctions<Transparent3d>>()
.unwrap();
.expect("Could not get `DrawFunctions` resource from the `World`.");
Copy link

Choose a reason for hiding this comment

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

Suggested change
.expect("Could not get `DrawFunctions` resource from the `World`.");
.expect("Could not get the `DrawFunctions<Transparent3d>` resource from the `World`. It can be added using the `CorePipelinePlugin`, or the `DefaultPlugins`");


let render_pass = render_context
.command_encoder
.begin_render_pass(&pass_descriptor);
let mut draw_functions = draw_functions.write();
let mut tracked_pass = TrackedRenderPass::new(render_pass);
for item in &transparent_phase.items {
let draw_function = draw_functions.get_mut(item.draw_function).unwrap();
let draw_function = draw_functions
.get_mut(item.draw_function)
.expect("Could not get draw function.");
Copy link

Choose a reason for hiding this comment

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

Suggested change
.expect("Could not get draw function.");
.unwrap_or_else(|| {
panic!("Could not get the draw function for id: {}", item.draw_function.0);
});

draw_function.draw(world, &mut tracked_pass, view_entity, item);
}
}
Expand Down
4 changes: 3 additions & 1 deletion crates/bevy_core_pipeline/src/main_pass_driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ impl Node for MainPassDriverNode {
_render_context: &mut RenderContext,
world: &World,
) -> Result<(), NodeRunError> {
let extracted_cameras = world.get_resource::<ExtractedCameraNames>().unwrap();
let extracted_cameras = world
.get_resource::<ExtractedCameraNames>()
.expect("Could not get `ExtractedCameraNames` resource from the `World`.");
Copy link

Choose a reason for hiding this comment

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

Suggested change
.expect("Could not get `ExtractedCameraNames` resource from the `World`.");
.expect("Could not get the `ExtractedCameraNames` resource from the `World`. It can be added using the `CameraPlugin`, `RenderPlugin` or the `DefaultPlugins`");

if let Some(camera_2d) = extracted_cameras.entities.get(CameraPlugin::CAMERA_2D) {
graph.run_sub_graph(
crate::draw_2d_graph::NAME,
Expand Down