Allow other plugins to create renderer resources #9632

awtterpip · 2023-08-29T19:47:57Z

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 RenderSettings enum instead of WgpuSettings. RenderSettings::default() returns RenderSettings::Automatic(WgpuSettings::default()).

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

// now
RenderPlugin {
    render_settings: RenderSettings::Automatic(WgpuSettings {
    ...
    }),
}

github-actions · 2023-08-29T19:48:09Z

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

JMS55

I'm for these changes overall. It'll also make DLSS nicer when I get around to implementing that again :)

I do have some small code refactor suggestions that would be nice, if you don't mind doing them while you're here.

crates/bevy_render/src/lib.rs

alice-i-cecile

I think we can and should make the API better, by swapping to a "field on RenderPlugin" strategy.

More importantly though, the extracted function needs to be unsafe.

awtterpip · 2023-09-08T19:37:24Z

i believe i have implemented a better way to accomplish this

crates/bevy_render/src/lib.rs

crates/bevy_render/src/settings.rs

github-actions · 2023-09-08T21:30:24Z

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

alice-i-cecile · 2023-09-08T21:30:37Z

I like the new initialization pattern a lot! Once there's a simple migration guide plus a SAFETY comment, this LGTM.

awtterpip · 2023-09-08T22:01:20Z

where should i write the migration guide?

paul-hansen · 2023-09-08T23:18:53Z

It goes in the PR description, here's an example from one of mine: #9516

The migration guides get created from a script that scrapes these from the PRs so make sure the header is exact. E.g.

## Migration Guide

Instructions for migrating code that will break

crates/bevy_render/src/settings.rs

superdump · 2023-09-09T05:44:23Z

I want to carefully review this before it is merged. What is the reason for needing to provide the ‘future render resources’ from outside bevy? What configurability is missing?

awtterpip · 2023-09-09T21:25:26Z

@superdump so the solution ended up actually changing. originally, i made FutureRendererResources a public struct, and i made the RenderPlugin skip creating the render resources if this already existed, this would allow other plugins to create these resources themselves. however, the solution has changed to making the RenderPlugin be able to take precreated render resources to prevent the resources being able to be edited at runtime. An example of where this would be needed is my openxr plugin for bevy. openxr has a very specific process to initalize render resources, requiring me to make these resources myself and then give them to the bevy RenderPlugin

JMS55 · 2023-09-09T21:27:42Z

What is the reason for needing to provide the ‘future render resources’ from outside bevy? What configurability is missing?

It's mainly for importing rendering resources from outside of bevy. For instance, with XR, you have a specific vulkan resource, that you then map to a wgpu resource in a specific manner. For DLSS, I needed to use raw vulkan to load some extra nvidia-dlss-only extensions (so, stuff wgpu would never get) as part of device creation, and then create a wgpu device wrapping it.

It's the kind of stuff bevy_render is never going to handle itself, for very specific platforms with custom rendering devices.

awtterpip · 2023-09-09T21:47:30Z

i believe that is a satisfactory migration guide. if it needs improving please let me know

patchedsoul · 2023-09-13T20:28:37Z

Is there anything else holding this up now?

alice-i-cecile · 2023-09-13T22:45:22Z

Waiting on final review from @superdump :) It's in the 0.12 milestone, so it will be considered.

mockersf · 2023-09-13T23:26:49Z

crates/bevy_render/src/settings.rs

+        RenderQueue,
+        RenderAdapterInfo,
+        RenderAdapter,
+        Mutex<Instance>,


the mutex here is to allow interior mutability, which breaks the assumption that setting up a plugin doesn't mutate it

(Not a bevy maintainer): from what I could tell, all methods on wgpu::Instance are on a shared ref, and the type is Send and Sync. Is there a reason that cloning a Arc<wgpu::Instance> cannot be used instead of taking from a Mutex<Instance>?

@mockersf my most recent commit should have fixed this. i implemented what @TheButlah recommended

awtterpip · 2023-09-26T00:12:12Z

