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

Shader is not loaded error is unclear #13644

Closed
alice-i-cecile opened this issue Jun 3, 2024 · 2 comments · Fixed by #13766
Closed

Shader is not loaded error is unclear #13644

alice-i-cecile opened this issue Jun 3, 2024 · 2 comments · Fixed by #13766
Labels
A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Uncontroversial This work is generally agreed upon

Comments

@alice-i-cecile
Copy link
Member

The actual error produced here feels confusing to me, which is unfortunate.

thread '<unnamed>' panicked at examples/shader/compute_shader_game_of_life.rs:233:25:
Initializing assets/shaders/game_of_life.wgsl:
Pipeline could not be compiled because the following shader is not loaded yet: AssetId<bevy_render::render_resource::shader::Shader>{ index: 0, generation: 0}
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Encountered a panic in system `bevy_render::renderer::render_system`!

Specifically, the not loaded *yet* part. I wonder if that could be improved (not in this PR), or if there are scenarios where that language actually makes sense?

Originally posted by @rparrett in #13624 (comment)

@alice-i-cecile alice-i-cecile added D-Trivial Nice and easy! A great choice to get started with Bevy A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples and removed D-Trivial Nice and easy! A great choice to get started with Bevy labels Jun 3, 2024
@imrn99
Copy link
Contributor

imrn99 commented Jun 8, 2024

Hello all,

I took some time to look into this issue for a first contribution, and here is what I've found so far.

The printed-out error corresponds to the following enum variant:

https://github.com/bevyengine/bevy/blob/3bfc42766621d12749c09eca3bfd9911dc0dc3f5/crates/bevy_render/src/render_resource/pipeline_cache.rs#L1016C1-L1022C38

From the variant's name & its error message, it seems like it's trying to cover different issues (all related to shader loading, but still different); However, I could only find two usages of the variant in the source code, only one of which being a return:

https://github.com/bevyengine/bevy/blob/3bfc42766621d12749c09eca3bfd9911dc0dc3f5/crates/bevy_render/src/render_resource/pipeline_cache.rs#L276C1-L288C62

All occurences (cross-checked in my IDE):

grep -rn --include="*.rs" "ShaderNotLoaded"
# ./crates/bevy_render/src/render_resource/pipeline_cache.rs:288 : .ok_or(PipelineCacheError::ShaderNotLoaded(id))?; (!)
# ./crates/bevy_render/src/render_resource/pipeline_cache.rs:929 : PipelineCacheError::ShaderNotLoaded(_)
# ./crates/bevy_render/src/render_resource/pipeline_cache.rs:1022: ShaderNotLoaded(AssetId<Shader>),

I'm willing to make a PR to fix this, but I'm not sure how it should be handled. Was there a greater motivation behind the current message (or a previous usage deleted at some point) that would justify having a ShaderNotLoadedYet variant? My guess is that it may relate to this else block during shader imports:

https://github.com/bevyengine/bevy/blob/3bfc42766621d12749c09eca3bfd9911dc0dc3f5/crates/bevy_render/src/render_resource/pipeline_cache.rs#L422C1-L455C1

The easy alternative would be to replace the current message with something along those line:

Pipeline could not be compiled because the following shader could not be loaded: {0:?}

@alice-i-cecile
Copy link
Member Author

The easy alternative would be to replace the current message with something along those line:

I would start with the easy alternative here.

github-merge-queue bot pushed a commit that referenced this issue Jun 10, 2024
# Objective

The error printed-out due to a missing shader file was confusing; This
PR changes the error message.

Fixes #13644 

## Solution

I replaced the confusing wording (`... shader is not loaded yet`) with a
clear explanation (`... shader could not be loaded`)

## Testing

> Did you test these changes? If so, how?

removing `assets/shaders/game_of_life.wgsl` & running its associated
example now produces the following error:

```
thread '<unnamed>' panicked at examples/shader/compute_shader_game_of_life.rs:233:25:
Initializing assets/shaders/game_of_life.wgsl:
Pipeline could not be compiled because the following shader could not be loaded: AssetId<bevy_render::render_resource::shader::Shader>{ index: 0, generation: 0}
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Encountered a panic in system `bevy_render::renderer::render_system`!
```

I don't think there are any tests expecting the previous error message,
so this change should not break anything.

> Are there any parts that need more testing?

If there was an intent behind the original message, this might need more
attention.

> How can other people (reviewers) test your changes? Is there anything
specific they need to know?

One should be able to preview the changes by running any example after
deleting/renaming their associated shader(s).

> If relevant, what platforms did you test these changes on, and are
there any important ones you can't test?

N/A
mockersf pushed a commit that referenced this issue Jun 10, 2024
# Objective

The error printed-out due to a missing shader file was confusing; This
PR changes the error message.

Fixes #13644 

## Solution

I replaced the confusing wording (`... shader is not loaded yet`) with a
clear explanation (`... shader could not be loaded`)

## Testing

> Did you test these changes? If so, how?

removing `assets/shaders/game_of_life.wgsl` & running its associated
example now produces the following error:

```
thread '<unnamed>' panicked at examples/shader/compute_shader_game_of_life.rs:233:25:
Initializing assets/shaders/game_of_life.wgsl:
Pipeline could not be compiled because the following shader could not be loaded: AssetId<bevy_render::render_resource::shader::Shader>{ index: 0, generation: 0}
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Encountered a panic in system `bevy_render::renderer::render_system`!
```

I don't think there are any tests expecting the previous error message,
so this change should not break anything.

> Are there any parts that need more testing?

If there was an intent behind the original message, this might need more
attention.

> How can other people (reviewers) test your changes? Is there anything
specific they need to know?

One should be able to preview the changes by running any example after
deleting/renaming their associated shader(s).

> If relevant, what platforms did you test these changes on, and are
there any important ones you can't test?

N/A
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-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants