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] - bevy_render: Support overriding wgpu features and limits #3912

Closed
wants to merge 1 commit into from

Conversation

superdump
Copy link
Contributor

Objective

Solution

  • Add disabled_features: Option<wgpu::Features> to WgpuOptions
  • Add constrained_limits: Option<wgpu::Limits> to WgpuOptions
  • After maybe obtaining updated features and limits from the adapter/backend in the case of WgpuOptionsPriority::Functionality, enable the WgpuOptions features, disable the disabled_features, and constrain the limits by constrained_limits.
    • Note that constraining the limits means for wgpu::Limits members named max_.* we take the minimum of that which was configured/queried for the backend/adapter and the specified constrained limit value. This means the configured/queried value is used if the constrained limit is larger as that is as much as the device/API supports, or the constrained limit value is used if it is smaller as we are imposing an artificial constraint. For members named min_.* we take the maximum instead. For example, a minimum stride might be 256 but we set constrained limit value of 1024, then 1024 is the more conservative value. If the constrained limit value were 16, then 256 would be the more conservative.

Supports explicitly enabling/disabling features, and constraining limits to
'less' than what they could be.
@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Feb 10, 2022
@superdump superdump added A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Feb 10, 2022
@superdump superdump requested a review from cart February 10, 2022 23:03
@superdump superdump self-assigned this Feb 10, 2022
@superdump superdump added this to the Bevy 0.6.1 milestone Feb 10, 2022
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

I think this is a solid solution to the problem!

features -= disabled_features;
}
// NOTE: |= is used here to ensure that any explicitly-enabled features are respected.
options.features |= features;
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking that we should make WgpuOptions a "read only" configuration item. Users shouldn't be reading these settings to get "actual" values, they should be querying the device. And writing on top like this makes it hard to talk about what these values mean. Buuuut given that these changes are going into a patch release, we should probably hold off on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking from the beginning that it’s a bit sketchy to reuse WgpuOptions to present the configured state after using it but it also isn’t used after init otherwise.

As regards this specific thing… we could have a private field with the final result or a separate resource so that WgpuOptions is used for configuration and WgpuCapabilities or some good name is what you can actually use. Both practically read only. But how do you even mark something as read-only? I didn’t know you could?

Copy link
Member

Choose a reason for hiding this comment

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

In terms of "read only config", that ties into the conversation here: #2988
At the very least, I think it makes sense to "generally" treat plugin settings as "immutable after insertion", even though we don't have that functionality yet.

Copy link
Contributor

@molikto molikto Feb 13, 2022

Choose a reason for hiding this comment

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

plz also consider making plugin settings freeze when read https://discord.com/channels/691052431525675048/692572690833473578/941182628499947520

This means you can:

  • reconfigure settings before it is read by plugin
  • disallow changing settings after it is read, so avoiding user confusion
  • plugin can change to "reactive" settings when it want to support it, without changing it's API

Also compare to the proposed changes, this is a very minimal change (requires add world.freeze_resource)

@cart
Copy link
Member

cart commented Feb 13, 2022

bors r+

bors bot pushed a commit that referenced this pull request Feb 13, 2022
# Objective

- Support overriding wgpu features and limits that were calculated from default values or queried from the adapter/backend.
- Fixes #3686

## Solution

- Add `disabled_features: Option<wgpu::Features>` to `WgpuOptions`
- Add `constrained_limits: Option<wgpu::Limits>` to `WgpuOptions`
- After maybe obtaining updated features and limits from the adapter/backend in the case of `WgpuOptionsPriority::Functionality`, enable the `WgpuOptions` `features`, disable the `disabled_features`, and constrain the `limits` by `constrained_limits`.
  - Note that constraining the limits means for `wgpu::Limits` members named `max_.*` we take the minimum of that which was configured/queried for the backend/adapter and the specified constrained limit value. This means the configured/queried value is used if the constrained limit is larger as that is as much as the device/API supports, or the constrained limit value is used if it is smaller as we are imposing an artificial constraint. For members named `min_.*` we take the maximum instead. For example, a minimum stride might be 256 but we set constrained limit value of 1024, then 1024 is the more conservative value. If the constrained limit value were 16, then 256 would be the more conservative.
@bors bors bot changed the title bevy_render: Support overriding wgpu features and limits [Merged by Bors] - bevy_render: Support overriding wgpu features and limits Feb 13, 2022
@bors bors bot closed this Feb 13, 2022
cart pushed a commit that referenced this pull request Feb 13, 2022
# Objective

- Support overriding wgpu features and limits that were calculated from default values or queried from the adapter/backend.
- Fixes #3686

## Solution

- Add `disabled_features: Option<wgpu::Features>` to `WgpuOptions`
- Add `constrained_limits: Option<wgpu::Limits>` to `WgpuOptions`
- After maybe obtaining updated features and limits from the adapter/backend in the case of `WgpuOptionsPriority::Functionality`, enable the `WgpuOptions` `features`, disable the `disabled_features`, and constrain the `limits` by `constrained_limits`.
  - Note that constraining the limits means for `wgpu::Limits` members named `max_.*` we take the minimum of that which was configured/queried for the backend/adapter and the specified constrained limit value. This means the configured/queried value is used if the constrained limit is larger as that is as much as the device/API supports, or the constrained limit value is used if it is smaller as we are imposing an artificial constraint. For members named `min_.*` we take the maximum instead. For example, a minimum stride might be 256 but we set constrained limit value of 1024, then 1024 is the more conservative value. If the constrained limit value were 16, then 256 would be the more conservative.
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-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

WgpuOptionsPriority::Functionality overwrites app preferences with suboptimal configuration
3 participants