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

MaterialPipelineKey should have public fields #4507

Closed
Shatur opened this issue Apr 17, 2022 · 2 comments
Closed

MaterialPipelineKey should have public fields #4507

Shatur opened this issue Apr 17, 2022 · 2 comments
Labels
A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use

Comments

@Shatur
Copy link
Contributor

Shatur commented Apr 17, 2022

In 0.6 MaterialPipeline used (MeshPipelineKey, M::Key) tuple as a key, where M is a custom material. But in 0.7 after #3959 it started using MaterialPipelineKey struct which is defined in the following way:

#[derive(Eq, PartialEq, Clone, Hash)]
pub struct MaterialPipelineKey<T> {
    mesh_key: MeshPipelineKey,
    material_key: T,
}

It look nicer, but all fields are private, so I can't call specialize in my user code for such materials (because the function accepts a key).

Here is how a real project uses it for overlay.
And here is my PR where I ported it. But it doesn't compile since in the mentioned place above I have to create MaterialPipelineKey (it compiles if I comment it out).

@superdump
Copy link
Contributor

It looks like the fields were private as MaterialPipelineKey only used in MaterialPlugin. In the bevy-hikari case, it does not use MaterialPlugin because MaterialPlugin only queues meshes with the specialized materials to the opaque / alpha mask / transparent render phases, and bevy-hikari has its own phases to queue to. So it uses the SpecializedMaterial abstraction, but not the MaterialPlugin and then implements its own queuing. This seems like a shortcoming of the API at the moment. Making the fields public does seem like a quick way to regain the flexibility that was there in 0.6 without requiring significant code duplication or significant design effort to make the material plugin handle specification of the phase(s) to support.

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Apr 17, 2022
Shatur added a commit to gardum-game/gardum that referenced this issue Apr 17, 2022
Required to migrate to Bevy 0.7. Current the plugin can't
work with 0.7 because of this upstream issue:
bevyengine/bevy#4507
bors bot pushed a commit that referenced this issue Apr 17, 2022
# Objective

Fixes #4507. This comment provides a very good explanation: #4507 (comment).

## Solution

Make `MaterialPipelineKey` fields public.
bors bot pushed a commit that referenced this issue Apr 18, 2022
# Objective

Fixes #4507. This comment provides a very good explanation: #4507 (comment).

## Solution

Make `MaterialPipelineKey` fields public.
@Shatur
Copy link
Contributor Author

Shatur commented Apr 19, 2022

Fixed by #4508.

@Shatur Shatur closed this as completed Apr 19, 2022
MonaMayrhofer pushed a commit to MonaMayrhofer/bevy that referenced this issue Jul 29, 2022
# Objective

Fixes bevyengine#4507. This comment provides a very good explanation: bevyengine#4507 (comment).

## Solution

Make `MaterialPipelineKey` fields public.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
# Objective

Fixes bevyengine#4507. This comment provides a very good explanation: bevyengine#4507 (comment).

## Solution

Make `MaterialPipelineKey` fields public.
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants