fix(log): TruePBR {} missing {}; treating as nonPBR every frame#1541
Closed
davo0411 wants to merge 9 commits into
Closed
fix(log): TruePBR {} missing {}; treating as nonPBR every frame#1541davo0411 wants to merge 9 commits into
davo0411 wants to merge 9 commits into
Conversation
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Contributor
WalkthroughIntroduces a new UnifiedWater feature spanning engine hooks, flowmap and cache systems, shader updates (Gerstner waves, foam), utilities for DDS texture I/O, and world-to-cell helpers. Wires the feature into globals and feature lists, adds a shader INI, updates a submodule reference, adjusts PBR warning deduplication, and bumps VR address lib version. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Game
participant Plugin as Plugin Init
participant UW as UnifiedWater
participant Cache as WaterCache
participant Flow as Flowmap
participant Engine as Engine Hooks
participant Shader as Water Shader (HLSL)
Game->>Plugin: Load()
Plugin->>UW: Register feature
Note over UW: On DataLoaded
UW->>Flow: LoadOrGenerateFlowmap()
UW->>Cache: LoadOrGenerateCaches()
UW->>Engine: Install thunks/hooks
Engine-->>UW: World/terrain/water events
UW->>Engine: Set flowmap/displacement params
Note over Shader: At draw
Engine->>Shader: SetupGeometry (flowmap SRV, params)
Shader->>Shader: If UNIFIED_WATER: Gerstner displacement, normals, foam
Shader-->>Engine: Shaded water output
sequenceDiagram
autonumber
actor User as Player
participant UW as UnifiedWater UI
participant Cache as WaterCache (Async)
participant Disk as Disk I/O
User->>UW: Open overlay
UW->>Cache: RegenerateCaches()
Cache->>Cache: Thread pool build per worldspace
Cache->>Disk: Read/Write precache/disk cache
Cache-->>UW: Progress snapshot (percent, status)
UW-->>User: Overlay progress/errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Utils/D3D.h (1)
4-21: Include<filesystem>to satisfy new API signature
std::filesystem::pathis now part of the header’s public surface, but the header doesn’t include<filesystem>. Any translation unit that includesD3D.hwithout previously pulling in<filesystem>will fail to compile. Please add the missing header.#pragma once #include <array> #include <d3d11.h> +#include <filesystem> #include <winrt/base.h>package/Shaders/Water.hlsl (1)
326-333: Displaced vertices must feed the projection multiplyLine 326 still multiplies
inputPositionbyWorldViewProj, so after you perturbworldPosthe actual geometry never moves—only the downstream lighting math sees the displacement. That desynchronises clip-space vs. world-space coordinates and will manifest as shimmering/incorrect shading. Recompute the projected position from the displaced world position instead.- float4 worldViewPos = mul(WorldViewProj[eyeIndex], inputPosition); + float4 worldViewPos = mul(WorldViewProj[eyeIndex], float4(worldPos.xyz, 1.0));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
extern/CommonLibSSE-NG(1 hunks)features/Unified Water/Shaders/Features/UnifiedWater.ini(1 hunks)package/Shaders/Water.hlsl(16 hunks)src/Feature.cpp(2 hunks)src/Features/UnifiedWater.cpp(1 hunks)src/Features/UnifiedWater.h(1 hunks)src/Features/UnifiedWater/Flowmap.cpp(1 hunks)src/Features/UnifiedWater/Flowmap.h(1 hunks)src/Features/UnifiedWater/WaterCache.cpp(1 hunks)src/Features/UnifiedWater/WaterCache.h(1 hunks)src/Globals.cpp(6 hunks)src/Globals.h(4 hunks)src/ShaderCache.h(1 hunks)src/TruePBR.cpp(2 hunks)src/Utils/D3D.cpp(2 hunks)src/Utils/D3D.h(2 hunks)src/Utils/Game.cpp(1 hunks)src/Utils/Game.h(1 hunks)src/XSEPlugin.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/XSEPlugin.cppsrc/Globals.cppsrc/Globals.hsrc/Utils/Game.cppsrc/Feature.cppsrc/Features/UnifiedWater.hsrc/TruePBR.cppsrc/Features/UnifiedWater/WaterCache.hsrc/Utils/D3D.hsrc/ShaderCache.hpackage/Shaders/Water.hlslsrc/Features/UnifiedWater.cppsrc/Features/UnifiedWater/WaterCache.cppsrc/Features/UnifiedWater/Flowmap.hsrc/Utils/Game.hsrc/Features/UnifiedWater/Flowmap.cppsrc/Utils/D3D.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/XSEPlugin.cppsrc/Globals.cppsrc/Globals.hsrc/Utils/Game.cppsrc/Feature.cppsrc/Features/UnifiedWater.hsrc/TruePBR.cppsrc/Features/UnifiedWater/WaterCache.hsrc/Utils/D3D.hsrc/ShaderCache.hsrc/Features/UnifiedWater.cppsrc/Features/UnifiedWater/WaterCache.cppsrc/Features/UnifiedWater/Flowmap.hsrc/Utils/Game.hsrc/Features/UnifiedWater/Flowmap.cppsrc/Utils/D3D.cpp
🧬 Code graph analysis (8)
src/Features/UnifiedWater.h (1)
src/Features/UnifiedWater.cpp (20)
DrawSettings(29-97)DrawSettings(29-29)DrawOverlay(99-136)DrawOverlay(99-99)IsOverlayVisible(138-141)IsOverlayVisible(138-138)DataLoaded(143-189)DataLoaded(143-143)LoadSettings(14-17)LoadSettings(14-14)SaveSettings(19-22)SaveSettings(19-19)RestoreDefaultSettings(24-27)RestoreDefaultSettings(24-24)PostPostLoad(254-290)PostPostLoad(254-254)SetFlowmapTex(242-252)SetFlowmapTex(242-242)LoadOrderChanged(191-240)LoadOrderChanged(191-191)
src/Features/UnifiedWater/WaterCache.h (1)
src/Features/UnifiedWater/WaterCache.cpp (38)
SetCurrentWorldSpace(5-40)SetCurrentWorldSpace(5-5)GetInstructions(42-50)GetInstructions(42-42)GetInstructions(52-75)GetInstructions(52-52)GenerateTamrielPrecache(77-102)GenerateTamrielPrecache(77-77)LoadOrGenerateCaches(104-112)LoadOrGenerateCaches(104-104)RegenerateCaches(114-141)RegenerateCaches(114-114)GenerateCaches(198-270)GenerateCaches(198-198)LoadCaches(143-196)LoadCaches(143-143)BuildPreCache(272-290)BuildPreCache(272-272)BuildDiskCache(292-396)BuildDiskCache(292-292)GenerateInstructions(398-537)GenerateInstructions(398-398)TryBuildRuntimeCache(539-591)TryBuildRuntimeCache(539-539)GetValidWorldSpaces(593-612)GetValidWorldSpaces(593-593)GetLODCoords(614-618)GetLODCoords(614-614)TryGetCellData(620-670)TryGetCellData(620-620)ReadWaterData(672-687)ReadWaterData(672-672)ReadMinLandHeightData(689-722)ReadMinLandHeightData(689-689)TryWriteCacheToFile(725-744)TryWriteCacheToFile(725-725)TryReadCacheFromFile(747-775)TryReadCacheFromFile(747-747)
src/Utils/D3D.h (1)
src/Utils/D3D.cpp (4)
SaveTextureToFile(411-473)SaveTextureToFile(411-411)LoadTextureFromFile(475-510)LoadTextureFromFile(475-475)
src/Features/UnifiedWater.cpp (1)
src/Utils/Game.cpp (4)
WorldToCell(316-320)WorldToCell(316-316)WorldToCell(322-325)WorldToCell(322-322)
src/Features/UnifiedWater/WaterCache.cpp (2)
src/Utils/Game.cpp (4)
WorldToCell(316-320)WorldToCell(316-316)WorldToCell(322-325)WorldToCell(322-322)src/Utils/Game.h (1)
FCC(177-180)
src/Features/UnifiedWater/Flowmap.h (1)
src/Features/UnifiedWater/Flowmap.cpp (12)
TryGetFlowmap(6-13)TryGetFlowmap(6-6)Reset(15-24)Reset(15-15)LoadOrGenerateFlowmap(26-36)LoadOrGenerateFlowmap(26-26)RegenerateAndLoadFlowmap(38-80)RegenerateAndLoadFlowmap(38-38)LoadFlowmap(82-149)LoadFlowmap(82-82)GenerateFlowmap(151-342)GenerateFlowmap(151-151)
src/Utils/Game.h (1)
src/Utils/Game.cpp (4)
WorldToCell(316-320)WorldToCell(316-316)WorldToCell(322-325)WorldToCell(322-322)
src/Features/UnifiedWater/Flowmap.cpp (2)
src/Utils/D3D.cpp (3)
file(100-100)SaveTextureToFile(411-473)SaveTextureToFile(411-411)src/Utils/Game.cpp (4)
WorldToCell(316-320)WorldToCell(316-316)WorldToCell(322-325)WorldToCell(322-322)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (3)
src/TruePBR.cpp (2)
11-11: LGTM!The
<unordered_set>include is necessary for the deduplication logic in theCHECK_PBR_TEXTUREmacro.
44-52: Ensure thread safety for CHECK_PBR_TEXTURE
The static unordered_set used for warning deduplication isn’t synchronized. I found no mutex/lock/atomic/thread references around BSLightingShader_SetupMaterial (lines 44–52 and its call at line 713), so if material setup runs solely on the main render thread it’s safe; otherwise guard insert/find with a mutex or switch to a thread-safe container.src/XSEPlugin.cpp (1)
148-148: Clarify version bump purpose.VR address library v0.192.0 is valid and the bump is correct, but it appears unrelated to the TruePBR logging fix. Please explain how this bump supports the TruePBR fix or the UnifiedWater feature.
Comment on lines
+138
to
+145
| flowmapTex = RE::NiPointer(sourceTex); | ||
|
|
||
| width = std::stoi(tokens[1]); | ||
| height = std::stoi(tokens[2]); | ||
| offsetX = std::stoi(tokens[3]); | ||
| offsetY = std::stoi(tokens[4]); | ||
| invWidth = 1.0f / static_cast<float>(width); | ||
| invHeight = 1.0f / static_cast<float>(height); |
Contributor
There was a problem hiding this comment.
Handle invalid Tamriel-Flowmap filenames safely
std::stoi will throw on tampered or corrupted filenames (or modded assets), crashing the plugin. Wrap the parsing in a try/catch and validate the parsed values before storing them, per our “robust error handling” guideline for runtime code.
- flowmapTex = RE::NiPointer(sourceTex);
-
- width = std::stoi(tokens[1]);
- height = std::stoi(tokens[2]);
- offsetX = std::stoi(tokens[3]);
- offsetY = std::stoi(tokens[4]);
+ int32_t parsedWidth = 0;
+ int32_t parsedHeight = 0;
+ int32_t parsedOffsetX = 0;
+ int32_t parsedOffsetY = 0;
+
+ try {
+ parsedWidth = std::stoi(tokens[1]);
+ parsedHeight = std::stoi(tokens[2]);
+ parsedOffsetX = std::stoi(tokens[3]);
+ parsedOffsetY = std::stoi(tokens[4]);
+ } catch (const std::exception& e) {
+ logger::error("[Unified Water] [Flowmap] Invalid flowmap metadata in {}: {}", file.path().string(), e.what());
+ return false;
+ }
+
+ if (parsedWidth <= 0 || parsedHeight <= 0) {
+ logger::error("[Unified Water] [Flowmap] Invalid flowmap dimensions {}x{} in {}", parsedWidth, parsedHeight, file.path().string());
+ return false;
+ }
+
+ flowmapTex = RE::NiPointer(sourceTex);
+ width = parsedWidth;
+ height = parsedHeight;
+ offsetX = parsedOffsetX;
+ offsetY = parsedOffsetY;As per coding guidelines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| flowmapTex = RE::NiPointer(sourceTex); | |
| width = std::stoi(tokens[1]); | |
| height = std::stoi(tokens[2]); | |
| offsetX = std::stoi(tokens[3]); | |
| offsetY = std::stoi(tokens[4]); | |
| invWidth = 1.0f / static_cast<float>(width); | |
| invHeight = 1.0f / static_cast<float>(height); | |
| int32_t parsedWidth = 0; | |
| int32_t parsedHeight = 0; | |
| int32_t parsedOffsetX = 0; | |
| int32_t parsedOffsetY = 0; | |
| try { | |
| parsedWidth = std::stoi(tokens[1]); | |
| parsedHeight = std::stoi(tokens[2]); | |
| parsedOffsetX = std::stoi(tokens[3]); | |
| parsedOffsetY = std::stoi(tokens[4]); | |
| } catch (const std::exception& e) { | |
| logger::error("[Unified Water] [Flowmap] Invalid flowmap metadata in {}: {}", file.path().string(), e.what()); | |
| return false; | |
| } | |
| if (parsedWidth <= 0 || parsedHeight <= 0) { | |
| logger::error( | |
| "[Unified Water] [Flowmap] Invalid flowmap dimensions {}x{} in {}", | |
| parsedWidth, parsedHeight, file.path().string() | |
| ); | |
| return false; | |
| } | |
| flowmapTex = RE::NiPointer(sourceTex); | |
| width = parsedWidth; | |
| height = parsedHeight; | |
| offsetX = parsedOffsetX; | |
| offsetY = parsedOffsetY; | |
| invWidth = 1.0f / static_cast<float>(width); | |
| invHeight = 1.0f / static_cast<float>(height); |
🤖 Prompt for AI Agents
In src/Features/UnifiedWater/Flowmap.cpp around lines 138 to 145, parsing
integers from the flowmap filename uses std::stoi directly which can throw on
corrupted or tampered filenames; wrap the parsing in a try/catch, first ensure
tokens has at least 5 elements, then attempt stoi for width, height, offsetX,
offsetY inside the try block, validate that width and height are > 0 (and
invWidth/invHeight computed only for valid nonzero dimensions), clamp or
validate offsets to a sane range if needed, and on any exception or invalid
value log an error and skip using the parsed values (or fall back to safe
defaults) instead of letting the exception propagate and crash the plugin.
Comment on lines
+164
to
+336
| static winrt::com_ptr<REX::W32::ID3D11Multithread> multithread; | ||
| if (SUCCEEDED(ctx->QueryInterface(multithread.put()))) { | ||
| multithread->SetMultithreadProtected(TRUE); | ||
| } else { | ||
| logger::error("[Unified Water] [Flowmap] ID3D11Multithread not available"); | ||
| return false; | ||
| } | ||
|
|
||
| const auto tamriel = RE::TESForm::LookupByEditorID<RE::TESWorldSpace>("Tamriel"); | ||
| if (!tamriel) { | ||
| logger::error("[Unified Water] [Flowmap] Failed to load Tamriel WorldSpace"); | ||
| return false; | ||
| } | ||
|
|
||
| int32_t worldMinX, worldMinY, worldMaxX, worldMaxY; | ||
| Util::WorldToCell(tamriel->minimumCoords, worldMinX, worldMinY); | ||
| Util::WorldToCell(tamriel->maximumCoords, worldMaxX, worldMaxY); | ||
| worldMaxX -= 1; | ||
| worldMaxY -= 1; | ||
|
|
||
| struct FlowCell | ||
| { | ||
| int32_t x; | ||
| int32_t y; | ||
| winrt::com_ptr<ID3D11Texture2D> tex; | ||
| }; | ||
|
|
||
| int32_t mapMinX = INT_MAX; | ||
| int32_t mapMinY = INT_MAX; | ||
| int32_t mapMaxX = INT_MIN; | ||
| int32_t mapMaxY = INT_MIN; | ||
|
|
||
| auto cells = std::vector<FlowCell>(); | ||
| cells.reserve(1024); | ||
|
|
||
| { | ||
| multithread->Enter(); | ||
| for (auto y = worldMinY; y < worldMaxY; ++y) { | ||
| for (auto x = worldMinX; x < worldMaxX; ++x) { | ||
| auto path = std::format(R"(Textures\Water\skyrim.esm\flow.{}.{}.dds)", x, y); | ||
| auto stream = RE::BSResourceNiBinaryStream(path); | ||
|
|
||
| if (!stream.good()) | ||
| continue; | ||
|
|
||
| const auto size = stream.stream->totalSize; | ||
| std::vector<uint8_t> buffer(size); | ||
| stream.read(buffer.data(), size); | ||
|
|
||
| DirectX::TexMetadata meta{}; | ||
| DirectX::ScratchImage src; | ||
| auto hr = DirectX::LoadFromDDSMemory(buffer.data(), size, DirectX::DDS_FLAGS_NONE, &meta, src); | ||
| if (FAILED(hr)) { | ||
| logger::warn("[Unified Water] [Flowmap] Flow texture at {},{} failed to load", x, y); | ||
| continue; | ||
| } | ||
|
|
||
| DirectX::ScratchImage conv; | ||
| if (DirectX::IsCompressed(meta.format)) { | ||
| hr = DirectX::Decompress(src.GetImages(), src.GetImageCount(), src.GetMetadata(), DXGI_FORMAT_B8G8R8A8_UNORM, conv); | ||
| if (FAILED(hr)) { | ||
| logger::warn("[Unified Water] [Flowmap] Flow texture at {},{} failed to decompress", x, y); | ||
| continue; | ||
| } | ||
| } else if (meta.format != DXGI_FORMAT_B8G8R8A8_UNORM) { | ||
| hr = DirectX::Convert(src.GetImages(), src.GetImageCount(), src.GetMetadata(), DXGI_FORMAT_B8G8R8A8_UNORM, DirectX::TEX_FILTER_DEFAULT, 0.0f, conv); | ||
| if (FAILED(hr)) { | ||
| logger::warn("[Unified Water] [Flowmap] Flow texture at {},{} failed to convert to the correct format", x, y); | ||
| continue; | ||
| } | ||
| } else { | ||
| conv = std::move(src); | ||
| } | ||
|
|
||
| winrt::com_ptr<ID3D11Resource> res; | ||
| hr = DirectX::CreateTexture(dvc, conv.GetImages(), conv.GetImageCount(), conv.GetMetadata(), res.put()); | ||
| if (FAILED(hr) || !res) { | ||
| logger::warn("[Unified Water] [Flowmap] Flow texture at {},{} creation failed", x, y); | ||
| continue; | ||
| } | ||
|
|
||
| winrt::com_ptr<ID3D11Texture2D> tex; | ||
| hr = res->QueryInterface(IID_PPV_ARGS(tex.put())); | ||
| if (FAILED(hr)) { | ||
| logger::warn("[Unified Water] [Flowmap] Flow texture at {},{} is not a Texture2D", x, y); | ||
| continue; | ||
| } | ||
|
|
||
| D3D11_TEXTURE2D_DESC d{}; | ||
| tex->GetDesc(&d); | ||
| if (d.Width != 64 || d.Height != 64 || d.Format != DXGI_FORMAT_B8G8R8A8_UNORM || d.MipLevels != 6) { | ||
| logger::warn("[Unified Water] [Flowmap] Flow texture at {},{} is invalid", x, y); | ||
| continue; | ||
| } | ||
|
|
||
| mapMinX = std::min(mapMinX, x); | ||
| mapMinY = std::min(mapMinY, y); | ||
| mapMaxX = std::max(mapMaxX, x); | ||
| mapMaxY = std::max(mapMaxY, y); | ||
|
|
||
| cells.emplace_back(FlowCell{ x, y, tex }); | ||
| } | ||
| } | ||
| multithread->Leave(); | ||
| } | ||
|
|
||
| const auto width = mapMaxX - mapMinX + 1; | ||
| const auto height = mapMaxY - mapMinY + 1; | ||
| const auto offsetX = -mapMinX; | ||
| const auto offsetY = -mapMinY; | ||
|
|
||
| logger::debug("[Unified Water] [Flowmap] Loaded {} flow textures, creating a {}x{} flow map...", cells.size(), width, height); | ||
|
|
||
| D3D11_TEXTURE2D_DESC desc{}; | ||
| desc.Width = width * 64; | ||
| desc.Height = height * 64; | ||
| desc.MipLevels = 6; | ||
| desc.ArraySize = 1; | ||
| desc.Format = DXGI_FORMAT_B8G8R8A8_UNORM; | ||
| desc.SampleDesc = { 1, 0 }; | ||
| desc.Usage = D3D11_USAGE_DEFAULT; | ||
| desc.BindFlags = D3D11_BIND_SHADER_RESOURCE; | ||
| desc.MiscFlags = 0; | ||
|
|
||
| winrt::com_ptr<ID3D11Texture2D> flowmap; | ||
| if (FAILED(dvc->CreateTexture2D(&desc, nullptr, flowmap.put()))) { | ||
| logger::error("[Unified Water] [Flowmap] Failed to create texture"); | ||
| return false; | ||
| } | ||
|
|
||
| for (const auto& [x, y, flowTex] : cells) { | ||
| D3D11_TEXTURE2D_DESC srcDesc{}; | ||
| flowTex->GetDesc(&srcDesc); | ||
|
|
||
| const UINT sx = static_cast<UINT>(x + offsetX); | ||
| const UINT sy = static_cast<UINT>(y + offsetY); | ||
| const UINT dstX0 = sx * 64; | ||
|
|
||
| const UINT maxMipLevel = useMips ? 6u : 1u; | ||
| for (UINT mipLevel = 0; mipLevel < maxMipLevel; ++mipLevel) { | ||
| const UINT srcSub = D3D11CalcSubresource(mipLevel, 0, srcDesc.MipLevels); | ||
| const UINT dstSub = D3D11CalcSubresource(mipLevel, 0, desc.MipLevels); | ||
| const UINT tileSize = std::max(1u, 64u >> mipLevel); | ||
| const UINT flowmapHeight = std::max(1u, desc.Height >> mipLevel); | ||
| const UINT dstX = dstX0 >> mipLevel; | ||
| const UINT dstY = flowmapHeight - (sy + 1) * tileSize; | ||
|
|
||
| deferredCtx->CopySubresourceRegion(flowmap.get(), dstSub, dstX, dstY, 0, flowTex.get(), srcSub, nullptr); | ||
| } | ||
| } | ||
|
|
||
| winrt::com_ptr<ID3D11CommandList> commandList; | ||
| if (deferredCtx && FAILED(deferredCtx->FinishCommandList(FALSE, commandList.put()))) { | ||
| logger::error("[Unified Water] [Flowmap] FinishCommandList failed"); | ||
| return false; | ||
| } | ||
|
|
||
| { | ||
| multithread->Enter(); | ||
| ctx->ExecuteCommandList(commandList.get(), FALSE); | ||
|
|
||
| const auto filename = std::format(L"Tamriel-Flowmap.{}.{}.{}.{}.dds", width, height, offsetX, offsetY); | ||
| const auto path = Util::PathHelpers::GetDataPath() / "textures" / "water" / "flowmaps" / filename; | ||
| const auto hr = Util::SaveTextureToFile(dvc, ctx, path, flowmap.get()); | ||
| multithread->Leave(); | ||
| if (FAILED(hr)) { | ||
| logger::error("[Unified Water] [Flowmap] Failed to save flowmap to {}: hr={:08X}", path.string().c_str(), static_cast<uint32_t>(hr)); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| multithread->SetMultithreadProtected(FALSE); | ||
|
|
Contributor
There was a problem hiding this comment.
Always release ID3D11Multithread protection on failure paths
After calling SetMultithreadProtected(TRUE) we have multiple return false exits that never flip it back, leaving the immediate context permanently locked. Use a small guard so every exit restores the flag and drop the final manual call.
- static winrt::com_ptr<REX::W32::ID3D11Multithread> multithread;
- if (SUCCEEDED(ctx->QueryInterface(multithread.put()))) {
- multithread->SetMultithreadProtected(TRUE);
- } else {
+ static winrt::com_ptr<REX::W32::ID3D11Multithread> multithread;
+ if (FAILED(ctx->QueryInterface(multithread.put()))) {
logger::error("[Unified Water] [Flowmap] ID3D11Multithread not available");
return false;
}
+
+ multithread->SetMultithreadProtected(TRUE);
+ struct MultithreadGuard
+ {
+ REX::W32::ID3D11Multithread* handle;
+ ~MultithreadGuard()
+ {
+ if (handle) {
+ handle->SetMultithreadProtected(FALSE);
+ }
+ }
+ };
+ const MultithreadGuard multithreadGuard{ multithread.get() };
@@
- multithread->SetMultithreadProtected(FALSE);As per coding guidelines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static winrt::com_ptr<REX::W32::ID3D11Multithread> multithread; | |
| if (SUCCEEDED(ctx->QueryInterface(multithread.put()))) { | |
| multithread->SetMultithreadProtected(TRUE); | |
| } else { | |
| logger::error("[Unified Water] [Flowmap] ID3D11Multithread not available"); | |
| return false; | |
| } | |
| const auto tamriel = RE::TESForm::LookupByEditorID<RE::TESWorldSpace>("Tamriel"); | |
| if (!tamriel) { | |
| logger::error("[Unified Water] [Flowmap] Failed to load Tamriel WorldSpace"); | |
| return false; | |
| } | |
| int32_t worldMinX, worldMinY, worldMaxX, worldMaxY; | |
| Util::WorldToCell(tamriel->minimumCoords, worldMinX, worldMinY); | |
| Util::WorldToCell(tamriel->maximumCoords, worldMaxX, worldMaxY); | |
| worldMaxX -= 1; | |
| worldMaxY -= 1; | |
| struct FlowCell | |
| { | |
| int32_t x; | |
| int32_t y; | |
| winrt::com_ptr<ID3D11Texture2D> tex; | |
| }; | |
| int32_t mapMinX = INT_MAX; | |
| int32_t mapMinY = INT_MAX; | |
| int32_t mapMaxX = INT_MIN; | |
| int32_t mapMaxY = INT_MIN; | |
| auto cells = std::vector<FlowCell>(); | |
| cells.reserve(1024); | |
| { | |
| multithread->Enter(); | |
| for (auto y = worldMinY; y < worldMaxY; ++y) { | |
| for (auto x = worldMinX; x < worldMaxX; ++x) { | |
| auto path = std::format(R"(Textures\Water\skyrim.esm\flow.{}.{}.dds)", x, y); | |
| auto stream = RE::BSResourceNiBinaryStream(path); | |
| if (!stream.good()) | |
| continue; | |
| const auto size = stream.stream->totalSize; | |
| std::vector<uint8_t> buffer(size); | |
| stream.read(buffer.data(), size); | |
| DirectX::TexMetadata meta{}; | |
| DirectX::ScratchImage src; | |
| auto hr = DirectX::LoadFromDDSMemory(buffer.data(), size, DirectX::DDS_FLAGS_NONE, &meta, src); | |
| if (FAILED(hr)) { | |
| logger::warn("[Unified Water] [Flowmap] Flow texture at {},{} failed to load", x, y); | |
| continue; | |
| } | |
| DirectX::ScratchImage conv; | |
| if (DirectX::IsCompressed(meta.format)) { | |
| hr = DirectX::Decompress(src.GetImages(), src.GetImageCount(), src.GetMetadata(), DXGI_FORMAT_B8G8R8A8_UNORM, conv); | |
| if (FAILED(hr)) { | |
| logger::warn("[Unified Water] [Flowmap] Flow texture at {},{} failed to decompress", x, y); | |
| continue; | |
| } | |
| } else if (meta.format != DXGI_FORMAT_B8G8R8A8_UNORM) { | |
| hr = DirectX::Convert(src.GetImages(), src.GetImageCount(), src.GetMetadata(), DXGI_FORMAT_B8G8R8A8_UNORM, DirectX::TEX_FILTER_DEFAULT, 0.0f, conv); | |
| if (FAILED(hr)) { | |
| logger::warn("[Unified Water] [Flowmap] Flow texture at {},{} failed to convert to the correct format", x, y); | |
| continue; | |
| } | |
| } else { | |
| conv = std::move(src); | |
| } | |
| winrt::com_ptr<ID3D11Resource> res; | |
| hr = DirectX::CreateTexture(dvc, conv.GetImages(), conv.GetImageCount(), conv.GetMetadata(), res.put()); | |
| if (FAILED(hr) || !res) { | |
| logger::warn("[Unified Water] [Flowmap] Flow texture at {},{} creation failed", x, y); | |
| continue; | |
| } | |
| winrt::com_ptr<ID3D11Texture2D> tex; | |
| hr = res->QueryInterface(IID_PPV_ARGS(tex.put())); | |
| if (FAILED(hr)) { | |
| logger::warn("[Unified Water] [Flowmap] Flow texture at {},{} is not a Texture2D", x, y); | |
| continue; | |
| } | |
| D3D11_TEXTURE2D_DESC d{}; | |
| tex->GetDesc(&d); | |
| if (d.Width != 64 || d.Height != 64 || d.Format != DXGI_FORMAT_B8G8R8A8_UNORM || d.MipLevels != 6) { | |
| logger::warn("[Unified Water] [Flowmap] Flow texture at {},{} is invalid", x, y); | |
| continue; | |
| } | |
| mapMinX = std::min(mapMinX, x); | |
| mapMinY = std::min(mapMinY, y); | |
| mapMaxX = std::max(mapMaxX, x); | |
| mapMaxY = std::max(mapMaxY, y); | |
| cells.emplace_back(FlowCell{ x, y, tex }); | |
| } | |
| } | |
| multithread->Leave(); | |
| } | |
| const auto width = mapMaxX - mapMinX + 1; | |
| const auto height = mapMaxY - mapMinY + 1; | |
| const auto offsetX = -mapMinX; | |
| const auto offsetY = -mapMinY; | |
| logger::debug("[Unified Water] [Flowmap] Loaded {} flow textures, creating a {}x{} flow map...", cells.size(), width, height); | |
| D3D11_TEXTURE2D_DESC desc{}; | |
| desc.Width = width * 64; | |
| desc.Height = height * 64; | |
| desc.MipLevels = 6; | |
| desc.ArraySize = 1; | |
| desc.Format = DXGI_FORMAT_B8G8R8A8_UNORM; | |
| desc.SampleDesc = { 1, 0 }; | |
| desc.Usage = D3D11_USAGE_DEFAULT; | |
| desc.BindFlags = D3D11_BIND_SHADER_RESOURCE; | |
| desc.MiscFlags = 0; | |
| winrt::com_ptr<ID3D11Texture2D> flowmap; | |
| if (FAILED(dvc->CreateTexture2D(&desc, nullptr, flowmap.put()))) { | |
| logger::error("[Unified Water] [Flowmap] Failed to create texture"); | |
| return false; | |
| } | |
| for (const auto& [x, y, flowTex] : cells) { | |
| D3D11_TEXTURE2D_DESC srcDesc{}; | |
| flowTex->GetDesc(&srcDesc); | |
| const UINT sx = static_cast<UINT>(x + offsetX); | |
| const UINT sy = static_cast<UINT>(y + offsetY); | |
| const UINT dstX0 = sx * 64; | |
| const UINT maxMipLevel = useMips ? 6u : 1u; | |
| for (UINT mipLevel = 0; mipLevel < maxMipLevel; ++mipLevel) { | |
| const UINT srcSub = D3D11CalcSubresource(mipLevel, 0, srcDesc.MipLevels); | |
| const UINT dstSub = D3D11CalcSubresource(mipLevel, 0, desc.MipLevels); | |
| const UINT tileSize = std::max(1u, 64u >> mipLevel); | |
| const UINT flowmapHeight = std::max(1u, desc.Height >> mipLevel); | |
| const UINT dstX = dstX0 >> mipLevel; | |
| const UINT dstY = flowmapHeight - (sy + 1) * tileSize; | |
| deferredCtx->CopySubresourceRegion(flowmap.get(), dstSub, dstX, dstY, 0, flowTex.get(), srcSub, nullptr); | |
| } | |
| } | |
| winrt::com_ptr<ID3D11CommandList> commandList; | |
| if (deferredCtx && FAILED(deferredCtx->FinishCommandList(FALSE, commandList.put()))) { | |
| logger::error("[Unified Water] [Flowmap] FinishCommandList failed"); | |
| return false; | |
| } | |
| { | |
| multithread->Enter(); | |
| ctx->ExecuteCommandList(commandList.get(), FALSE); | |
| const auto filename = std::format(L"Tamriel-Flowmap.{}.{}.{}.{}.dds", width, height, offsetX, offsetY); | |
| const auto path = Util::PathHelpers::GetDataPath() / "textures" / "water" / "flowmaps" / filename; | |
| const auto hr = Util::SaveTextureToFile(dvc, ctx, path, flowmap.get()); | |
| multithread->Leave(); | |
| if (FAILED(hr)) { | |
| logger::error("[Unified Water] [Flowmap] Failed to save flowmap to {}: hr={:08X}", path.string().c_str(), static_cast<uint32_t>(hr)); | |
| return false; | |
| } | |
| } | |
| multithread->SetMultithreadProtected(FALSE); | |
| static winrt::com_ptr<REX::W32::ID3D11Multithread> multithread; | |
| if (FAILED(ctx->QueryInterface(multithread.put()))) { | |
| logger::error("[Unified Water] [Flowmap] ID3D11Multithread not available"); | |
| return false; | |
| } | |
| multithread->SetMultithreadProtected(TRUE); | |
| struct MultithreadGuard | |
| { | |
| REX::W32::ID3D11Multithread* handle; | |
| ~MultithreadGuard() | |
| { | |
| if (handle) { | |
| handle->SetMultithreadProtected(FALSE); | |
| } | |
| } | |
| }; | |
| const MultithreadGuard multithreadGuard{ multithread.get() }; | |
| const auto tamriel = RE::TESForm::LookupByEditorID<RE::TESWorldSpace>("Tamriel"); | |
| if (!tamriel) { | |
| logger::error("[Unified Water] [Flowmap] Failed to load Tamriel WorldSpace"); | |
| return false; | |
| } | |
| int32_t worldMinX, worldMinY, worldMaxX, worldMaxY; | |
| Util::WorldToCell(tamriel->minimumCoords, worldMinX, worldMinY); | |
| Util::WorldToCell(tamriel->maximumCoords, worldMaxX, worldMaxY); | |
| worldMaxX -= 1; | |
| worldMaxY -= 1; | |
| // … the rest of your loading, copy, save logic unchanged … | |
| // Note: the original final call | |
| // multithread->SetMultithreadProtected(FALSE); | |
| // has been removed — the guard’s destructor will handle it automatically. |
🤖 Prompt for AI Agents
src/Features/UnifiedWater/Flowmap.cpp lines 164-336: after setting
multithread->SetMultithreadProtected(TRUE) several early returns leave the flag
set; introduce a small RAII guard (e.g., a local struct whose constructor takes
the multithread pointer and whose destructor calls
SetMultithreadProtected(FALSE)) immediately after the successful QueryInterface
so that any return path automatically clears protection; remove the final manual
SetMultithreadProtected(FALSE) call at the end of the function and ensure
existing multithread->Enter()/Leave() usages remain unchanged (they should still
be called while protection is active).
Comment on lines
+270
to
+276
| const auto width = mapMaxX - mapMinX + 1; | ||
| const auto height = mapMaxY - mapMinY + 1; | ||
| const auto offsetX = -mapMinX; | ||
| const auto offsetY = -mapMinY; | ||
|
|
||
| logger::debug("[Unified Water] [Flowmap] Loaded {} flow textures, creating a {}x{} flow map...", cells.size(), width, height); | ||
|
|
Contributor
There was a problem hiding this comment.
Guard against empty flowmap input
When no per-cell flow textures exist, mapMinX/Y stay at INT_MAX/INT_MIN, so the width/height calculation underflows and we feed absurd dimensions into CreateTexture2D, which is undefined behaviour and usually crashes. Please bail out before computing the extents when we fail to gather at least one valid tile.
+ if (cells.empty() || mapMinX == INT_MAX || mapMaxX == INT_MIN || mapMinY == INT_MAX || mapMaxY == INT_MIN) {
+ logger::error("[Unified Water] [Flowmap] No valid source flow tiles found; aborting generation");
+ return false;
+ }
+
const auto width = mapMaxX - mapMinX + 1;
const auto height = mapMaxY - mapMinY + 1;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const auto width = mapMaxX - mapMinX + 1; | |
| const auto height = mapMaxY - mapMinY + 1; | |
| const auto offsetX = -mapMinX; | |
| const auto offsetY = -mapMinY; | |
| logger::debug("[Unified Water] [Flowmap] Loaded {} flow textures, creating a {}x{} flow map...", cells.size(), width, height); | |
| const auto offsetX = -mapMinX; | |
| const auto offsetY = -mapMinY; | |
| if (cells.empty() || | |
| mapMinX == INT_MAX || mapMaxX == INT_MIN || | |
| mapMinY == INT_MAX || mapMaxY == INT_MIN) | |
| { | |
| logger::error("[Unified Water] [Flowmap] No valid source flow tiles found; aborting generation"); | |
| return false; | |
| } | |
| const auto width = mapMaxX - mapMinX + 1; | |
| const auto height = mapMaxY - mapMinY + 1; | |
| logger::debug("[Unified Water] [Flowmap] Loaded {} flow textures, creating a {}x{} flow map...", cells.size(), width, height); |
🤖 Prompt for AI Agents
In src/Features/UnifiedWater/Flowmap.cpp around lines 270 to 276, the code
computes map extents even when no per-cell flow textures were gathered, which
lets mapMinX/Y remain at INT_MAX/INT_MIN and causes width/height underflow; add
an early guard that checks if cells.empty() (or otherwise confirms at least one
valid tile) and if true log a debug/warn message and return/skip creating the
flow texture (or return an error/false from the surrounding function) before
computing offset/width/height so CreateTexture2D never receives absurd
dimensions.
Comment on lines
+436
to
+468
| DirectX::ScratchImage cpuImage; | ||
| if (const auto hr = CaptureTexture(device, context, tex, cpuImage); FAILED(hr)) | ||
| return hr; | ||
|
|
||
| const auto parent = path.parent_path(); | ||
| std::error_code ec; | ||
| if (!parent.empty() && !fs::exists(parent, ec)) { | ||
| if (!fs::create_directories(parent, ec)) { | ||
| logger::error("SaveTextureToFile: failed to create directories"); | ||
| return HRESULT_FROM_WIN32(static_cast<DWORD>(ec.value())); | ||
| } | ||
| } | ||
|
|
||
| const std::wstring tmp = path.wstring().append(L".tmp"); | ||
| if (const auto hr = SaveToDDSFile(cpuImage.GetImages(), cpuImage.GetImageCount(), cpuImage.GetMetadata(), DirectX::DDS_FLAGS_NONE, tmp.c_str()); FAILED(hr)) { | ||
| logger::error("SaveTextureToFile: failed to save to file"); | ||
| fs::remove(tmp); | ||
| return hr; | ||
| } | ||
|
|
||
| fs::rename(tmp, path, ec); | ||
| if (ec) { | ||
| fs::remove(path, ec); | ||
| if (ec) { | ||
| logger::error("SaveTextureToFile: failed to remove existing file"); | ||
| fs::remove(tmp); | ||
| return HRESULT_FROM_WIN32(static_cast<DWORD>(ec.value())); | ||
| } | ||
| fs::rename(tmp, path, ec); | ||
| if (ec) { | ||
| logger::error("SaveTextureToFile: failed to rename file"); | ||
| fs::remove(tmp); | ||
| return HRESULT_FROM_WIN32(static_cast<DWORD>(ec.value())); |
Contributor
There was a problem hiding this comment.
Qualify DirectXTex helper calls
CaptureTexture and SaveToDDSFile are declared inside the DirectX namespace. Calling them without the namespace won’t compile (unresolved identifier). Qualify the calls so the new API links correctly.
- if (const auto hr = CaptureTexture(device, context, tex, cpuImage); FAILED(hr))
+ if (const auto hr = DirectX::CaptureTexture(device, context, tex, cpuImage); FAILED(hr))
return hr;
...
- if (const auto hr = SaveToDDSFile(cpuImage.GetImages(), cpuImage.GetImageCount(), cpuImage.GetMetadata(), DirectX::DDS_FLAGS_NONE, tmp.c_str()); FAILED(hr)) {
+ if (const auto hr = DirectX::SaveToDDSFile(cpuImage.GetImages(), cpuImage.GetImageCount(), cpuImage.GetMetadata(), DirectX::DDS_FLAGS_NONE, tmp.c_str()); FAILED(hr)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| DirectX::ScratchImage cpuImage; | |
| if (const auto hr = CaptureTexture(device, context, tex, cpuImage); FAILED(hr)) | |
| return hr; | |
| const auto parent = path.parent_path(); | |
| std::error_code ec; | |
| if (!parent.empty() && !fs::exists(parent, ec)) { | |
| if (!fs::create_directories(parent, ec)) { | |
| logger::error("SaveTextureToFile: failed to create directories"); | |
| return HRESULT_FROM_WIN32(static_cast<DWORD>(ec.value())); | |
| } | |
| } | |
| const std::wstring tmp = path.wstring().append(L".tmp"); | |
| if (const auto hr = SaveToDDSFile(cpuImage.GetImages(), cpuImage.GetImageCount(), cpuImage.GetMetadata(), DirectX::DDS_FLAGS_NONE, tmp.c_str()); FAILED(hr)) { | |
| logger::error("SaveTextureToFile: failed to save to file"); | |
| fs::remove(tmp); | |
| return hr; | |
| } | |
| fs::rename(tmp, path, ec); | |
| if (ec) { | |
| fs::remove(path, ec); | |
| if (ec) { | |
| logger::error("SaveTextureToFile: failed to remove existing file"); | |
| fs::remove(tmp); | |
| return HRESULT_FROM_WIN32(static_cast<DWORD>(ec.value())); | |
| } | |
| fs::rename(tmp, path, ec); | |
| if (ec) { | |
| logger::error("SaveTextureToFile: failed to rename file"); | |
| fs::remove(tmp); | |
| return HRESULT_FROM_WIN32(static_cast<DWORD>(ec.value())); | |
| DirectX::ScratchImage cpuImage; | |
| if (const auto hr = DirectX::CaptureTexture(device, context, tex, cpuImage); FAILED(hr)) | |
| return hr; | |
| const auto parent = path.parent_path(); | |
| std::error_code ec; | |
| if (!parent.empty() && !fs::exists(parent, ec)) { | |
| if (!fs::create_directories(parent, ec)) { | |
| logger::error("SaveTextureToFile: failed to create directories"); | |
| return HRESULT_FROM_WIN32(static_cast<DWORD>(ec.value())); | |
| } | |
| } | |
| const std::wstring tmp = path.wstring().append(L".tmp"); | |
| if (const auto hr = DirectX::SaveToDDSFile( | |
| cpuImage.GetImages(), | |
| cpuImage.GetImageCount(), | |
| cpuImage.GetMetadata(), | |
| DirectX::DDS_FLAGS_NONE, | |
| tmp.c_str() | |
| ); FAILED(hr) | |
| ) { | |
| logger::error("SaveTextureToFile: failed to save to file"); | |
| fs::remove(tmp); | |
| return hr; | |
| } | |
| fs::rename(tmp, path, ec); | |
| if (ec) { | |
| fs::remove(path, ec); | |
| if (ec) { | |
| logger::error("SaveTextureToFile: failed to remove existing file"); | |
| fs::remove(tmp); | |
| return HRESULT_FROM_WIN32(static_cast<DWORD>(ec.value())); | |
| } | |
| fs::rename(tmp, path, ec); | |
| if (ec) { | |
| logger::error("SaveTextureToFile: failed to rename file"); | |
| fs::remove(tmp); | |
| return HRESULT_FROM_WIN32(static_cast<DWORD>(ec.value())); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/Utils/D3D.cpp around lines 436 to 468, the calls to CaptureTexture and
SaveToDDSFile are unqualified and therefore unresolved; prefix both calls with
the DirectX:: namespace (i.e., replace CaptureTexture(...) with
DirectX::CaptureTexture(...) and SaveToDDSFile(...) with
DirectX::SaveToDDSFile(...)) so the compiler finds the functions declared in the
DirectX namespace; no other logic changes are needed.
|
✅ A pre-release build is available for this PR: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes for log spamming.
had an issue where the log was printing this message every single frame.
Summary by CodeRabbit
New Features
Improvements
Chores