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

Allow other plugins to create renderer resources #9925

Merged
merged 5 commits into from
Sep 26, 2023
Merged

Allow other plugins to create renderer resources #9925

merged 5 commits into from
Sep 26, 2023

Conversation

awtterpip
Copy link
Contributor

@awtterpip awtterpip commented Sep 26, 2023

This is a duplicate of #9632, it was created since I forgot to make a new branch when I first made this PR, so I was having trouble resolving merge conflicts, meaning I had to rebuild my PR.

Objective

  • Allow other plugins to create the renderer resources. An example of where this would be required is my OpenXR plugin

Solution

  • Changed the bevy RenderPlugin to optionally take precreated render resources instead of a configuration.

Migration Guide

The RenderPlugin now takes a RenderCreation enum instead of WgpuSettings. RenderSettings::default() returns RenderSettings::Automatic(WgpuSettings::default()). RenderSettings also implements From<WgpuSettings>.

// before
RenderPlugin {
    wgpu_settings: WgpuSettings {
    ...
    },
}

// now
RenderPlugin {
    render_creation: RenderCreation::Automatic(WgpuSettings {
    ...
    }),
}
// or
RenderPlugin {
    render_creation: WgpuSettings {
    ...
    }.into(),
}

@mockersf
Copy link
Member

works for me on macOS, iOS, Android, WebGL2 and WebGPU

@mockersf mockersf added the A-Rendering Drawing game state to the screen label Sep 26, 2023
@mockersf mockersf requested a review from superdump September 26, 2023 06:29
@mockersf mockersf added the O-XR Specific to virtual and augmented reality platforms label Sep 26, 2023
@@ -93,6 +96,45 @@ impl Default for WgpuSettings {
}
}

/// An enum describing how the renderer will initialize resources. This is used when creating the [`RenderPlugin`](crate::RenderPlugin).
pub enum RenderSettings {
Copy link
Contributor

Choose a reason for hiding this comment

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

The manual mode doesn't contain settings. @mockersf suggested calling it RenderCreation instead. I much prefer that. With manual creation you provide the handles. With automatic, you provide the settings.

And I think maybe the Default trait can be derived if you annotate the Automatic variant with #[default], right? If not, keep the manual implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the #[default] attribute can only be used on unit variants

Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

Just the renaming of RenderSettings to RenderCreation and this is good to go I think.

@awtterpip awtterpip requested a review from superdump September 26, 2023 17:33
Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

Still one minor variable name change that was missed.

Co-authored-by: Robert Swain <robert.swain@gmail.com>
@superdump superdump enabled auto-merge September 26, 2023 19:10
auto-merge was automatically disabled September 26, 2023 19:15

Head branch was pushed to by a user without write access

@superdump superdump enabled auto-merge September 26, 2023 19:32
@superdump superdump added this pull request to the merge queue Sep 26, 2023
Merged via the queue into bevyengine:main with commit bc88f33 Sep 26, 2023
21 checks passed
@patchedsoul
Copy link

I could be wrong, but should the migration guide be updated to reflect RenderCreation vs RenderSettings?

rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
This is a duplicate of bevyengine#9632, it was created since I forgot to make a
new branch when I first made this PR, so I was having trouble resolving
merge conflicts, meaning I had to rebuild my PR.

# Objective

- Allow other plugins to create the renderer resources. An example of
where this would be required is my [OpenXR
plugin](https://github.com/awtterpip/bevy_openxr)

## Solution

- Changed the bevy RenderPlugin to optionally take precreated render
resources instead of a configuration.

## Migration Guide

The `RenderPlugin` now takes a `RenderCreation` enum instead of
`WgpuSettings`. `RenderSettings::default()` returns
`RenderSettings::Automatic(WgpuSettings::default())`. `RenderSettings`
also implements `From<WgpuSettings>`.

```rust
// before
RenderPlugin {
    wgpu_settings: WgpuSettings {
    ...
    },
}

// now
RenderPlugin {
    render_creation: RenderCreation::Automatic(WgpuSettings {
    ...
    }),
}
// or
RenderPlugin {
    render_creation: WgpuSettings {
    ...
    }.into(),
}
```

---------

Co-authored-by: Malek <pocmalek@gmail.com>
Co-authored-by: Robert Swain <robert.swain@gmail.com>
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 O-XR Specific to virtual and augmented reality platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants