Confusing docs and type of DescriptorSetLayoutBinding.immutableSamplers about vulkan HOT 13 CLOSED

I suppose the type could become something like

Either ("descriptorCount" ::: Word32) (Vector Sampler)

from vulkan.

I've gone with 0 because in these cases (where the length is non-optional) 0 is a disallowed value. Additionally it makes zero { stencils = [a, b, c] } still work :)

from vulkan.

Given the lack of next/flags and being a standalone type I think a pack of smart constructors would be preferred way to prevent such an issue.

Either that (heh), or moving to a sum type with more descriptive constructor names. That damned "descriptorCount" has to be referenced somewhere in Haskell code.

from vulkan.

from vulkan.

There's probably something which could be done with RecordDotSyntax, one could have a virtual descriptorCount member (would need some thinking regarding overlap though). (RDS is exciting in general for this library, may be it could be made ergonomic to switch over to storing structs as ByteArray and keeping them always in their C representation (whether this is actually a performance/usability benefit remains to be seen)).

What did you mean by smart constructors?

I don't think that it would be much of a problem to have a separate type data OptionalArray a = JustLength Word32 | Contents (Vector a) (Better name suggestions appreciated!).

from vulkan.

In fact, there are probably plenty of places where documentation on the Haskell->C translation would be useful.

from vulkan.

What did you mean by smart constructors?

One function per descriptorType encoding its valid usage. Perhaps that's the proposal of a higher level library. 🤔

from vulkan.

It appears that making descriptorCount mutually exclusive with immutableSamplers makes combined image sampler unavailable:

If descriptorType is DESCRIPTOR_TYPE_SAMPLER or DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER, and descriptorCount is not 0 and pImmutableSamplers is not NULL,

Maybe the less surprising way to encode is to add descriptorCount field and allow setting it explicitly. At least the docs are there to cover its usage.

from vulkan.

What did you mean by smart constructors?

One function per descriptorType encoding its valid usage. Perhaps that's the proposal of a higher level library. thinking

Yeah, sounds like something for vulkan-utils or similar.

It appears that making descriptorCount mutually exclusive with immutableSamplers makes combined image sampler unavailable:

It's not exclusive though, if immutableSamplers is Right v then descriptorCount is set to length v.

Agree that the currently formulation is confusing. I want to avoid having to burden the programmer with having to set descriptorCount = length immutableSamplers, perhaps a custom sum type as mentioned above, along with documentation along the lines of the following would be best.

data DescriptorSetLayoutBinding = DescriptorSetLayoutBinding
  { blah blah
  , -- | <regular documentation for immutableSamplers, if any>
    --
    -- If 'immutableSamplers' is @'JustLength' l@ then @descriptorCount@ is set 
    -- to the specified length @l@ and @immutableSamplers@ is set to @NULL@. 
    -- If instead it is @'Contents' v@ then @descriptorCount@ is set to @length v@
    -- and the members of @v@ are passed as @immutableSamplers@
    immutableSamplers :: OptionalArray Sampler
  }

There are currently 16 places in the spec where a length can be set, but the array with that length can be NULL.

I think it's particularly confusing in this case as descriptorCount is the "primary" member and immutableSamplers is the "additional" info the programmer may wish to set. Most of the time it's the other way round, the contents are important, and the length is some uninteresting automatable value

from vulkan.

Actually, looking through all the other Either Word32 (Vector _) values in the bindings I think the current behaviour is confusing in most cases.

Maybe the less surprising way to encode is to add descriptorCount field and allow setting it explicitly. At least the docs are there to cover its usage

I think this is the correct thing to do.

from vulkan.

Lol, I can't believe how small that patch was 1ae3c30

from vulkan.

It would be nice to not have to specify lengths if the vector is specified, adb538a this seems like an accident waiting to happen.

Would it be so bad to: if the length is 0, infer it from the length of the vector? If it's non-zero and and doesn't match the non-zero length of the vector, then error.

from vulkan.

I'd go with maxBound value under a pattern instead. It is so outrageously wrong, everyone should complain loudly or explode if code somehow misses its length-detection turn.
But, again, maybe it's too early for such concerns and a simplest dumbest implementation would make a more solid foundation.

from vulkan.