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

(fix) SSRPlugin: Don't reference default deferred lighting pass if it doesn't exist #16932

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

CrushedPixel
Copy link

@CrushedPixel CrushedPixel commented Dec 22, 2024

Fixes a crash when using deferred rendering but disabling the default deferred lighting plugin.

The Issue

The ScreenSpaceReflectionsPlugin references NodePbr::DeferredLightingPass, which hasn't been added when PbrPlugin::add_default_deferred_lighting_plugin is false.

This yields the following crash:

thread 'main' panicked at /Users/marius/Documents/dev/bevy/crates/bevy_render/src/render_graph/graph.rs:155:26:
InvalidNode(DeferredLightingPass)
stack backtrace:
   0: rust_begin_unwind
             at /rustc/90b35a6239c3d8bdabc530a6a0816f7ff89a0aaf/library/std/src/panicking.rs:665:5
   1: core::panicking::panic_fmt
             at /rustc/90b35a6239c3d8bdabc530a6a0816f7ff89a0aaf/library/core/src/panicking.rs:74:14
   2: bevy_render::render_graph::graph::RenderGraph::add_node_edges
             at /Users/marius/Documents/dev/bevy/crates/bevy_render/src/render_graph/graph.rs:155:26
   3: <bevy_app::sub_app::SubApp as bevy_render::render_graph::app::RenderGraphApp>::add_render_graph_edges
             at /Users/marius/Documents/dev/bevy/crates/bevy_render/src/render_graph/app.rs:66:13
   4: <bevy_pbr::ssr::ScreenSpaceReflectionsPlugin as bevy_app::plugin::Plugin>::finish
             at /Users/marius/Documents/dev/bevy/crates/bevy_pbr/src/ssr/mod.rs:234:9
   5: bevy_app::app::App::finish
             at /Users/marius/Documents/dev/bevy/crates/bevy_app/src/app.rs:255:13
   6: bevy_winit::state::winit_runner
             at /Users/marius/Documents/dev/bevy/crates/bevy_winit/src/state.rs:859:9
   7: core::ops::function::FnOnce::call_once
             at /Users/marius/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
   8: core::ops::function::FnOnce::call_once{{vtable.shim}}
             at /Users/marius/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
   9: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
             at /Users/marius/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/boxed.rs:2454:9
  10: bevy_app::app::App::run
             at /Users/marius/Documents/dev/bevy/crates/bevy_app/src/app.rs:184:9
  11: bevy_deferred_test::main
             at ./src/main.rs:9:5
  12: core::ops::function::FnOnce::call_once
             at /Users/marius/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5

Minimal reproduction example:

use bevy::core_pipeline::prepass::{DeferredPrepass, DepthPrepass};
use bevy::pbr::{DefaultOpaqueRendererMethod, PbrPlugin, ScreenSpaceReflections};
use bevy::prelude::*;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins.set(PbrPlugin {
            add_default_deferred_lighting_plugin: false,
            ..default()
        }))
        .add_systems(Startup, setup)
        .insert_resource(DefaultOpaqueRendererMethod::deferred())
        .run();
}

/// set up a camera
fn setup(
    mut commands: Commands
) {
    // camera
    commands.spawn((
        Camera3d::default(),
        Transform::from_xyz(-2.5, 4.5, 9.0).looking_at(Vec3::ZERO, Vec3::Y),
        DepthPrepass,
        DeferredPrepass,
        ScreenSpaceReflections::default(),
    ));
}

The Fix

When no node under the default lighting node's label exists, this label isn't added to the SSR's graph node edges. It's good to keep the SSRPlugin enabled, this way, users can plug in their own lighting system, which I have successfully done on top of this PR.

Workarounds

A current workaround for this issue is to re-use Bevy's NodePbr::DeferredLightingPass as the label for your own custom lighting pass node.

Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@CrushedPixel CrushedPixel force-pushed the fix/ssr-with-default-deferred-lighting-disabled branch from 059f47b to 6526c8a Compare December 22, 2024 18:27
Fixes a crash when using DefaultPlugins with `PbrPlugin { add_default_deferred_lighting_plugin: false, ..default() }`
@CrushedPixel CrushedPixel force-pushed the fix/ssr-with-default-deferred-lighting-disabled branch from 6526c8a to 3ad3b1d Compare December 22, 2024 18:29
@CrushedPixel
Copy link
Author

Did a force push to run rustfmt, had the IDE setup a bit wrong

let has_default_deferred_lighting_pass = render_app
.world_mut()
.get_resource_mut::<RenderGraph>()
.unwrap()
Copy link
Contributor

Choose a reason for hiding this comment

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

This should essentially never be reached, but I'd prefer if this used expect with an error message. Alternatively, just use .resource_mut which should already panic with a decent error message

Copy link
Author

Choose a reason for hiding this comment

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

Used resource_mut, I agree that that is the more idiomatic approach

@IceSentry IceSentry added A-Rendering Drawing game state to the screen S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Trivial Nice and easy! A great choice to get started with Bevy C-Bug An unexpected or incorrect behavior labels Dec 22, 2024
@CrushedPixel
Copy link
Author

A current workaround for this issue is to simply re-use Bevy's NodePbr::DeferredLightingPass as the label for your own custom lighting pass node.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior D-Trivial Nice and easy! A great choice to get started with Bevy S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants