Deprecation notice of VkDeviceCreateInfo enabledLayerCount and ppEnabledLayerNames

[charles-lunarg]

(apologies for using the C structure names)

Recently had a user try to enable validation layers through the device. This does nothing, and makes users that are unaware of its uselessness think they are enabling validation.

Maybe it isn't possible, but it would be lovely if there was a 'deprecated' tag for this.

[Ralith]

Sounds reasonable to me!

[MarijnS95]

@charles-lunarg good catch, thanks for bringing this to our attention!

Both the structure and builder are automatically created by our generator; while we can hack in a workaround for these specific fields it'd be much better if vk.xml provides a standard way to communicate this.

For misnamed enum variants for example we currently scan for comment="Backwards-compatible alias containing a typo" and ignore those variants entirely (because we're not able to generate a proper shortened name for it). Perhaps a similar comment attribute could be set for these fields (doesn't seem the case on 1.3.231 yet), or even a more appropriate deprecated= attribute?

For reference, in Rust we have:

#[deprecated = "Friendly message shown to the user"]

This friendly message allows us to explain why something was deprecated, and/or what the alternatives are - we can propagate a useful message directly from vk.xml (quoting https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#extendingvulkan-layers-devicelayerdeprecation, for example Device-only layers are deprecated [..]) 🙂

[MarijnS95]

@charles-lunarg any updates here?

[charles-lunarg]

The WG is unlikely to create a solution deprecation notices anytime soon, so I see two ways to resolve this github issue.

1. Close it as wont-fix, due to preferring a explicit vk.xml tag so that all cases can be handled
2. Hack it in just for this case.

I'm in the 2. camp due to wanting to prevent users from making preventable mistakes, but as I am not the maintainer, that is just my opinion.

Better to have a fix now rather than possibly wait 1-2 years to get a vk.xml change.

[MarijnS95]

I'm fine with 2. as long as we've explored that 1. isn't viable "because upstream™", though I'd still like to push for such a notice sooner rather than later. Even if they don't create official deprecation notices, a simple comment= already allow us to treat it as official, please keep pushing to that effect!

Feel free to make PR that adds the suggested deprecation notice above, or I can do it for you.

[charles-lunarg]

The issue with 1 is that I found discussion around adding deprecated tags, and it isn't clear that will go anywhere anytime soon. I definitely can pursue adding deprecated fields to the VkDeviceCreateInfo members directly, since there isn't any question about whether it is deprecated.

I'm definitely not the first to think about this, internally and externally to the WG.
For instance: KhronosGroup/Vulkan-Docs#1388

[MarijnS95]

That link specifically seems to concern deprecated/superseded extensions, while we're dealing with deprecated fields (and deprecated-but-maintained-for-backwards-compatibility aliases for broken names). Would be great if both could get a deprecated="Reason" (in favour of having to "parse" that out from the comment= field as mentioned above).

[charles-lunarg]

Ah right, extensions being deprecated is different than members of structs.

I think it actually shouldn't be too hard to get buy in for a deprecated= for the struct members in question. I'll propose that in an internal PR. May be a while before it is released, but I'll try to update this issue with its status. (including if it gets summarily closed)

[MarijnS95]

Thanks! Feel free to ping me there too.

[charles-lunarg]

With today's Vulkan-Docs and Vulkan-Headers, this issue should be able to make progress now!

And best of all it benefits every language binding, rather than just ash. Proper 'deprecation' status is very nice to have.

[MarijnS95]

@charles-lunarg see #697; the new deprecated property is already put to use and we'll be able to merge that PR after integrating the new api property too to filter out SC members/definitions etc.

[MarijnS95]

@charles-lunarg #697 now also takes care of generating #[deprecated] annotations on these fields 🎉

As mentioned many times before the generator is getting messier and messier, but we're working on a replacement so don't look too much into the current monstrosity. Do however check if its outputs match your expectation though!

[charles-lunarg]

Looks good to me! Thanks for working on making this change happen in vk.xml, seems like it was a good spot of clean up all around to boot.

[MarijnS95]

It was!

Just noticing now though that we didn't drop one deprecated enum alias for VK_SAMPLER_ADDRESS_MODE_MIRROR_CLAMP_TO_EDGE on the last breaking Ash release (when we dropped everything else because it just made no sense to let the generator come up with sensible names for them, only to annotate it with #[deprecated] pointing to the right member because it's a warning by default that no-one would ever use), meaning I have to make an exemption for it when backporting this to 0.37 :(