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

Confusing trait bounds on MaterialPlugin #17165

Open
Lege19 opened this issue Jan 5, 2025 · 2 comments
Open

Confusing trait bounds on MaterialPlugin #17165

Lege19 opened this issue Jan 5, 2025 · 2 comments
Labels
A-Rendering Drawing game state to the screen C-Docs An addition or correction to our documentation D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Design This issue requires design work to think about how it would best be accomplished

Comments

@Lege19
Copy link

Lege19 commented Jan 5, 2025

use bevy::{prelude::*, render::render_resource::AsBindGroup};

//#[derive(Clone, Copy, PartialEq, Eq, Hash)]
struct TestMaterialKey {
    data: u8,
}
impl From<&TestMaterial> for TestMaterialKey {
    fn from(value: &TestMaterial) -> Self {
        Self {
            data: value.data
        }
    }
}

#[derive(Clone, TypePath, AsBindGroup, Asset)]
#[bind_group_data(TestMaterialKey)]
struct TestMaterial {
    data: u8
}
impl Material for TestMaterial { }

fn main() {
    App::new().add_plugins(MaterialPlugin::<TestMaterial>::default()).run();
}

Does not compile, with the error:

error[E0277]: the trait bound `bevy::prelude::MaterialPlugin<TestMaterial>: Plugins<_>` is not satisfied
   --> src/main.rs:23:28
    |
23  |     App::new().add_plugins(MaterialPlugin::<TestMaterial>::default()).run();
    |                ----------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `bevy::bevy_app::plugin::sealed::Plugins<_>` is not implemented for `bevy::prelude::MaterialPlugin<TestMaterial>`
    |                |
    |                required by a bound introduced by this call
    |
    = help: the following other types implement trait `bevy::bevy_app::plugin::sealed::Plugins<Marker>`:
              `()` implements `bevy::bevy_app::plugin::sealed::Plugins<(bevy::bevy_app::plugin::sealed::PluginsTupleMarker,)>`
              `(S,)` implements `bevy::bevy_app::plugin::sealed::Plugins<(bevy::bevy_app::plugin::sealed::PluginsTupleMarker, P)>`
              `(S0, S1)` implements `bevy::bevy_app::plugin::sealed::Plugins<(bevy::bevy_app::plugin::sealed::PluginsTupleMarker, P0, P1)>`
              `(S0, S1, S2)` implements `bevy::bevy_app::plugin::sealed::Plugins<(bevy::bevy_app::plugin::sealed::PluginsTupleMarker, P0, P1, P2)>`
              `(S0, S1, S2, S3)` implements `bevy::bevy_app::plugin::sealed::Plugins<(bevy::bevy_app::plugin::sealed::PluginsTupleMarker, P0, P1, P2, P3)>`
              `(S0, S1, S2, S3, S4)` implements `bevy::bevy_app::plugin::sealed::Plugins<(bevy::bevy_app::plugin::sealed::PluginsTupleMarker, P0, P1, P2, P3, P4)>`
              `(S0, S1, S2, S3, S4, S5)` implements `bevy::bevy_app::plugin::sealed::Plugins<(bevy::bevy_app::plugin::sealed::PluginsTupleMarker, P0, P1, P2, P3, P4, P5)>`
              `(S0, S1, S2, S3, S4, S5, S6)` implements `bevy::bevy_app::plugin::sealed::Plugins<(bevy::bevy_app::plugin::sealed::PluginsTupleMarker, P0, P1, P2, P3, P4, P5, P6)>`
            and 8 others
    = note: required for `bevy::prelude::MaterialPlugin<TestMaterial>` to implement `Plugins<_>`
note: required by a bound in `bevy::prelude::App::add_plugins`
   --> /home/jacobs/.cargo/registry/src/index.crates.io-6f17d22bba15001f/bevy_app-0.15.1/src/app.rs:548:52
    |
548 |     pub fn add_plugins<M>(&mut self, plugins: impl Plugins<M>) -> &mut Self {
    |                                                    ^^^^^^^^^^ required by this bound in `App::add_plugins`

Uncommenting #[derive(Clone, Copy, PartialEq, Eq, Hash)] fixes the error.
The reason this does not work, is that impl Plugin for MaterialPlugin requires <M as AsBindGroup>::Data: PartialEq + Eq + Hash + Clone however this is a pretty sneaky trait bound that is very easy to look over. I think that it should be a trait bound on MaterialPlugin too so that when a user attempts to do this they get a much more useful error. Failing that, I think more attention should be drawn to this in the docs because currently it's very easy to assume that any MaterialPlugin you are allowed to construct would implement Plugin. It's in the name after all.

@Lege19 Lege19 added C-Docs An addition or correction to our documentation S-Needs-Triage This issue needs to be labelled labels Jan 5, 2025
@BenjaminBrienen
Copy link
Contributor

I think improving the error message without making MaterialPlugin more restrictive would be better. There is an attribute that we can put on the trait to give a better message: #[diagnostic::on_unimplemented()]. I'd try that first.

@BenjaminBrienen BenjaminBrienen added A-Rendering Drawing game state to the screen S-Needs-Design This issue requires design work to think about how it would best be accomplished D-Straightforward Simple bug fixes and API improvements, docs, test and examples and removed S-Needs-Triage This issue needs to be labelled labels Jan 5, 2025
@Lege19
Copy link
Author

Lege19 commented Jan 5, 2025

I thought about the making it less restrictive thing, and I think I disagree. If something isn't MaterialPlugin only implements Plugin and Default, and has no methods. So the only thing you can do with it is add it as a plugin. It exists to be a plugin. If something is a problem, it should cause an error at the earliest possible "time", so that the error is as close to the source as possible.

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-Docs An addition or correction to our documentation D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Design This issue requires design work to think about how it would best be accomplished
Projects
None yet
Development

No branches or pull requests

2 participants