Use embedded_asset to load PBR shaders #19137

atlv24 · 2025-05-09T05:12:43Z

Objective

Get in-engine shader hot reloading working

Solution

Adopt Use embedded_asset to load shaders #12009
Cut back on everything possible to land an MVP: we only hot-reload PBR in deferred shading mode. This is to minimize the diff and avoid merge hell. The rest shall come in followups.

Testing

cargo run --example pbr --features="embedded_watcher" and edit some pbr shader code

andriyDev

Thank you so much for making this happen! I'm so excited to clean out these weak handles!

crates/bevy_asset/src/io/embedded/mod.rs

crates/bevy_pbr/src/deferred/mod.rs

crates/bevy_render/src/lib.rs

crates/bevy_render/src/render_resource/shader.rs

crates/bevy_pbr/src/lib.rs

janhohenheim

Please ensure that this does not break our Wasm builds when compiling on Windows, see #14246
Hopefully that rustc bug was fixed in the year since :D
Edit: it was not ._.
Also relevant to test: #14718 and #10575

atlv24 · 2025-05-12T08:00:17Z

Waiting for #19139

superdump · 2025-05-12T10:13:15Z

crates/bevy_asset/src/io/embedded/mod.rs

+macro_rules! load_embedded_asset {
+    (@get: $path: literal, $provider: expr) => {{
+        let path = $crate::embedded_path!($path);
+        let path = $crate::AssetPath::from_path_buf(path).with_source("embedded");


I wonder if we want to name the AssetPaths given they have a label field for that.

crates/bevy_render/src/lib.rs

crates/bevy_pbr/src/deferred/mod.rs

superdump · 2025-05-12T10:23:44Z

crates/bevy_pbr/src/pbr_material.rs

@@ -1345,7 +1345,7 @@ impl From<&StandardMaterial> for StandardMaterialKey {

 impl Material for StandardMaterial {
    fn fragment_shader() -> ShaderRef {
-        PBR_SHADER_HANDLE.into()
+        shader_ref(bevy_asset::embedded_path!("render/pbr.wgsl"))


This feels like possibly a not so discoverable pattern? I wonder if we should have a clearly-named macro for this particularly and likely common usage?

superdump

Mostly looks good to me. Just left a few comments. If someone wants to merge before I get back to this, I am not opposed.

superdump

I'm not going to block on the two remaining comments I had.

janhohenheim

From my limited understanding, this also looks correct :)

crates/bevy_pbr/src/lib.rs

crates/bevy_asset/src/path.rs

ecoskey · 2025-05-12T18:37:21Z

crates/bevy_asset/src/io/embedded/mod.rs

@@ -140,7 +207,7 @@ impl EmbeddedAssetRegistry {
 #[macro_export]
 macro_rules! embedded_path {
    ($path_str: expr) => {{
-        embedded_path!("src", $path_str)
+        $crate::embedded_path!("src", $path_str)


is this meant to fix use bevy_asset::embedded_path as [something else]?

im not sure why it doesn't work without this.

crates/bevy_pbr/src/lib.rs

atlv24 force-pushed the shader-hot-reload branch from c2c886a to 9572536 Compare May 9, 2025 05:17

tychedelia added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 9, 2025

github-project-automation bot added this to Rendering May 9, 2025

andriyDev reviewed May 9, 2025

View reviewed changes

janhohenheim reviewed May 9, 2025

View reviewed changes

atlv24 force-pushed the shader-hot-reload branch from 9572536 to 6995c4a Compare May 9, 2025 07:44

atlv24 added the S-Blocked This cannot move forward until something else changes label May 12, 2025

atlv24 requested a review from andriyDev May 12, 2025 08:11

superdump reviewed May 12, 2025

View reviewed changes

crates/bevy_render/src/lib.rs Show resolved Hide resolved

superdump reviewed May 12, 2025

View reviewed changes

crates/bevy_pbr/src/deferred/mod.rs Show resolved Hide resolved

superdump reviewed May 12, 2025

View reviewed changes

janhohenheim removed the S-Blocked This cannot move forward until something else changes label May 12, 2025

superdump approved these changes May 12, 2025

View reviewed changes

janhohenheim approved these changes May 12, 2025

View reviewed changes

janhohenheim added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 12, 2025

ecoskey approved these changes May 12, 2025

View reviewed changes

andriyDev approved these changes May 13, 2025

View reviewed changes

crates/bevy_pbr/src/lib.rs Show resolved Hide resolved

crates/bevy_pbr/src/lib.rs Show resolved Hide resolved

Use embedded_asset to load PBR shaders

1679b3e

atlv24 force-pushed the shader-hot-reload branch from 6995c4a to 1679b3e Compare May 15, 2025 21:08

andriyDev approved these changes May 15, 2025

View reviewed changes

mockersf approved these changes May 15, 2025

View reviewed changes

Merge branch 'main' into shader-hot-reload

74177e3

superdump added this pull request to the merge queue May 16, 2025

Merged via the queue into bevyengine:main with commit 1395152 May 16, 2025
32 checks passed

github-project-automation bot moved this to Done in Rendering May 16, 2025

mockersf mentioned this pull request May 16, 2025

'bevy_render::maths' import path not found on web #19226

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use embedded_asset to load PBR shaders #19137

Use embedded_asset to load PBR shaders #19137

atlv24 commented May 9, 2025

andriyDev left a comment

janhohenheim left a comment •

edited

Loading

atlv24 commented May 12, 2025

superdump May 12, 2025 •

edited

Loading

superdump May 12, 2025

superdump left a comment

superdump left a comment

janhohenheim left a comment

ecoskey May 12, 2025

atlv24 May 12, 2025

Use embedded_asset to load PBR shaders #19137

Use embedded_asset to load PBR shaders #19137

Conversation

atlv24 commented May 9, 2025

Objective

Solution

Testing

andriyDev left a comment

Choose a reason for hiding this comment

janhohenheim left a comment • edited Loading

Choose a reason for hiding this comment

atlv24 commented May 12, 2025

superdump May 12, 2025 • edited Loading

Choose a reason for hiding this comment

superdump May 12, 2025

Choose a reason for hiding this comment

superdump left a comment

Choose a reason for hiding this comment

superdump left a comment

Choose a reason for hiding this comment

janhohenheim left a comment

Choose a reason for hiding this comment

ecoskey May 12, 2025

Choose a reason for hiding this comment

atlv24 May 12, 2025

Choose a reason for hiding this comment

janhohenheim left a comment •

edited

Loading

superdump May 12, 2025 •

edited

Loading