pMessageIdName is always set to nullptr in MVKInstance::debugReportMessage()

[SRSaunders]

When working on macOS/iOS fixes and updates in Sascha Willems' Vulkan examples project, I noticed that MoltenVK's VkDebugUtilsMessengerCallbackDataEXT::pMessageIdName is always set to nullptr in MVKInstance::debugReportMessage(MVKVulkanAPIObject* mvkAPIObj, MVKConfigLogLevel logLevel, const char* pMessage).

Instead it should probably be set to _debugReportCallbackLayerPrefix or getReportingLevelString(logLevel). This would allow the debug utils callback message to indicate the message is coming from MoltenVK via labels like: MoltenVK (the simplest option), or even mvk-error, mvk-warn, etc.

Here is a before and after example... Before: WARNING: [0] : vkCreateMacOSSurfaceMVK() is deprecated. Use vkCreateMetalSurfaceEXT() from the VK_EXT_metal_surface extension. After: WARNING: [0][MoltenVK] : vkCreateMacOSSurfaceMVK() is deprecated. Use vkCreateMetalSurfaceEXT() from the VK_EXT_metal_surface extension. or WARNING: [0][mvk-warn] : vkCreateMacOSSurfaceMVK() is deprecated. Use vkCreateMetalSurfaceEXT() from the VK_EXT_metal_surface extension.

This analagous functionality to the Vulkan Loader, which populates the pMessageIdName field with things like: Loader Message. For example:

ERROR: [0][Loader Message] : loader_validate_device_extensions: Device extension VK_EXT_descriptor_buffer not supported by selected physical device or enabled layers.

I could provide a small PR to add this if you are interested. Please advise.

[billhollings]

From the spec description of pMessageIdName, it seems like they're looking for something more unique or detailed here, in an attempt to specifically identify the error source, not just the error level. I don't think we can provide that.

But I may be over interpreting, and I certainly like your idea of setting pMessageIdName to either MoltenVK or mvk-xxx, to at least provide more helpful info.

So, yes, a PR for this would be great, thanks!

I guess of the two, the mvk-xxx option provides the most info to the user. Probably best to move getReportingLevelString() to MVKInstance, I guess.

[SRSaunders]

Thanks, I will proceed with a PR. One minor question however: If I move getReportingLevelString() to MVKInstance, then the call in MVKBaseObject::reportMessage() becomes:

if (shouldLog && mvkInst) { fprintf(stderr, "[%s] %s\n", mvkInst->getReportingLevelString(logLevel), pMessage); }

Previously it was able to fprintf independently of whether mvkInst was defined or not. If this is a concern to you, I could instead leave getReportingLevelString() where it is now, and pass in the string result as a parameter of debugReportMessage().

Please let me know which approach you prefer. FYI - this issue is why I originally suggested just using _debugReportCallbackLayerPrefix (i.e. "MoltenVK"), which is already defined inside MVKInstance::debugReportMessage().

[billhollings]

If I move getReportingLevelString() to MVKInstance, then the call in MVKBaseObject::reportMessage() becomes: if (shouldLog && mvkInst) { fprintf(stderr, "[%s] %s\n", mvkInst->getReportingLevelString(logLevel), pMessage); }

Ah. Good point.

What we generally do in this case is expose the support function as a stand-alone function. Search for #pragma mark Support functions for examples. They always start with mvk..., so you could rename getReportingLevelString() to mvkGetReportingLevelString(), move it to the bottom of MVKBaseObject.mm, and expose it as a support function in MVKBaseObject.h. Then it can be called from anywhere.

[billhollings]

BTW...no desperate hurry on this one, but if you could get the PR in by Monday (May 6), we can include it in the upcoming SDK release.

[SRSaunders]

Will do - thanks.

PR now submitted with a question. See comments in #2224.