fix(terrain helper): fix terrain helper hook#2320
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughIntroduce append-only HookChain slots in ChangesMaterial Hooks Centralization
Sequence Diagram(s)sequenceDiagram
participant TESObjectLAND
participant Hooks_cpp as Hooks.cpp
participant TruePBR
participant BSLightingShader
participant TerrainHelper
TESObjectLAND->>Hooks_cpp: call SetupMaterial()
Hooks_cpp->>TruePBR: run TESObjectLANDPost interceptors
alt handled
TruePBR-->>Hooks_cpp: handled (early return)
Hooks_cpp-->>TESObjectLAND: return handled
else not handled
TruePBR-->>Hooks_cpp: not handled
Hooks_cpp->>BSLightingShader: call original/vfunc SetupMaterial
BSLightingShader-->>Hooks_cpp: original result
Hooks_cpp->>TerrainHelper: run BSLightingShader post callbacks
TerrainHelper-->>Hooks_cpp: applied
Hooks_cpp-->>TESObjectLAND: return final result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.20.0)OpenGrep fatal error (exit code 2): [00.19][ERROR]: Error: exception Unix_error: No such file or directory stat src/Features/TerrainHelper.cpp 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 |
|
No actionable suggestions for changed features. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Hooks.cpp`:
- Around line 952-957: The hooks for TESObjectLAND_SetupMaterial and
BSLightingShader_SetupMaterial are installed unconditionally using SE/AE-only
relocation/vtable access and will crash on VR builds; wrap the
stl::detour_thunk<TESObjectLAND_SetupMaterial>(REL::RelocationID(18368, 18791))
and stl::write_vfunc<0x4,
BSLightingShader_SetupMaterial>(RE::VTABLE_BSLightingShader[0]) calls in a
VR-safe guard (e.g., if (!REL::Module::IsVR()) { ... }) or add VR
relocation/vtable paths similar to the pattern used at lines 947–950 so these
hooks are only applied when not running VR.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 264b89bc-6109-4ce9-b0b8-50001e8be4d9
📒 Files selected for processing (3)
src/Features/TerrainHelper.cppsrc/Hooks.cppsrc/TruePBR.cpp
💤 Files with no reviewable changes (2)
- src/Features/TerrainHelper.cpp
- src/TruePBR.cpp
|
✅ A pre-release build is available for this PR: |
SkrubbySkrubInAShrub
left a comment
SkrubbySkrubInAShrub
left a comment
There was a problem hiding this comment.
doesn't work
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
Setup hook registration. Do not violate the feature boundary.
|
I'll have a fix incoming shortly since no one looks like they're working on this. |
alandtse
force-pushed
the
fix-terrain-helper
branch
from
4deca10 to
c38d208
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Features/TerrainHelper.cpp (1)
200-218: 💤 Low valueConsider extracting magic number to a named constant.
The flag value
8on line 209 has a comment explaining its purpose, but extracting it to a named constant would improve maintainability and make the cross-feature contract with TruePBR more explicit.♻️ Suggested refactor
Add a constant (perhaps in a shared header or at namespace scope):
// Flag set by TruePBR on land cells it processes; TerrainHelper should skip these. inline constexpr auto kPBRProcessedFlag = static_cast<RE::OBJ_LAND::Flag>(8);Then use it in the callback:
- if (land->data.flags.any(static_cast<RE::OBJ_LAND::Flag>(8))) { + if (land->data.flags.any(kPBRProcessedFlag)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Features/TerrainHelper.cpp` around lines 200 - 218, The literal flag value 8 used in the TESObjectLANDPost callback is a magic number; replace it with a named constant to clarify the cross-feature contract with TruePBR and improve maintainability. Add an inline constexpr constant (e.g., kPBRProcessedFlag) of type RE::OBJ_LAND::Flag at an appropriate shared/namespace scope, document that it is the flag TruePBR sets on land cells, then update the check in the lambda (land->data.flags.any(...)) to use kPBRProcessedFlag instead of the raw casted 8; keep the rest of the TESObjectLANDPost and BSLightingShaderPost registrations unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/Features/TerrainHelper.cpp`:
- Around line 200-218: The literal flag value 8 used in the TESObjectLANDPost
callback is a magic number; replace it with a named constant to clarify the
cross-feature contract with TruePBR and improve maintainability. Add an inline
constexpr constant (e.g., kPBRProcessedFlag) of type RE::OBJ_LAND::Flag at an
appropriate shared/namespace scope, document that it is the flag TruePBR sets on
land cells, then update the check in the lambda (land->data.flags.any(...)) to
use kPBRProcessedFlag instead of the raw casted 8; keep the rest of the
TESObjectLANDPost and BSLightingShaderPost registrations unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a7207b7-abbd-4dfa-ae1d-0f10708a02e8
📒 Files selected for processing (5)
features/Terrain Helper/Shaders/Features/TerrainHelper.inisrc/Features/TerrainHelper.cppsrc/Hooks.cppsrc/Hooks.hsrc/TruePBR.cpp
✅ Files skipped from review due to trivial changes (1)
- features/Terrain Helper/Shaders/Features/TerrainHelper.ini
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Hooks.h
- src/TruePBR.cpp
- src/Hooks.cpp
|
@coderabbitai is this the right scope? |
|
Tip For best results, initiate chat on the files or code changes.
This tension was already acknowledged when terrain helper was first introduced: "I had to move 2 hooks out of TruePBR class because terrain helper needs to hook them as well." The new PR formalizes a proper solution. The changes needed to fix the conflict are:
The intended behavior is for TruePBR to override terrain materials with PBR textures, and if no PBR material is found, fall back to vanilla behavior and allow TerrainHelper to apply its own modifications. The The new framework is also a net positive for maintainability — any future feature needing to hook these material functions can register a callback rather than risk clobbering an existing detour. So the scope is minimal for a correct fix, not over-engineered. |
Hooks.cpp no longer references TruePBR or TerrainHelper directly. Adds a small multi-subscriber dispatcher in Hooks::MaterialHooks that each feature registers against from its own PostPostLoad, restoring the feature boundary that PR #2210 established while keeping a single detour/vfunc install per address. Semantics preserved from the prior consolidated thunk: - TESObjectLAND::SetupMaterial: vanilla first, then post callbacks in registration order; first callback to return true claims the result. TruePBR registers a claiming post-callback; TerrainHelper observes. - BSLightingShader::SetupMaterial: interceptors before vanilla (return true to skip vanilla and the post chain), post callbacks after. TruePBR is an interceptor; TerrainHelper is post. A single HookChain<Fn> template backs each slot - range-for over a vector of function pointers, no std::function, no allocation. Adding a new dispatch slot is a one-liner accessor. Registration order is deterministic via Feature::GetFeatureList() (TruePBR ahead of TerrainHelper), so PBR's claim short-circuits TerrainHelper on PBR land cells. The flag-8 gate stays in TerrainHelper as a defensive backstop.
alandtse
force-pushed
the
fix-terrain-helper
branch
from
c38d208 to
b64c05e
Compare
|
@coderabbitai should the scope have a hyphen? |
|
Tip For best results, initiate chat on the files or code changes.
In short: |
|
@coderabbitai what is the convention for the terrain helper in the past? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2586
There's one outlier ( |
|
@SkrubbySkrubInAShrub please confirm the new build fixes it. I have no idea how to test this. Please note that since this is based on dev, grass lighting must have default settings. |
still doesn't seem to work |
3 participants
Summary by CodeRabbit
Refactor
Chores