Repository navigation
Conversation
ghost
left a comment
There was a problem hiding this comment.
Thanks, this is a lot of careful work, and putting the model inside the upscale chain (before the HUD) is the right fix for the HUD smudge. I read this PR along with the parts of #1831 and #1834 that wire it in, since this PR can't be judged on its own. Several things need sorting before it can land:
This PR can't merge on its own. nx2/build.py on main compiles every gpu/vk/*.cpp, so the four new .cpp files get built right away. vk_dlssnr_ngx.cpp then fails to compile with no NGX SDK: asked() and nr:: use nr_settings() and opts.nr_backend, which only #1834 adds. vk_opennr.cpp needs opennr_ptx.h (generated by #1834's build.py) and vk_camera_motion.h / scene_frame_inputs (from #1831). Please mark it as depending on those two, or fold the three into one PR. As things stand, merging them in any other order breaks every build.
Tests. main's tests/run_all.py retired the central ARGS/SKIP tables. Each test now declares RUN_ALL_ARGS / RUN_ALL_SKIP in its own file, and any entry added to the old tables stops the whole run ("ARGS, MODULES and SKIP are retired"). #1834 adds the opennr tests to those retired tables, and these test files declare nothing. So test_opennr_kernels/harness/khr.py run with no arguments and fail on their required=True args, and test_nr_settings_live.py / test_opennr_backend.py fail on main without #1834's vk_menu.h and vk_main.cpp. CI hasn't run on the branch ("no checks reported").
Two threads, one unlocked state. nr::apply is now called from temporal_upscale (the replay thread, with the chain's lock held) and also from present_image (the display thread). Only dlss_nr_before_hud()'s frame window keeps them apart. On the first chain frame after a gap (2 frames in the default stage, 600 before the upscaler), both threads can be inside dlssnr_ngx::apply / opennr::apply at once. Neither has a lock. They share one NVSDK_NGX_Parameter block (each points it at its own stack NVSDK_NGX_Resource_VKs) and one set of images. If the two sizes differ (render size vs window size in Before upscaler mode), each call frees and rebuilds the feature and images the other thread just recorded. vk_dlssnr.cpp's resize comment ("apply() runs on the display thread") no longer holds.
Compared with #1590:
- Avoided: there is no DLSS SR history reset tied to NR. Refusals are held against the exact settings, so there's no once-a-second pulse and no per-frame reset with HDR Native. The Neural tab being shown in every build is justified here because the Open backend is in every build.
- Still the same:
- The NGX feature (and the Open graph) is rebuilt, with a full device idle, on every size change. With Resolution Auto in Before upscaler mode, every render-size change stalls, and six refusals turn NR off for the run.
- Nothing frees NR's VRAM when it's turned off. No
shutdown()of the three backends is called anywhere in the four PRs, so none is called even at exit. Separately,opennr'scheckerstd::thread can be left joinable in a static at exit (inline comment). - The model gets jitter 0, no motion and a flat depth. In the new Before upscaler stage that's worse: it gets jittered render-size colour while the real
s.depth,s.motionandcam.jitterare right there intemporal_upscale. If the model is temporal, it will average the jitter away before DLSS sees it ("slightly softer on fine foliage" fits that) and ghost on camera motion.
- On the overall approach, this PR is the better base: the in-chain placement, the separation from SR history, and the explained refusals. The two can't both merge, since both add a DLSS 5 tab and the feature-18 path. I'd suggest maintainers pick this design and close #1590 once it lands. This PR would be much easier to review if trimmed to NGX direct plus the in-chain placement first, with Streamline and the vendored OpenDLSS-NR (~50k lines, ~2.8 MB of generated SPIR-V headers) as follow-ups.
Other notes:
- #1473 (recorder thread) is still open, not merged, so
B_DIRECTdoesn't exist on main yet. If it lands first, every NGX create/evaluate here and the directvkCmd*in these files needrec::boundary(B_DIRECT). Itstest_recorder_sync.pywould flag these files. - I checked env names and option values against #1008 and #1773. No collisions: the
RX_VK_DLSS_NR*names are distinct,RX_VK_DLSS=0deliberately turns off NGX NR too, and NR's quality values are separate fromopts.dlss3/4/5. This PR adds no numeric option ids (the Neural rows in #1834 are keyed by name). - Docs and maintainer call: the NGX path documents running a community-patched
nvngx_dlssnr.dllwith NGX's signature override turned on. The Open backend needs a model folder that can only come from NVIDIA's weights. Maintainers should decide whether they want to document either. - Nit: the Before upscaler path takes the 16F scene colour down to RGBA8 and back into the 16F colour input, an 8-bit hop before DLSS.
| /// reach the next frame (tests/test_nr_settings_live.py says why that matters here). | ||
| bool asked() { | ||
| if (const char* e = getenv("RX_VK_DLSS_NR")) return atoi(e) != 0; | ||
| return nr_settings().on; |
There was a problem hiding this comment.
This (and nr::asked_backend reading opts.nr_backend below) is compiled in every build, with or without NGX, and nr_settings() / opts.nr_backend only exist after #1834. Since build.py globs gpu/vk/*.cpp, this PR alone breaks the build on main.
| VmaAllocation memory{}; | ||
| }; | ||
|
|
||
| struct State { |
There was a problem hiding this comment.
There's no lock on this state, and apply now has two callers on different threads: temporal_upscale (replay thread, Before upscaler / before HUD) and present_image (display thread). Only dlss_nr_before_hud()'s frame window keeps them apart, and on the first chain frame after a gap both can be in here at once. They share params (each points it at its own stack resources) and in/out, and when the sizes differ, ensure_feature / resize free what the other thread just recorded. A mutex around apply, or one owner per stage, would fix it. vk_opennr.cpp has the same problem.
| /// The feature, made for this size, preset and quality: those are creation parameters, so a change of any of them makes | ||
| /// it again. Creation records into `cmd`. | ||
| bool ensure_feature(State& s, VkCommandBuffer cmd, const NrSettings& want, uint32_t w, uint32_t h) { | ||
| if (s.feature && s.feature_w == w && s.feature_h == h && s.feature_preset == want.preset && |
There was a problem hiding this comment.
Same as #1590: any size change releases the feature and recreates it, and release_feature / resize each do a full device idle. In Before upscaler mode with Resolution Auto, that's a stall on every render-size change. Each refusal is also held against the size, and after six NR is off for the run. Could the feature be created at the max render size and evaluated with subrects, or at least kept for a size seen recently?
|
|
||
| VkImage apply(VkCommandBuffer cmd, VkImage src, uint32_t sw, uint32_t sh, VkImageView* out_view) { | ||
| State& s = state(); | ||
| if (!src || !sw || !sh || !asked()) return VK_NULL_HANDLE; |
There was a problem hiding this comment.
When NR is turned off (or another backend is picked), this returns before anything is released. The feature, the parameter block and four full-size images stay allocated for the rest of the run. No shutdown() of the three backends is called anywhere in this series, so none is called at exit either.
| // (vk_dlssnr.cpp scene_constants). | ||
| NVSDK_NGX_Parameter_SetF(p, NVSDK_NGX_Parameter_Jitter_Offset_X, 0.0f); | ||
| NVSDK_NGX_Parameter_SetF(p, NVSDK_NGX_Parameter_Jitter_Offset_Y, 0.0f); | ||
| NVSDK_NGX_Parameter_SetF(p, "DLSSNR.JitterOffsetX", 0.0f); |
There was a problem hiding this comment.
Fine at the present, but #1831 now also calls this from temporal_upscale on the jittered render-size scene (Before upscaler). The model is told there is no jitter and no motion, and gets a flat depth, while the real s.depth, s.motion and cam.jitter are right there. If the model keeps history, it will average out the jitter DLSS needs and ghost on camera motion. Can that path pass the real inputs, or at least reset on cuts?
| s.check_done = false; | ||
| s.phase = kChecking; | ||
| if (s.checker.joinable()) s.checker.join(); | ||
| s.checker = std::thread([&s] { |
There was a problem hiding this comment.
This thread is joined only on a later apply with NR on (the kChecking branch) or in shutdown(), and nothing calls shutdown(). If the row is turned off while the check runs, apply returns early from then on, so the std::thread stays joinable in the static State. Its destructor at exit then calls std::terminate.
|
|
||
| def main(): | ||
| p = argparse.ArgumentParser() | ||
| p.add_argument("--glslang", required=True) |
There was a problem hiding this comment.
main's run_all.py has retired the ARGS/SKIP tables (#1834 adds these tests there, which stops the whole run). Each test now declares RUN_ALL_ARGS = ["--glslang", "{glslang}", "--spirv-val", "{spirv_val}"] and RUN_ALL_SKIP = {...} at top level. Without that, this test runs with no args and fails here. The same applies to test_opennr_harness.py and test_opennr_khr.py.
…SS frame generation (NGX), FSR 4 fallback (builds with NotRexed#1833 and NotRexed#1834) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3934125 to
7c806d5
Compare
…s; lock diagnostics (builds with NotRexed#1831 and NotRexed#1833) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks; this review found real bugs. I rebuilt the series on main at d7a7306, and the description is rewritten.
|
ghost
left a comment
There was a problem hiding this comment.
Thanks for the thorough follow-up. Most of what I raised is fixed, but the ground under this PR moved today, so it can't land as it stands.
Fixed since my last review:
- Two threads, one state:
nr::applynow takesapply_mu, and the lock order (chain, thenapply_mu, then the backend's own) holds in the code. - VRAM: the backend that stops running is now released from
nr::apply(row off or another backend picked). - The checker thread no longer reaches
std::terminateat exit (one nit inline). - Real inputs before the upscaler:
NrFramecarries depth, motion, jitter and the cut reset, and both NGX and Open use them. - Tests: the three opennr tests declare
RUN_ALL_ARGS/RUN_ALL_SKIP, and the placeholders (cxx,vulkan_headers,glslang,spirv_val) are ones main'srun_all.pyprovides. - #1473:
vk_work.hcomes last, withB_DIRECTbefore NGX's, Streamline's and the network's own recording.
Answered, kept as known limits: the rebuild with a device idle on every size change, and the RGBA8 hop. Both are listed in the description, which is fine for an off-by-default feature.
Still open, and now blocking: the series no longer has a base. #1831 was closed today in favour of #2118 (DLSS-G through NGX, merged). This PR still relies on things only #1831 added:
vk_dlssnr_ngx.cppneedsdlss_ngx_session()/dlss_ngx_runtime()fromvk_dlss.handvk_ngx.h. Main'svk_dlss.hhas neither, andvk_ngx.hdoesn't exist. #2118's NGX session lives invk_dlss.cpp's own state (ngx_init(), file-local).vk_opennr.cppincludesvk_camera_motion.hand callsscene_frame_inputs()/camera_motion::record, which are also #1831-only. That include isn't behindHOOKSHOT_NGX, so it breaks every build, not just NGX ones.- It still needs #1834 for
nr_settings(),opts.nr_*,opennr_ptx.hfrom build.py, the menu rows and the callers in the chain and the present path. #1834 also patches thevk_dlss.cpp/vk_framegen.cppthat #2118 has since rewritten.
So this has to be rebased onto main as it is now. The NGX direct backend should use #2118's session: one small exported accessor from vk_dlss.cpp (e.g. "NGX is up on this device", running ngx_init() once), and no second NVSDK_NGX_VULKAN_Init. The camera motion for the Open backend should either come from the chain's NrFrame (which already has motion before the upscaler) or be dropped from the present-time path. Before that, please check two things against #2118. Is nvngx_dlssnr found in runtime_dirs(), the paths #2118 gives NGX? And with DLSS-G on, do NR's feature create/release (with its device idle) and DLSS-G's evaluate stay apart? vk_dlss.cpp serialises its own NGX calls under s.mu, but NR's run outside that lock.
New:
- Non-NVIDIA GPUs aren't untouched (inline): on any non-NVIDIA device,
opennr::selected()is true whether or not NR is on, so device creation asks forVK_EXT_shader_float8(and chains its feature struct) on AMD/Intel even when NR is off. NR off should mean no change to device creation there. If Open on AMD must be possible without a restart, that's worth a sentence in the description and docs, plus a test on a non-RDNA GPU. - Licensing looks right:
third_party/opendlss-nris MIT with LICENSE, NOTICE and PROVENANCE pinned to an upstream commit, no weights, and the NGX path copies no SDK code. The maintainer questions from last time (documenting the community-patched DLL and a model folder that only NVIDIA's weights can fill) are still for @NotRexed. - CI: only the title check ran on this head, so no build or tests ran here.
Overlap: #1590 (DLSS 5 tab plus feature 18 inside vk_dlss.cpp) and this PR can't both land. #1590 now conflicts with #2118's vk_dlss.cpp as well. #1542 is a different mechanism (an external AGPL Vulkan layer driven over IPC, with its own F1 page), so the overlap there is the menu and the "DLSS 5" naming, not code. My suggestion from last time still stands: rebuild this onto main as NGX direct plus the in-chain placement (with the #1834 plumbing it needs folded in, now that #1831 is gone), and send Streamline and the vendored OpenDLSS-NR as follow-ups. That would also make the non-NVIDIA question moot for the first PR.
| #include "vk_streamline.h" | ||
|
|
||
| #if defined(HOOKSHOT_NGX) | ||
| #include "vk_dlss.h" // (dlss_ngx_session, dlss_ngx_runtime: the one NGX session and the folders it was given) |
There was a problem hiding this comment.
dlss_ngx_session() / dlss_ngx_runtime() and vk_ngx.h came from #1831, which is closed. Main's vk_dlss.h (after #2118) exports neither, and its NGX init is file-local in vk_dlss.cpp. This needs a small accessor there, sharing #2118's single NVSDK_NGX_VULKAN_Init, rather than bringing #1831's version back.
| #include "shaders/opennr_khr_spv.h" | ||
| #include "shaders/opennr_post_comp_spv.h" | ||
| #include "shaders/opennr_pre_comp_spv.h" | ||
| #include "vk_camera_motion.h" |
There was a problem hiding this comment.
vk_camera_motion.h, scene_frame_inputs() and camera_motion::record are #1831-only too, and this isn't behind HOOKSHOT_NGX, so every build fails on main. Before the upscaler, NrFrame already carries the chain's motion. Could the present-time path go without it, or get it from the chain?
| if (const char* e = getenv("RX_VK_NR_BACKEND"); e && !strcmp(e, "open")) return true; | ||
| if (const char* e = getenv("RX_VK_DLSS_NR_BACKEND"); e && (!strcmp(e, "open") || !strcmp(e, "3"))) return true; | ||
| if (opts.nr_backend == 3) return true; | ||
| if (dev.physical && dev.props.vendorID != 0x10de) return true; // (not NVIDIA: Open, the khr route) |
There was a problem hiding this comment.
This makes selected() true on every non-NVIDIA GPU, so device_extensions / device_features add VK_EXT_shader_float8 and its feature struct on AMD/Intel even with NR off. NR off should leave device creation on non-NVIDIA cards as it is on main. Could this be gated on the row or env asking for Open (or on a model folder being there, as on NVIDIA)?
| // (a check still running at exit: let go of, not destroyed joinable, which is std::terminate. It is joined on a later | ||
| // apply with the row on, or in shutdown; the row turned off mid-check left it joinable in this static for good) | ||
| ~State() { | ||
| if (checker.joinable()) checker.detach(); |
There was a problem hiding this comment.
Nit: this avoids std::terminate, but the detached lambda captures &s and still writes s.check / s.check_done into a State that is being destroyed. It's only at exit and only mid-check, so it's minor. Giving the thread its own heap result (a shared_ptr it writes into) would make the detach clean.
Joey-Magnus
left a comment
There was a problem hiding this comment.
I think this should be closed. It only builds together with #1831 and #1834, and #1831 was closed today in favour of #2118 (merged). vk_opennr.cpp still includes vk_camera_motion.h and calls scene_frame_inputs, which only #1831 added, and neither is on main. vk_dlssnr_ngx.cpp needs nr_settings() from #1834. GitHub shows it as mergeable, but nx2/build.py compiles every gpu/vk/.cpp, so merging it would break every build. Separately, the ~29k lines of opennr__spv.h come from a new scripts/opennr_kernels.py instead of scripts/host_shaders.py, which the repo rules require. The Open backend's model folder can also only come from NVIDIA's weights. #1590 covers the same NGX feature 18 path, builds on its own and is in active review. If you want to keep going, I'd offer the in-chain placement before the HUD to #1590 as a follow-up.
ghost
left a comment
There was a problem hiding this comment.
No new commits since my review at this head, so everything in it still stands: the series has lost its base now that #1831 is closed (vk_ngx.h, dlss_ngx_session() / dlss_ngx_runtime(), vk_camera_motion.h and scene_frame_inputs() aren't on main), it still needs #1834's plumbing, and the non-NVIDIA device-creation change (VK_EXT_shader_float8 asked for with NR off) is still open.
On @Joey-Magnus's review: I agree on the build break, which is the same blocker as above. One point I'd correct: scripts/host_shaders.py only builds the texture shaders in gpu/vk/host_shaders/ (a fixed list of three), not gpu/vk/shaders/, and main already has a separate generator in scripts/fsr3_shaders.py. The rule in AGENTS.md is that *_spv.h headers are compiled by glslang from their sources rather than hand-edited. scripts/opennr_kernels.py does that, and tests/test_opennr_kernels.py --check recompiles and compares. The real cost of those headers is size (~29k lines of SPIR-V for the vendored network), which is one more reason to send OpenDLSS-NR as a follow-up, as suggested before.
On closing in favour of #1590: #1590 currently shows conflicts with main too (it touches the vk_dlss.cpp that #2118 rewrote), so both need a rebase before either builds on main. Which design lands stays with @NotRexed. My technical view is unchanged: the in-chain placement before the HUD and the separation from SR history are the better base, and the cleanest next step for this PR is a rebuild onto main as NGX direct plus the in-chain placement, using #2118's NGX session, with the #1834 pieces it needs folded in.
…before the HUD NVIDIA's DLSS 5 neural rendering model (NGX feature 18), off by default, with a Neural tab (on/off, Stage, Model, Style, Preset, Intensity, the tone and structure strengths, Auto mask; saved as neural_rendering and nr_<name>): - it runs in the temporal chain on the upscaler's picture before the game draws its HUD (the model never sees it: over the finished frame it left a dark smudge behind the hearts and text), or with Stage set to Before upscaler on the render-size scene, which the upscaler then takes, with the chain's depth, motion, jitter and resets (NrFrame). Without a temporal upscaler, at the present over the finished frame (and headless the same way) - a feature on DLSS's one NGX session: vk_dlss.cpp exports dlss_ngx_ready (its ngx_init), dlss_ngx_runtime (its runtime folders), dlss_ngx_feature_mutex (DLSS Frame Gen's lock, held around neural rendering's create, evaluate and release, so the two features' calls never overlap) and dlss_nr_device_extensions (DLSS-G's discovery code, made a helper for any feature; only on a device DLSS may run on) - nr::apply has two callers on two threads (the chain's, the present's) and takes one at a time; turning the row off lets go of the feature and its images; left out with HDR set to Native (its light past 1 would be cut by the model's 8 bits) - vk_work.h last, with a B_DIRECT boundary before NGX records (NotRexed#1473) Neural rows sit before Bug report: Restart and Quit stay the table's last two. The control bridge names the tab "neural". docs/neural-rendering.md; tests/test_nr_settings_live.py (rows, defaults, nothing cached, the mapping compiled); fixtures that compile ini_key get TAB_NEURAL. Parameter names after openNR (MIT, credited in the file); no SDK or openNR code copied. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7c806d5 to
ef1b7b8
Compare
|
Rebuilt on main at 51da046 (ef1b7b8), as hckrman101 suggested: NGX direct plus the in-chain placement, on #2118's session, with the plumbing it needs folded in. It builds on its own now. Streamline and OpenDLSS-NR are gone from it. Your two questions:
The rest:
@Joey-Magnus: thanks for the review. The build break is gone: Locally, the whole |
Joey-Magnus
left a comment
There was a problem hiding this comment.
The rebuild fixes what I raised. It builds on its own now (no vk_camera_motion.h, no scene_frame_inputs, nothing from #1834), the OpenDLSS-NR SPIR-V headers and Streamline are gone, and the NGX session, the runtime folders and the DLSS-G lock all come from #2118. CI is green on ef1b7b8, which sits on current main.
Still open:
- The Neural tab shows in every build. TAB_NEURAL is a full tab of 11 rows, and nothing in row_shown hides it without NGX, so AMD, Intel and Linux users get a tab that can only say "not available". hckrman101 asked #1590 for a row under Graphics that opens its own page and hides when !dlss_built(). The same would fit here.
- There are no screenshots of the tab or of the result, and the in-game run of this head hasn't happened yet. The 47 vs 37 fps figure is from before the rebuild.
- The new menu strings aren't in menu_i18n.h. #1590 has its strings there in 13 languages.
#1590 adds the same feature-18 path, so only one of them can land. Which one is @NotRexed's call.
This adds NVIDIA's DLSS 5 neural rendering model, through NGX, running in the temporal chain on the upscaler's picture before the HUD. Over the finished frame it left a dark smudge behind the hearts and text. Off by default.
Rebuilt on main (51da046) as reviewers suggested: NGX direct plus the in-chain placement, on #2118's NGX session. It builds on its own now. Streamline and the vendored OpenDLSS-NR are gone from this PR; they could come later as follow-ups if wanted. The menu rows and hooks it needs, which #1834 used to carry, are folded in.
What it does
tests/test_nr_settings_live.py). They're saved asneural_renderingandnr_<name>. The rows sit before Bug report, so Restart and Quit stay the table's last two.vk_temporal.cpp, on the engine's picture before the game draws its HUD. That's DLSS, DLAA, FSR 4 or FSR 3.1 on the chain.depth_reversedfor FSR 3.1's 1 − d), motion, jitter and cut resets (NrFrame). It's much cheaper: on an RTX card at 4168×1976, DLSS Quality, 47 fps against 37 after.dlss_nr_before_hud).On #2118's NGX session
vk_dlss.cppexports four small things; there's no secondNVSDK_NGX_VULKAN_Init:dlss_ngx_ready(): itsngx_init(), once.dlss_ngx_runtime(name, …): looks the runtime up inruntime_dirs(), the same folders feat(dlss): DLSS Frame Generation through NGX, evenly paced at 3x and up #2118 gives NGX.nvngx_dlssnr.dllgoes wherenvngx_dlss.dllandnvngx_dlssg.dlldo: the game folder'sngx,RX_VK_DLSS_DIR, or%LOCALAPPDATA%\Hookshot\ngx. The log and the row name where it looked.dlss_ngx_feature_mutex(): DLSS Frame Gen's own lock. Neural rendering holds it around its create, evaluate and release, so with DLSS-G on, the two features' NGX calls never overlap. The order matches DLSS-G's inputs: the chain's lock, then this one. The device idle before a release is taken outside it, under the queue's lock, asfg_releasedoes.dlss_nr_device_extensions(): DLSS-G's own discovery code, made a helper that takes the feature. It's asked only on a device DLSS may run on, so AMD and Intel device creation is unchanged.Fixed from the earlier reviews
nr::apply's two callers (the chain's, under its lock;present_image's) take one at a time.vk_work.hcomes last, withB_DIRECTbefore NGX records.NrFrame) and resets at cuts.Known limits
For maintainers
nvngx_dlssnr.dllruns on RTX 50. The docs don't describe anything else, and Hookshot doesn't read or change NGX's settings or NVIDIA's files.Testing
vk_dlssnr_ngx.cppandvk_dlss.cppcompile with-DHOOKSHOT_NGXagainst NVIDIA's NGX SDK headers and without it, with clang andnx2/build.py's renderer flags. So dovk_temporal.cpp,vk_replay.cpp,vk_main.cppandvk_menu.cpp.tests/run_all.py, the whole suite, locally (clang targeting MSVC, the CPM Vulkan headers, glslang 16.6): 434 passed. The failures are exactly the ones main at 51da046 has on this machine, all down to the local toolchain.test_recorder_sync.pyandtest_nr_settings_live.pypass.🤖 Generated with Claude Code