i had to rebuild my pr since i was having some merge conflicts and forgot to make a new branch when i started making this pr, however, github won't let me reopen this PR without the original branch, so i had to create a duplicate of this pr here: #9925

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](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 <[email protected]> Co-authored-by: Robert Swain <[email protected]>

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 <[email protected]> Co-authored-by: Robert Swain <[email protected]>

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 O-XR Specific to virtual and augmented reality platforms labels Aug 29, 2023

alice-i-cecile modified the milestones: 0.11.2, 0.12 Aug 29, 2023

JMS55 suggested changes Aug 30, 2023

View reviewed changes

crates/bevy_render/src/lib.rs Outdated Show resolved Hide resolved

crates/bevy_render/src/lib.rs Outdated Show resolved Hide resolved

crates/bevy_render/src/lib.rs Outdated Show resolved Hide resolved

JMS55 approved these changes Sep 1, 2023

View reviewed changes

JMS55 requested a review from alice-i-cecile September 1, 2023 05:06

mockersf approved these changes Sep 6, 2023

View reviewed changes

paul-hansen reviewed Sep 7, 2023

View reviewed changes

crates/bevy_render/src/lib.rs Outdated Show resolved Hide resolved

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 Sep 8, 2023

alice-i-cecile reviewed Sep 8, 2023

View reviewed changes

crates/bevy_render/src/lib.rs Outdated Show resolved Hide resolved

alice-i-cecile requested changes Sep 8, 2023

View reviewed changes

awtterpip closed this Sep 8, 2023

awtterpip reopened this Sep 8, 2023

awtterpip closed this Sep 8, 2023

awtterpip reopened this Sep 8, 2023

awtterpip requested a review from alice-i-cecile September 8, 2023 19:39

alice-i-cecile requested changes Sep 8, 2023

View reviewed changes

crates/bevy_render/src/lib.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Sep 8, 2023

View reviewed changes

crates/bevy_render/src/settings.rs Outdated Show resolved Hide resolved

alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Sep 8, 2023

mockersf requested review from mockersf and JMS55 September 8, 2023 23:31

mockersf removed 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 Sep 8, 2023

mockersf reviewed Sep 8, 2023

View reviewed changes

crates/bevy_render/src/settings.rs Outdated Show resolved Hide resolved

mockersf self-requested a review September 8, 2023 23:35

superdump self-requested a review September 9, 2023 05:43

awtterpip requested a review from alice-i-cecile September 9, 2023 21:20

alice-i-cecile approved these changes Sep 9, 2023

View reviewed changes

MalekiRe approved these changes Sep 10, 2023

View reviewed changes

mockersf reviewed Sep 13, 2023

View reviewed changes

awtterpip closed this Sep 25, 2023

awtterpip mentioned this pull request Sep 26, 2023

Allow other plugins to create renderer resources #9925

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow other plugins to create renderer resources #9632

Allow other plugins to create renderer resources #9632

awtterpip commented Aug 29, 2023 •

edited

Loading

github-actions bot commented Aug 29, 2023

JMS55 left a comment •

edited

Loading

alice-i-cecile left a comment

awtterpip commented Sep 8, 2023

github-actions bot commented Sep 8, 2023

alice-i-cecile commented Sep 8, 2023

awtterpip commented Sep 8, 2023

paul-hansen commented Sep 8, 2023 •

edited

Loading

superdump commented Sep 9, 2023

awtterpip commented Sep 9, 2023

JMS55 commented Sep 9, 2023

awtterpip commented Sep 9, 2023

patchedsoul commented Sep 13, 2023

alice-i-cecile commented Sep 13, 2023

mockersf Sep 13, 2023

TheButlah Sep 20, 2023 •

edited

Loading

awtterpip Sep 20, 2023

awtterpip commented Sep 26, 2023

Allow other plugins to create renderer resources #9632

Allow other plugins to create renderer resources #9632

Conversation

awtterpip commented Aug 29, 2023 • edited Loading

Objective

Solution

Migration Guide

github-actions bot commented Aug 29, 2023

JMS55 left a comment • edited Loading

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

awtterpip commented Sep 8, 2023

github-actions bot commented Sep 8, 2023

alice-i-cecile commented Sep 8, 2023

awtterpip commented Sep 8, 2023

paul-hansen commented Sep 8, 2023 • edited Loading

superdump commented Sep 9, 2023

awtterpip commented Sep 9, 2023

JMS55 commented Sep 9, 2023

awtterpip commented Sep 9, 2023

patchedsoul commented Sep 13, 2023

alice-i-cecile commented Sep 13, 2023

mockersf Sep 13, 2023

Choose a reason for hiding this comment

TheButlah Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

awtterpip Sep 20, 2023

Choose a reason for hiding this comment

awtterpip commented Sep 26, 2023

awtterpip commented Aug 29, 2023 •

edited

Loading

JMS55 left a comment •

edited

Loading

paul-hansen commented Sep 8, 2023 •

edited

Loading

TheButlah Sep 20, 2023 •

edited

Loading