JuliaGPU / Vulkan.jl

Using Vulkan from Julia
Other
109 stars 12 forks source link

Better design for (logging) flags #13

Closed Seelengrab closed 3 years ago

Seelengrab commented 3 years ago

Follow up discussion for #11, how should comparisons of the form

(A | B) >= A

be handled for bitmasks? The context this came up in was logging severity.

Also worth thinking about - should type flags work more like an enum that shouldn't be possible to combine with |? Does that make sense in the context of Vulkan? I.e, should the fact that bitmasks/flags are implemented as a UInt32 be purely an implementation detail?

serenity4 commented 3 years ago

Also worth thinking about - should type flags work more like an enum that shouldn't be possible to combine with |? Does that make sense in the context of Vulkan? I.e, should the fact that bitmasks/flags are implemented as a UInt32 be purely an implementation detail?

Regarding that part, combining flags makes sense. See for example this use case where

IMAGE_USAGE_COLOR_ATTACHMENT_BIT | IMAGE_USAGE_TRANSFER_SRC_BIT

is used to indicate that an image can be used as a color attachment or a source for data transfer.

serenity4 commented 3 years ago

Regarding logging, there are a few functions defined here. Some people may want to turn off warnings and leave info/debug levels on maybe, I think we shouldn't try to restrict that in the core wrapper, more so in higher-level wrappers.

Seelengrab commented 3 years ago

I see - so it makes sense at least for some flags, but not necessarily all. E.g. the docs for DEBUG_UTILS_MESSAGE_TYPE_GENERAL_BIT_EXT seem to suggest that these flags are exclusive, but neither they nor the specs seem to give such a guarantee..

I think we shouldn't try to restrict that in the core wrapper, more so in higher-level wrappers.

Agreed - so maybe it's best to just document it.

serenity4 commented 3 years ago

I see - so it makes sense at least for some flags, but not necessarily all. E.g. the docs for DEBUG_UTILS_MESSAGE_TYPE_GENERAL_BIT_EXT seem to suggest that these flags are exclusive, but neither they nor the specs seem to give such a guarantee..

What do you mean by exclusive? They can actually be combined, though we can't really define an order between flags themselves.

Seelengrab commented 3 years ago

Conceptually, does DEBUG_UTILS_MESSAGE_TYPE_GENERAL_BIT_EXT | DEBUG_UTILS_MESSAGE_TYPE_VALIDATION_BIT_EXT even make sense? That's what I mean with "are these flags exclusive". That would say that the given debug message is both general and from validation, which the doc seems to softly reject as invalid:

VK_DEBUG_UTILS_MESSAGE_TYPE_GENERAL_BIT_EXT specifies that some general event has occurred. This is typically a non-specification, non-performance event.

This typically really isn't decisive...

Seelengrab commented 3 years ago

Well, never mind - these kinds of combinations are supposed to be implicitly valid:

https://www.khronos.org/registry/vulkan/specs/1.2-extensions/html/vkspec.html#VUID-VkDebugUtilsMessengerCreateInfoEXT-pfnUserCallback-01914

messageType must be a valid combination of VkDebugUtilsMessageTypeFlagBitsEXT values

Though the spec seems to be silent on just what precisely a "valid combination" constitutes.

serenity4 commented 3 years ago

Well, VK_DEBUG_UTILS_MESSAGE_TYPE_GENERAL_BIT_EXT in itself is for non-specification, non-performance events, but it does not mean that it is to be used alone. For example, if you want all kinds of messages (validation, performance, general), you just take the combination of all three.

serenity4 commented 3 years ago

The more I think about it, the more I believe that we shouldn't commit to anything more for <. It is now possible from #11 to compare bitmasks the same way as would be done for integer bitmasks. For some types it may make sense (like logging severity), but most often it doesn't, and we have no way of knowing when it does (it's not specified anywhere).

For logging specifically, we already provide a high-level function that leverages this order, anything further than that should be done in application code IMO.

Seelengrab commented 3 years ago

Yeah - unless we go with a very high level route, integrating Vulkan log levels into julia LogLevels, I don't think we should do this. This is the low level(ish) wrapper after all.