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

Opt-in to Vulkan features we actually use. #81827

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

darksylinc
Copy link
Contributor

Opt-in to the features we actually need/use. These can be changed in the future.
The reasons for this change are:

  1. Certain features (like sparse* stuff) cause unnecessary internal driver allocations.
  2. Others like shaderStorageImageMultisample are a huge red flag (MSAA + Storage is rarely needed).
  3. Most features when turned off aren't actually off (we just promise the driver not to use them) and it is validation what will complain. This allows us to target a minimum baseline.

TODO: Allow the user to override these settings (i.e. turn off more stuff) using profiles
so they can target a broad range of HW. For example Mali HW does not have
shaderClipDistance/shaderCullDistance; thus validation would complain if such feature is used;
allowing them to fix the problem without even owning Mali HW to test on.

Before this PR, Godot was opting in to whatever feature is supported by the HW except for robustBufferAccess.
Had this practice been done earlier #81775 would've been caught much earlier.

Unfortunately Godot exposed certain features (like wideLines & largePoints) that I would've preferred if they remained unexposed by default (GPU support is... suboptimal). They're only well-supported if you're doing CAD stuff with an NV Quadro.

@Calinou
Copy link
Member

Calinou commented Sep 17, 2023

Would this help for #74227?

@Calinou Calinou added this to the 4.x milestone Sep 17, 2023
@darksylinc
Copy link
Contributor Author

Would this help for #74227?

Oh I wish. No, it doesn't. Currently the code checks for it and aborts initialization because it is required.

To fix that issue, a fallback shader & C++ code needs to be written.

Copy link
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

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

Yeah this looks good to me, I didn't give this a lot of thought originally. Originally we actually didn't enable a lot of features we did use and ran into both the validation errors you mentioned, and in some HW crashes when we started using features we never enabled.

While this change doesn't prevent us from enabling features that we use and probably shouldn't, at least it's immediately clear from the code which features we are enabling because we expect to use them if available.

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Sep 18, 2023
@akien-mga akien-mga requested a review from clayjohn September 18, 2023 07:59
@darksylinc darksylinc force-pushed the matias-vkfeatures-opt-in branch from 75af385 to 9084042 Compare September 23, 2023 16:10
@darksylinc darksylinc force-pushed the matias-vkfeatures-opt-in branch from 9084042 to d5f913c Compare September 25, 2023 00:06
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks good!

No need to cherrypick to earlier versions

@akien-mga akien-mga merged commit 68926d5 into godotengine:master Oct 5, 2023
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants