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

[Merged by Bors] - Implement Clone for all pipeline types #6653

Closed
wants to merge 1 commit into from

Conversation

SludgePhD
Copy link
Contributor

@SludgePhD SludgePhD commented Nov 16, 2022

Objective

Pipelines can be customized by wrapping an existing pipeline in a newtype and adding custom logic to its implementation of SpecializedMeshPipeline::specialize. To make that easier, the wrapped pipeline type needs to implement Clone.

For example, the current non-cloneable pipelines require wrapper pipelines to pull apart the wrapped pipeline like this:

impl FromWorld for Wireframe2dPipeline {
    fn from_world(world: &mut World) -> Self {
        let p = &world.resource::<Material2dPipeline<ColorMaterial>>();
        Self {
            mesh2d_pipeline: p.mesh2d_pipeline.clone(),
            material2d_layout: p.material2d_layout.clone(),
            vertex_shader: p.vertex_shader.clone(),
            fragment_shader: p.fragment_shader.clone(),
        }
    }
}

Solution

Derive or implement Clone on all built-in pipeline types. This is easy to do since they mostly just contain cheaply clonable reference-counted types.


Changelog

Implement Clone for all pipeline types.

@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 Nov 17, 2022
Copy link

@Andrewp2 Andrewp2 left a comment

Choose a reason for hiding this comment

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

not an expert here but LGTM

@@ -237,6 +237,18 @@ pub struct MaterialPipeline<M: Material> {
marker: PhantomData<M>,
}

impl<M: Material> Clone for MaterialPipeline<M> {
fn clone(&self) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fn clone(&self) -> Self {
// This needs to be manually implemented, as we do not want to force a M: Clone trait bound
fn clone(&self) -> Self {

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 11, 2023
@alice-i-cecile
Copy link
Member

Left a suggestion for a comment to clarify the pattern used, but it's non-blocking. I'll let the rendering folks (cc @superdump) merge this one though.

Copy link
Contributor

@kurtkuehnert kurtkuehnert left a comment

Choose a reason for hiding this comment

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

Not sure if this will be a common use case, but for small wrapper pipelines, this seems totally reasonable. Do you have a specific example in mind?
LGTM

@SludgePhD
Copy link
Contributor Author

I believe my use case was

impl FromWorld for Wireframe2dPipeline {
    fn from_world(world: &mut World) -> Self {
        let p = &world.resource::<Material2dPipeline<ColorMaterial>>();
        Self {
            mesh2d_pipeline: p.mesh2d_pipeline.clone(),
            material2d_layout: p.material2d_layout.clone(),
            vertex_shader: p.vertex_shader.clone(),
            fragment_shader: p.fragment_shader.clone(),
        }
    }
}

If Material2dPipeline<ColorMaterial> implemented Clone I wouldn't have to pull it apart like that.

@kurtkuehnert
Copy link
Contributor

That makes sense, thank you! Maybe add this to the PR description :).

@SludgePhD
Copy link
Contributor Author

Done

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Jan 14, 2023
# Objective

Pipelines can be customized by wrapping an existing pipeline in a newtype and adding custom logic to its implementation of `SpecializedMeshPipeline::specialize`. To make that easier, the wrapped pipeline type needs to implement `Clone`.

For example, the current non-cloneable pipelines require wrapper pipelines to pull apart the wrapped pipeline like this:

```rust
impl FromWorld for Wireframe2dPipeline {
    fn from_world(world: &mut World) -> Self {
        let p = &world.resource::<Material2dPipeline<ColorMaterial>>();
        Self {
            mesh2d_pipeline: p.mesh2d_pipeline.clone(),
            material2d_layout: p.material2d_layout.clone(),
            vertex_shader: p.vertex_shader.clone(),
            fragment_shader: p.fragment_shader.clone(),
        }
    }
}
```

## Solution

Derive or implement `Clone` on all built-in pipeline types. This is easy to do since they mostly just contain cheaply clonable reference-counted types.

---

## Changelog

Implement `Clone` for all pipeline types.
@bors bors bot changed the title Implement Clone for all pipeline types [Merged by Bors] - Implement Clone for all pipeline types Jan 14, 2023
@bors bors bot closed this Jan 14, 2023
@SludgePhD SludgePhD deleted the cloneable-pipelines branch January 14, 2023 19:17
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
# Objective

Pipelines can be customized by wrapping an existing pipeline in a newtype and adding custom logic to its implementation of `SpecializedMeshPipeline::specialize`. To make that easier, the wrapped pipeline type needs to implement `Clone`.

For example, the current non-cloneable pipelines require wrapper pipelines to pull apart the wrapped pipeline like this:

```rust
impl FromWorld for Wireframe2dPipeline {
    fn from_world(world: &mut World) -> Self {
        let p = &world.resource::<Material2dPipeline<ColorMaterial>>();
        Self {
            mesh2d_pipeline: p.mesh2d_pipeline.clone(),
            material2d_layout: p.material2d_layout.clone(),
            vertex_shader: p.vertex_shader.clone(),
            fragment_shader: p.fragment_shader.clone(),
        }
    }
}
```

## Solution

Derive or implement `Clone` on all built-in pipeline types. This is easy to do since they mostly just contain cheaply clonable reference-counted types.

---

## Changelog

Implement `Clone` for all pipeline types.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

Pipelines can be customized by wrapping an existing pipeline in a newtype and adding custom logic to its implementation of `SpecializedMeshPipeline::specialize`. To make that easier, the wrapped pipeline type needs to implement `Clone`.

For example, the current non-cloneable pipelines require wrapper pipelines to pull apart the wrapped pipeline like this:

```rust
impl FromWorld for Wireframe2dPipeline {
    fn from_world(world: &mut World) -> Self {
        let p = &world.resource::<Material2dPipeline<ColorMaterial>>();
        Self {
            mesh2d_pipeline: p.mesh2d_pipeline.clone(),
            material2d_layout: p.material2d_layout.clone(),
            vertex_shader: p.vertex_shader.clone(),
            fragment_shader: p.fragment_shader.clone(),
        }
    }
}
```

## Solution

Derive or implement `Clone` on all built-in pipeline types. This is easy to do since they mostly just contain cheaply clonable reference-counted types.

---

## Changelog

Implement `Clone` for all pipeline types.
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 S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants