fix: mesh external emittance in interiors#2294
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds conditional suppression of external emittance indoors: new permutation flag, shader lighting helper refactor, effect shader conditional use of scene lighting, C++ helpers to update permutations, hook integration, and a version bump. ChangesExternal Emittance Suppression
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 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.13][ERROR]: Error: exception Unix_error: No such file or directory stat src/Utils/ExternalEmittance.h 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Features/LightLimitFix.cpp (1)
565-572:⚠️ Potential issue | 🔴 CriticalCritical vtable slot conflict: only one BSEffectShader_SetupGeometry hook can be active.
Both
LightLimitFix::Hooks::BSEffectShader_SetupGeometry::thunkandEffectExtensions::BSEffectShader_SetupGeometry::thunkwrite to the same virtual function slot (0x6) onRE::VTABLE_BSEffectShader[0]. The second hook installation completely overwrites the first, causing one feature's functionality to be silently disabled.
src/Features/LightLimitFix.hline 229:stl::write_vfunc<0x6, BSEffectShader_SetupGeometry>(...)src/Hooks.cppline 961:stl::write_vfunc<0x6, EffectExtensions::BSEffectShader_SetupGeometry>(...)Additionally,
EffectExtensions::BSEffectShader_SetupGeometry::thunkupdatesglobals::state->permutationData.EffectRadius, which the LightLimitFix version does not perform. Resolve this by implementing hook chaining or merging the functionality into a single hook handler.🤖 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/LightLimitFix.cpp` around lines 565 - 572, Two hooks are clobbering vtable slot 0x6 for BSEffectShader_SetupGeometry so only one runs; fix by merging their logic into a single handler and install only that handler via stl::write_vfunc<0x6,...>. Create/replace the thunk for BSEffectShader_SetupGeometry so it calls the original func(This, Pass, RenderFlags), then calls ExternalEmittance::UpdatePermutation(Pass), updates globals::state->permutationData.EffectRadius with the same logic found in EffectExtensions::BSEffectShader_SetupGeometry::thunk, and finally invokes both feature entry points (LightLimitFix singleton: BSLightingShader_SetupGeometry_Before and _After and EffectExtensions behavior if any) in the correct order; remove the second stl::write_vfunc<0x6,...> install so only the merged handler is written to slot 0x6.
🧹 Nitpick comments (1)
src/ShaderTools/ExternalEmittance.h (1)
1-7: 💤 Low valueConsider adding a forward declaration for
RE::BSRenderPass.The header uses
RE::BSRenderPass*without including the necessary type definition. While this works because all current includers already have the RE headers, adding a forward declaration would make the header self-contained.♻️ Suggested improvement
`#pragma` once +namespace RE +{ + class BSRenderPass; +} + namespace ExternalEmittance { bool ShouldSuppress(const RE::BSRenderPass* a_pass); void UpdatePermutation(const RE::BSRenderPass* a_pass); }🤖 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/ShaderTools/ExternalEmittance.h` around lines 1 - 7, The header declares functions ShouldSuppress and UpdatePermutation that take RE::BSRenderPass* but doesn't forward-declare RE::BSRenderPass; make the header self-contained by adding a forward declaration for namespace RE and class BSRenderPass (so the declarations of ShouldSuppress and UpdatePermutation compile without requiring includers to already include RE headers).
🤖 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.
Outside diff comments:
In `@src/Features/LightLimitFix.cpp`:
- Around line 565-572: Two hooks are clobbering vtable slot 0x6 for
BSEffectShader_SetupGeometry so only one runs; fix by merging their logic into a
single handler and install only that handler via stl::write_vfunc<0x6,...>.
Create/replace the thunk for BSEffectShader_SetupGeometry so it calls the
original func(This, Pass, RenderFlags), then calls
ExternalEmittance::UpdatePermutation(Pass), updates
globals::state->permutationData.EffectRadius with the same logic found in
EffectExtensions::BSEffectShader_SetupGeometry::thunk, and finally invokes both
feature entry points (LightLimitFix singleton:
BSLightingShader_SetupGeometry_Before and _After and EffectExtensions behavior
if any) in the correct order; remove the second stl::write_vfunc<0x6,...>
install so only the merged handler is written to slot 0x6.
---
Nitpick comments:
In `@src/ShaderTools/ExternalEmittance.h`:
- Around line 1-7: The header declares functions ShouldSuppress and
UpdatePermutation that take RE::BSRenderPass* but doesn't forward-declare
RE::BSRenderPass; make the header self-contained by adding a forward declaration
for namespace RE and class BSRenderPass (so the declarations of ShouldSuppress
and UpdatePermutation compile without requiring includers to already include RE
headers).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c8d3124-bb39-4487-b5df-da5c8ee83769
📒 Files selected for processing (8)
package/Shaders/Common/Permutation.hlslipackage/Shaders/Common/ShadowSampling.hlslipackage/Shaders/Effect.hlslsrc/Features/LightLimitFix.cppsrc/Hooks.cppsrc/ShaderTools/ExternalEmittance.cppsrc/ShaderTools/ExternalEmittance.hsrc/State.h
|
✅ A pre-release build is available for this PR: |
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
Shadertools is deprecated and shouldn't be used for anything. It's only for historical reasons. Can this be properly placed in the utils directory or some other area? If this should go into commmonlib, also consider putting it there.
Moved to utils. This doesn't seem low level enough to include in commonlib. |
Catches up with 4 commits from upstream/dev: * refactor(hlsl): use Game constants (community-shaders#2298) * fix: mesh external emittance in interiors (community-shaders#2294) -- adds Utils/ExternalEmittance, hooks LLF's BSEffectShader_SetupGeometry via ExternalEmittance::UpdatePermutation(Pass) * fix(grass-collision): avoid sorting actor smart pointers (community-shaders#2299) * fix(ishdr): gate bloom for legacy weather mods (community-shaders#2297) Conflicts resolved: * features/Inverse Square Lighting/Shaders/Features/InverseSquareLighting.ini Took upstream's 1-3-0 (their refactor; we hadn't bumped ISL). * features/Light Limit Fix/Shaders/Features/LightLimitFix.ini Kept our 3-1-0 -- supersedes upstream's 3-0-3 because our UI consolidation is the larger structural change. Upstream's external-emittance hook is folded in alongside. * src/Features/LightLimitFix.cpp Kept BOTH include sets: our Deferred.h plus upstream's Menu/ThemeManager.h and Utils/ExternalEmittance.h. The BSEffectShader_SetupGeometry hook gained ExternalEmittance::UpdatePermutation(Pass) cleanly via the auto-merger -- no manual reconciliation needed there. Build verified: ALL preset compiles cleanly post-merge.
Catches up with 4 commits from upstream/dev: * refactor(hlsl): use Game constants (#2298) * fix: mesh external emittance in interiors (#2294) -- adds Utils/ExternalEmittance, hooks LLF's BSEffectShader_SetupGeometry via ExternalEmittance::UpdatePermutation(Pass) * fix(grass-collision): avoid sorting actor smart pointers (#2299) * fix(ishdr): gate bloom for legacy weather mods (#2297) Conflicts resolved: * features/Inverse Square Lighting/Shaders/Features/InverseSquareLighting.ini Took upstream's 1-3-0 (their refactor; we hadn't bumped ISL). * features/Light Limit Fix/Shaders/Features/LightLimitFix.ini Kept our 3-1-0 -- supersedes upstream's 3-0-3 because our UI consolidation is the larger structural change. Upstream's external-emittance hook is folded in alongside. * src/Features/LightLimitFix.cpp Kept BOTH include sets: our Deferred.h plus upstream's Menu/ThemeManager.h and Utils/ExternalEmittance.h. The BSEffectShader_SetupGeometry hook gained ExternalEmittance::UpdatePermutation(Pass) cleanly via the auto-merger -- no manual reconciliation needed there. Build verified: ALL preset compiles cleanly post-merge.
Branch-preserving adaptation: keep this branch's existing ExtraShaderDescriptor bit layout, add SuppressExternalEmittance as a new bit, and wire the suppression through the current effect-shadow/IBL path.
Branch-preserving adaptation: keep this branch's existing ExtraShaderDescriptor bit layout, add SuppressExternalEmittance as a new bit, and wire the suppression through the current effect-shadow/IBL path.
2 participants
Fixes black effect meshes flagged to use external emittance in interiors.
If it is an interior, and no record defining emittance is set, external emittance is disabled on the mesh.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Version