nvpro-samples / vk_raytracing_tutorial_KHR

Ray tracing examples and tutorials using VK_KHR_ray_tracing
Apache License 2.0
1.36k stars 143 forks source link

Couple minor issues / improvements possible #10

Closed BattleAxeVR closed 4 years ago

BattleAxeVR commented 4 years ago

Hi, I noticed the command buffers are only enabled in debug builds, here:

ifdef _DEBUG

for(size_t i = 0; i < m_commandBuffers.size(); i++)
{
  std::string name = std::string("AppBase") + std::to_string(i);
  m_device.setDebugUtilsObjectNameEXT(
      {vk::ObjectType::eCommandBuffer, reinterpret_cast<const uint64_t&>(m_commandBuffers[i]), name.c_str()});
}

endif // _DEBUG

whereas the rest of the code adds the names in Release too. I'd remove the #ifdef _DEBUG and use the appropriate DebugUtils function for command buffers, like this:

for(size_t i = 0; i < m_commandBuffers.size(); i++) { std::string name = std::string("AppBase") + std::to_string(i); m_debug.setObjectName(m_commandBuffers[i], name); }

This util function also checks s_enabled whereas the above code doesn't.

A couple other places I've noticed are redundant in the KHR basic sample, like one could call setViewport from AppBase (passing in the command buffer ref) instead of manually calling setViewPort/setScissors combo each time.

There are some other general things I could suggest, like renaming all the individual HelloVK classes to reflect the name of their respective samples, get rid of all the static variables, put the context struct as a member variable in the base class, with the derived class setting the required extensions during a virtual init method...this way you could build all the samples with unique class names and stuff them all in a single project, and even switch between them at runtime like the older DX11 samples do, so you can skim through all the samples individually in a single session. It would also make sure all the init / deinit stuff is done properly as you switch sessions, or even re-use the same base vulkan context and only init / reinit stuff for the derived classes, unless the required extensions are different.

I don't understand why both the Nvpro and Vulkan C samples use HelloVK as the class name for each sample. Call the sample's what it is and what it does explicitly (E.g. RayTracingBasic), and keep all their specific state / extensions in the class itself since that's the point of using classes, to encapsulate the requirements so it's self-contained. IMO it's not ideal to set the extensions externally the context prior to initializing it. I also prefer not duplicating raw pointers like m_device.setup(device) in various places, and instead pass references to what's needed in any member vars. Duplicating raw pointers to things can lead to double deinit errors, whereas if you pass a ref to a constructor in each member var that needs those things (like m_debug or m_device), you know they're all accessing the same base vulkan pointers everywhere and there's no chance of them being modified.

Just a few suggestions, take'em or leave'em! Btw great job on the VK debug functionality with NSight Graphics, super polished and easy to use.

mklefrancois commented 4 years ago

Thanks you for the time of writing those comments. I hope the samples and tutorials were useful. Some more advance or complete samples exists under https://github.com/nvpro-samples.