refactor(pbr): use feature boundary#2210
Merged
Merged
Conversation
Contributor
|
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 (20)
💤 Files with no reviewable changes (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughRefactors TruePBR into a Feature subclass, moves the global TruePBR into ChangesTruePBR Feature Conversion
Global and Feature System Integration
Hook Decoupling and Shader Permutation Dispatch
TerrainHelper Post-load Hooks
Menu / State UI and Boot Cleanup
Sequence Diagram(s)sequenceDiagram
participant BSShaderLoader as "BSShader Loader"
participant FeatureRegistry as "Feature Registry"
participant Feature as "Feature (e.g. TruePBR)"
participant DiskCache as "Disk Cache / Writer"
BSShaderLoader->>FeatureRegistry: ForEachLoadedFeature("GenerateShaderPermutations")
FeatureRegistry->>Feature: call GenerateShaderPermutations(shader)
Feature-->>FeatureRegistry: optionally emit permutation descriptors
FeatureRegistry->>DiskCache: collect permutations -> write cache
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Review rate limit: 8/10 reviews remaining, refill in 8 minutes and 33 seconds. Comment |
|
No actionable suggestions for changed features. |
alandtse
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/TruePBR.cpp (1)
187-191: Drop redundant nullptr re-checks.Both
Savebuttons are already insideif (selectedPbrTextureSet != nullptr)(line 114) andif (selectedPbrMaterialObject != nullptr)(line 201) respectively, so the inner re-checks at lines 187 and 293 are dead branches.♻️ Proposed cleanup
- if (selectedPbrTextureSet != nullptr) { - if (ImGui::Button("Save")) { - PNState::SavePBRRecordConfig("Data\\PBRTextureSets", selectedPbrTextureSetName, *selectedPbrTextureSet); - } + if (ImGui::Button("Save")) { + PNState::SavePBRRecordConfig("Data\\PBRTextureSets", selectedPbrTextureSetName, *selectedPbrTextureSet); }- if (selectedPbrMaterialObject != nullptr) { - if (ImGui::Button("Save")) { - PNState::SavePBRRecordConfig("Data\\PBRMaterialObjects", selectedPbrMaterialObjectName, *selectedPbrMaterialObject); - } + if (ImGui::Button("Save")) { + PNState::SavePBRRecordConfig("Data\\PBRMaterialObjects", selectedPbrMaterialObjectName, *selectedPbrMaterialObject); }Also applies to: 293-297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TruePBR.cpp` around lines 187 - 191, Remove the redundant nullptr checks inside the inner Save button blocks: the ImGui::Button("Save") handlers that call PNState::SavePBRRecordConfig already live inside outer guards that ensure selectedPbrTextureSet and selectedPbrMaterialObject are non-null, so delete the inner "if (selectedPbrTextureSet != nullptr)" and "if (selectedPbrMaterialObject != nullptr)" checks and leave the button + PNState::SavePBRRecordConfig calls directly under the existing outer guards; this simplifies the control flow while preserving the same behavior for selectedPbrTextureSet, selectedPbrTextureSetName and selectedPbrMaterialObject.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/TruePBR.cpp`:
- Around line 187-191: Remove the redundant nullptr checks inside the inner Save
button blocks: the ImGui::Button("Save") handlers that call
PNState::SavePBRRecordConfig already live inside outer guards that ensure
selectedPbrTextureSet and selectedPbrMaterialObject are non-null, so delete the
inner "if (selectedPbrTextureSet != nullptr)" and "if (selectedPbrMaterialObject
!= nullptr)" checks and leave the button + PNState::SavePBRRecordConfig calls
directly under the existing outer guards; this simplifies the control flow while
preserving the same behavior for selectedPbrTextureSet,
selectedPbrTextureSetName and selectedPbrMaterialObject.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 60ebc2fd-d17c-4c01-b3e3-dcb29650b1a2
📒 Files selected for processing (20)
features/TruePBR/COREfeatures/TruePBR/Shaders/Features/TruePBR.inisrc/Deferred.cppsrc/Feature.cppsrc/Feature.hsrc/FeatureBuffer.cppsrc/Features/TerrainHelper.cppsrc/Features/TerrainHelper.hsrc/Globals.cppsrc/Globals.hsrc/Hooks.cppsrc/Menu.cppsrc/Menu/AdvancedSettingsRenderer.cppsrc/Menu/AdvancedSettingsRenderer.hsrc/State.cppsrc/State.hsrc/TruePBR.cppsrc/TruePBR.hsrc/TruePBR/BSLightingShaderMaterialPBR.cppsrc/XSEPlugin.cpp
💤 Files with no reviewable changes (7)
- src/State.h
- src/XSEPlugin.cpp
- src/Menu/AdvancedSettingsRenderer.h
- src/Deferred.cpp
- src/Menu.cpp
- src/Menu/AdvancedSettingsRenderer.cpp
- src/FeatureBuffer.cpp
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Feature.cpp (1)
213-231: Document the TruePBR-before-TerrainHelper ordering.
PostPostLoad()runs inGetFeatureList()order, and bothsrc/TruePBR.cppandsrc/Features/TerrainHelper.cpphookTESObjectLAND/BSLightingShader::SetupMaterial. The current flag-8 coordination only works becausetruePBRis listed beforeterrainHelper. A short comment here would make that coupling much harder to break in a future reorder.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Feature.cpp` around lines 213 - 231, Add a brief comment in the feature list to document the required ordering of truePBR before terrainHelper: explain that PostPostLoad() runs in GetFeatureList() order and both src/TruePBR.cpp (truePBR) and src/Features/TerrainHelper.cpp (terrainHelper) hook TESObjectLAND / BSLightingShader::SetupMaterial, so the current flag-8 coordination depends on truePBR appearing before terrainHelper; place this comment adjacent to the &globals::features::truePBR and &globals::features::terrainHelper entries so future reorders preserve the dependency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Feature.cpp`:
- Around line 213-231: Add a brief comment in the feature list to document the
required ordering of truePBR before terrainHelper: explain that PostPostLoad()
runs in GetFeatureList() order and both src/TruePBR.cpp (truePBR) and
src/Features/TerrainHelper.cpp (terrainHelper) hook TESObjectLAND /
BSLightingShader::SetupMaterial, so the current flag-8 coordination depends on
truePBR appearing before terrainHelper; place this comment adjacent to the
&globals::features::truePBR and &globals::features::terrainHelper entries so
future reorders preserve the dependency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 379ab4a2-6163-49c0-9625-02e6dd09fc4a
📒 Files selected for processing (20)
features/TruePBR/COREfeatures/TruePBR/Shaders/Features/TruePBR.inisrc/Deferred.cppsrc/Feature.cppsrc/Feature.hsrc/FeatureBuffer.cppsrc/Features/TerrainHelper.cppsrc/Features/TerrainHelper.hsrc/Globals.cppsrc/Globals.hsrc/Hooks.cppsrc/Menu.cppsrc/Menu/AdvancedSettingsRenderer.cppsrc/Menu/AdvancedSettingsRenderer.hsrc/State.cppsrc/State.hsrc/TruePBR.cppsrc/TruePBR.hsrc/TruePBR/BSLightingShaderMaterialPBR.cppsrc/XSEPlugin.cpp
💤 Files with no reviewable changes (7)
- src/FeatureBuffer.cpp
- src/State.h
- src/Menu/AdvancedSettingsRenderer.h
- src/XSEPlugin.cpp
- src/Menu.cpp
- src/Deferred.cpp
- src/Menu/AdvancedSettingsRenderer.cpp
✅ Files skipped from review due to trivial changes (1)
- features/TruePBR/Shaders/Features/TruePBR.ini
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Features/TerrainHelper.h
- src/Globals.cpp
- src/TruePBR/BSLightingShaderMaterialPBR.cpp
|
✅ A pre-release build is available for this PR: |
alandtse
added
deferred
TruePBR now inherits from Feature and participates in the standard ForEachLoadedFeature lifecycle instead of being special-cased throughout the codebase. Removes the singleton pattern, specialFeatures map, and direct lifecycle calls scattered across State, Deferred, XSEPlugin, and Menu. Adds IsCore(), GetFeatureSummary(), and correct category/VR flags. Creates features/TruePBR/CORE marker for AIO packaging. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fold SetupFrame() into Prepass() — removes the direct call from Deferred.cpp - Add virtual GenerateShaderPermutations(RE::BSShader*) to Feature base; Hooks.cpp now calls it via ForEachLoadedFeature instead of a direct TruePBR reference, making the hook extensible to any future feature - Move TESObjectLAND and BSLightingShader material hook installation from Hooks::Install() into TruePBR::PostPostLoad(), removing Hooks.cpp's dependency on TruePBR entirely - TerrainHelper installs its own TESObjectLAND and BSLightingShader hooks in its new PostPostLoad(), removing TerrainHelper calls from TruePBR's hook bodies; TerrainHelper skips PBR land cells via the flag-8 sentinel set by TruePBR::TESObjectLAND_SetupMaterial Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Set IsInMenu() = true so TruePBR appears in the regular feature sidebar under the Materials category, where DrawSettings() is already called for all loaded features. Remove the dedicated "PBR Settings" tab from the Advanced menu and its RenderPBRSection helper. Add Doxygen comment to Feature::GenerateShaderPermutations explaining its contract and cold-path usage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…messages Follows the [FEATURE_NAME] convention used across the codebase (e.g. [SKYLIGHTING]) so hook installation messages are attributable in the log without ambiguity. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Core features still need a version INI so the standard Feature::Load path (version check, settings load, error reporting) works without any special-casing. Adding features/TruePBR/Shaders/Features/TruePBR.ini at version 1-0-0 also registers TruePBR in the CMake-generated FEATURE_MINIMAL_VERSIONS map, making IsFeatureKnown() return true. Reverts the incorrect virtual keyword on Feature::Load(json&) and the TruePBR::Load override that was the original workaround. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
SkrubbySkrubInAShrub
approved these changes
May 3, 2026
doodlum
approved these changes
May 3, 2026
This was referenced May 11, 2026
SkrubbySkrubInAShrub
pushed a commit
that referenced
this pull request
May 14, 2026
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
ParticleTroned
pushed a commit
to ParticleTroned/skyrim-community-shaders
that referenced
this pull request
May 15, 2026
Branch-preserving adaptation of upstream community-shaders#2210: keep this branch's combined TruePBR/TerrainHelper material hook path and existing PBR settings UI while routing TruePBR lifecycle through Feature. Do not add HDR, Exponential Height Fog, or Volumetric Shadows feature wiring. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
ParticleTroned
pushed a commit
to ParticleTroned/skyrim-community-shaders
that referenced
this pull request
May 16, 2026
Branch-preserving adaptation of upstream community-shaders#2210: keep this branch's combined TruePBR/TerrainHelper material hook path and existing PBR settings UI while routing TruePBR lifecycle through Feature. Do not add HDR, Exponential Height Fog, or Volumetric Shadows feature wiring. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
alandtse
added a commit
that referenced
this pull request
May 18, 2026
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
added a commit
that referenced
this pull request
May 18, 2026
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
added a commit
that referenced
this pull request
May 18, 2026
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.
IgorAlanAlbuquerque
pushed a commit
to IgorAlanAlbuquerque/skyrim-community-shaders
that referenced
this pull request
May 29, 2026
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
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
3 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.
Summary by CodeRabbit
User Interface
New Features
Refactor