-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Initial DLSS implementation #19864
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
Initial DLSS implementation #19864
Conversation
It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note. Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes. |
@cart not sure what licensing concerns there are with this. See the license notes in https://github.com/JMS55/dlss_wgpu/blob/main/README.md. |
You added a new example but didn't add metadata for it. Please update the root Cargo.toml file. |
Lies D: |
You added a new example but didn't add metadata for it. Please update the root Cargo.toml file. |
Rad! My only concern here is that we're taking a dependency on an external crate that depends on a specific version of wgpu. This will make upstream bevy wgpu updates harder, as it will require collaboration with owner of the crate (in this case, you, which is much better than a stranger, but still a risk).
I think this is ok, given that the onus is on developers to wire up the SDK themselves. |
Theoretically it's an optional feature, so it shouldn't block wgpu upgrades. Of course I'm happy to update it asap. I'm also happy to put it in the bevy repo, up to maintainers. I don't have a preference either way. |
You added a new example but didn't add metadata for it. Please update the root Cargo.toml file. |
As part of your reviews, please take a look at (and leave feedback on https://github.com/bevyengine/dlss_wgpu. We need to be confident in (and ideally have multiple people comfortable working with) both this particular PR and the underlying crate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm working on some changes to remove the need to hard code DLSS into bevy_render
.
In the interim, please try to answer my open questions :)
#[cfg(any(not(feature = "dlss"), feature = "force_disable_dlss"))] | ||
let instance = Instance::new(&instance_descriptor); | ||
|
||
#[cfg(all(feature = "dlss", not(feature = "force_disable_dlss")))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if a different feature needed to enable DX12 specific instance features? Using Instance::from_hal
, at a glance, feels like we're throwing away backend support (or at least, we're throwing away the ability to customize other backends).
Like my previous comment, I don't want to merge until we understand these bounds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, you wouldn't ever use the dlss feature on platforms where you didn't intend to support Vulkan.
We could also add DX12 support to dlss_wgpu, but it's just a lot of extra work for no real gain.
I've pushed my port to #20565 (which includes those changes). This won't build until bevyengine/dlss_wgpu#8 is merged. The diff will be cleaner when #20565 merges. |
# Objective Currently registering additional required Vulkan features requires either hard-coding them into `bevy_render` (see the current [DLSS proposal](#19864)), or forcing the plugin to take full manual control over wgpu initialization. Neither is an acceptable or scalable option for a modular engine like Bevy. ## Solution * Add a new `raw_vulkan_init` Cargo feature, that when enabled switches to wgpu's raw Vulkan init path, which accepts callbacks that allow checking and requiring additional Vulkan features. * Add a new `WgpuRawVulkanInitSettings` resource, which provides wgpu Vulkan Instance and Device init callbacks, which can be used to detect and register vulkan features. * These callbacks can register arbitrary features in the new `AdditionalVulkanFeatures` resource, which is inserted into the RenderApp at the same time that the RenderDevice is. This enables plugins to register initialization callbacks, which must happen _before_ RenderPlugin. They can then feed off of `AdditionalVulkanFeatures`, after the renderer is initialized, to detect if a given feature is supported. Due to the current lifecycles, this is best accomplished with either: * A separate "init plugin" that is registered before RenderPlugin, and a "logic plugin" that does everything else. This should be used if the plugin logic needs `Plugin::build()` access to render device state, which needs to be registered _after_ RenderPlugin to have access. The proposed DLSS feature needs this pattern. * A single "init plugin" that is registered before RenderPlugin and does everything. Use this pattern if you can as it is simpler. With deferred plugin init, we could remove the need for this split. --------- Co-authored-by: JMS55 <47158642+JMS55@users.noreply.github.com>
# Objective For example, in: - #19864 (comment) The link to the directory containing the release notes is broken. ## Solution Fix the link. ## Testing Copy pasted the new link in URL bar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pushed some changes to adapt to the latest raw_vulkan_init API. I think this is in a good spot to land now. DLSS is a bit more cordoned-off. I think we've landed on the right "middle ground" pattern for proprietary APIs like this:
- Separate repo for the bindings to those APIs, with instructions on how to comply with the SDK license requirements.
- Bevy-specific logic in the
bevy
repo, behind an opt-in cargo flag - Do not bake proprietary features into foundational libs (ex:
bevy_render
,bevy_ecs
, etc), even in an opt-in way
Objective
Solution
Testing
Showcase