search

RobertBeckebans / RBDOOM-3-BFG

Doom 3 BFG Edition source port with updated DX12 / Vulkan renderer and modern game engine features
https://www.moddb.com/mods/rbdoom-3-bfg
GNU General Public License v3.0
1.37k stars 244 forks source link

NVRHI is old #853

Closed kablooey123 closed 2 months ago

kablooey123 commented 3 months ago

This project's NVRHI fork is two years old. There's been countless improvements since then.

I know of three incompatibilities; NVRHI headers no longer publicly include vulkan.hpp, upstream FrameBufferInfoEx has no hash template override, and the shader build system has changed. These seem trivial, but I remember there was something going on with MoltenVk.

What are the blockers on this work?

ROCKNROLLKID commented 3 months ago

https://github.com/RobertBeckebans/RBDOOM-3-BFG/pull/749

SRSaunders commented 3 months ago

I have prototyped the merge, and found a few things:

  1. The vulkan.hpp thing was easy to fix, but I had to be careful with VULKAN_HPP_DISPATCH_LOADER_DYNAMIC=1
  2. I didn't face any issues with FrameBufferInfoEx
  3. The new shader build system was the majority of the work: CMakeLists changes, RenderProgs_NVRHI changes, compileshaders.cmake changes (lots of iterations to get this right since some defaults have changed for Vulkan), shaders.cfg changes (shader model moved out to compiler options now), plus a few macOS-specific type, build and parallel compilation issues (had to port my previous PRs into the new stream, will likely offer these upstream with a PR).

I'm not sure if the new shader build system (ShaderMake) is any better than the previous one integrated with nvrhi. It seems like a sideways move with some changes providing less flexibility (e.g. fixed compiler options vs. pass-through to DXC which was more flexible before, and a less-complete shader blob library). However, I suspect nVidia sees a benefit for this to be separate. Just more work on the user side now.

It's up and working now against the current master of RBDoom3BFG (including MoltenVK/macOS). @RobertBeckebans I could contribute some of this if you are interested. Let me know.

RobertBeckebans commented 3 months ago

@SRSaunders does the new NVRHI version have any impact on the performance? I think there is a general flaw in the Vulkan implementation of NVRHI which affects the performance. If you can wrap together your changes for a PR then I would be happy to look at it. ShaderMake was introduced 1 month before I wanted the feature freeze for RBDoom 1.5 so we went out of time to integrate it and seeing if it makes any trouble.

SRSaunders commented 3 months ago

I can do some benchmarking this weekend and report back.

Regarding contributions, I can provide the RBDoom3BFG changes in a PR. However, It might be best if you would fast-forward your nvrhi fork yourself plus open a new fork for ShaderMake. I could then add fixes on those, as well as pushing them upstream.

The nvrhi merge itself was pretty easy. I just had to resolve one conflict to preserve some work from Stephen Pridham.

I have not merged any new stuff from Donut’s DeviceManager. I’m not sure if that’s important or even desirable at this point if you are happy with vulkan 1.2.

RobertBeckebans commented 3 months ago

I looked into this today for several hours. I'm not really sure if I like the split of ShaderMake into another external repo and that it requires Vulkan 1.3 which might break it for some users and especially Linux users who just apt get Vulkan headers on older systems like I do. It seems that the new NVRHI version only adds minor bugfixes and some Nvidia specific Raytracing extensions.

SRSaunders commented 3 months ago

A couple of things I have found:

  1. The split was a bit of a pain, but not that bad once you see the implementation.
  2. ShaderMake does not require Vulkan 1.3 - that is only the default. I have set the implementation to 1.2 but with ShaderModel 6.0, which is the minimum for DXC anyways. Note that DXC was already silently moving the 5.0 setting to 6.0 before these changes. It's just that we couldn't see it due to suppressed messages from the compiler.
  3. Performance of the updated nvrhi does look the same. No major changes on that front.

The reason I have delayed pushing the PR until today was:

  1. I ran into a dependency on the Windows side where ShaderMake requires a minimum Windows SDK version 2104 (10.0.20348.0). Fortunately this is available for Windows 10 at https://go.microsoft.com/fwlink/?linkid=2164145.
  2. I ran into another issue on the Linux side where Vulkan-Headers are installed at a system level by package managers and conflict with the Vulkan-Headers included with nvrhi. This issue shows up as build failures when precompiled headers are enabled for linux. I had to juggle things a bit to limit where <vulkan/vulkan.hpp> was included and change a few vk:: namespace references, but it is all working fine now on Windows, Linux, and macOS. While only a linux issue, it forced me to look closer at the #includes and the implementation is probably more robust now.

I will push a PR later today so you can look at it and decide the path forward.

RobertBeckebans commented 2 months ago

NVRHI is update to date to the newest version now.