fix: support seasons swaps in PBR and TerrainHelper#1099
Conversation
|
""" WalkthroughThe control flow in the Changes
Poem
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
alandtse
left a comment
There was a problem hiding this comment.
Please confirm with test builds.
|
There's also the test that nothing breaks with the changes. You don't need seasons for that. |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/TruePBR.cpp (1)
1062-1070:⚠️ Potential issueSeasonal swap is fetched but then ignored for PBR detection
textureSetDatais still queried with the originallandTexture.textureSet, meaning PBR configs are silently missed when a seasonal swap is active.- auto* textureSetData = truePBR->GetPBRTextureSetData(landTexture.textureSet); + auto* textureSetData = truePBR->GetPBRTextureSetData(textureSet);Without this fix, swapped landscapes render with non-PBR materials even when a PBR JSON exists for the swapped set.
🧹 Nitpick comments (2)
src/Utils/Game.h (1)
81-85: Add minimal contract & const-correctness to the new helper
The helper neither mutates nor takes ownership of the texture set, yet the signature accepts/returns a mutable raw pointer. Consider tightening the contract:-RE::BGSTextureSet* GetSeasonalSwap(RE::BGSTextureSet* textureSet); +[[nodiscard]] +const RE::BGSTextureSet* GetSeasonalSwap(const RE::BGSTextureSet* textureSet);Benefits: documents intent, enables the compiler to catch accidental writes, and nudges callers to respect the returned pointer’s lifetime.
src/TruePBR.cpp (1)
1098-1116: Minor: avoid redundant seasonal-swap calls inside hot loop
Util::GetSeasonalSwapis called twice per texture in the quad loops. Cache the result once per pointer to reduce call overhead and make intent clearer:auto* txst = land->loadedData->defQuadTextures[quadIndex]->textureSet; if (singleton->IsPBRTextureSet(Util::GetSeasonalSwap(txst))) { … }→
auto* txst = Util::GetSeasonalSwap(land->loadedData->defQuadTextures[quadIndex]->textureSet); if (singleton->IsPBRTextureSet(txst)) { … }Same applies to
quadTextures[quadIndex][textureIndex].
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Features/TerrainHelper.cpp(2 hunks)src/Hooks.cpp(1 hunks)src/TruePBR.cpp(3 hunks)src/Utils/Game.cpp(1 hunks)src/Utils/Game.h(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Features/TerrainHelper.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Hooks.cpp
|
@hakasapl the bot had some interesting thoughts on code outside your changes that seem related to some of the swapping issues you mentioned. You should take a look and see if it makes sense. |
added nodiscard to GetSeasonalSwap fixed PBR meta not being used in SetupLandscapeTexture
|
@alandtse do you know why this would crash on startup (just after bethesda logo)? I tried debugging but I guess the crash happens outside CS because the debugger doesn't catch anything. It only crashes now that it is up to date with dev, it was not crashing before so I tried looking at some of those commits but I can't seem to pin down what would cause it. |
|
Git bisect. Are you sure it's not happening in regular dev? |
|
@alandtse this should be ready now, users report test build working I've also pushed this code to the ENB version of terrain helper and I have no complaints from nexus users there thus far |
|
✅ A pre-release build is available for this PR: |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
…s#1099) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
|
Closes #891 |
…s#1099) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
This allows the vanilla function to execute and create the vanilla materials and shaders. We technically do not need the materials and shaders to be created for PBR but it allows hooks from other plugins to run.
This enables PBR to see the updated swaps from seasons of skyrim for example. There is a 2nd part of fixing seasons with PBR which is in this PR for seasons of skyrim: powerof3/SeasonsOfSkyrim#7
Let me know if there is a better way to avoid the entire vanilla function running in addition to PBR, but I couldn't think of one where PBR would work with seasons.
Tested with a PBR landscape retexture, seasonal swaps work fine now including special features like glint on the swapped parts.
Other Attempted Fixes
Summary by CodeRabbit
Summary by CodeRabbit