Conversation
WalkthroughThree new hook structs—ValidLight1, ValidLight2, and ValidLight3—were introduced in the LightLimitFix::Hooks namespace. Each struct provides a thunk function that wraps an original function pointer and adds extra validation logic. The Install() method was updated to install these hooks at specific relocated addresses. Changes
Sequence Diagram(s)sequenceDiagram
participant ClientCode
participant ValidLightX (thunk)
participant OriginalFunc
participant Light
ClientCode->>ValidLightX (thunk): Call(RE::BSShaderProperty*, RE::BSLight*)
ValidLightX (thunk)->>OriginalFunc: Call original function with parameters
OriginalFunc-->>ValidLightX (thunk): Return bool
ValidLightX (thunk)->>Light: Check portalStrict/portalGraph
ValidLightX (thunk)-->>ClientCode: Return true if both checks pass
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 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
Documentation and Community
|
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Features/LightLimitFix.h (1)
320-322: Consider adding error handling for hook installation.The hook installations don't have error handling, which could make debugging difficult if installation fails. Consider adding validation or logging for failed installations.
- stl::write_thunk_call<ValidLight1>(REL::RelocationID(100994, 107781).address() + 0x92); - stl::write_thunk_call<ValidLight2>(REL::RelocationID(100997, 107784).address() + REL::Relocate(0x139, 0x12A)); - stl::write_thunk_call<ValidLight3>(REL::RelocationID(101296, 108283).address() + REL::Relocate(0xB7, 0x7E)); + try { + stl::write_thunk_call<ValidLight1>(REL::RelocationID(100994, 107781).address() + 0x92); + stl::write_thunk_call<ValidLight2>(REL::RelocationID(100997, 107784).address() + REL::Relocate(0x139, 0x12A)); + stl::write_thunk_call<ValidLight3>(REL::RelocationID(101296, 108283).address() + REL::Relocate(0xB7, 0x7E)); + } catch (const std::exception& e) { + logger::error("[LLF] Failed to install ValidLight hooks: {}", e.what()); + throw; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Features/LightLimitFix.h(2 hunks)
🔇 Additional comments (1)
src/Features/LightLimitFix.h (1)
281-306: Verify the light filtering logic aligns with performance goals.The validation logic
(a_light->portalStrict || !a_light->portalGraph)filters lights based on portal properties. Please confirm this logic correctly identifies lights that can skip per-object sorting to achieve the stated performance improvements.#!/bin/bash # Description: Search for usage of portalStrict and portalGraph to understand their purpose # Expected: Find context about how these properties are used in light processing echo "Searching for portalStrict usage..." rg -A 3 -B 3 "portalStrict" echo "Searching for portalGraph usage..." rg -A 3 -B 3 "portalGraph" echo "Searching for light validation patterns..." rg -A 5 -B 5 "BSLight.*portal"
| static bool thunk(RE::BSShaderProperty* a_property, RE::BSLight* a_light) | ||
| { | ||
| return func(a_property, a_light) && (a_light->portalStrict || !a_light->portalGraph); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null pointer validation for runtime safety.
The thunk functions access a_light->portalStrict and a_light->portalGraph without null checks, which could cause crashes if a null pointer is passed.
static bool thunk(RE::BSShaderProperty* a_property, RE::BSLight* a_light)
{
- return func(a_property, a_light) && (a_light->portalStrict || !a_light->portalGraph);
+ return a_light && func(a_property, a_light) && (a_light->portalStrict || !a_light->portalGraph);
}Also applies to: 292-294, 301-303
🤖 Prompt for AI Agents
In src/Features/LightLimitFix.h at lines 283-285, 292-294, and 301-303, the
thunk functions access a_light->portalStrict and a_light->portalGraph without
checking if a_light is null, risking crashes. Add null pointer checks for
a_light before accessing its members by returning false or handling the null
case appropriately to ensure runtime safety.
| struct ValidLight1 | ||
| { | ||
| static bool thunk(RE::BSShaderProperty* a_property, RE::BSLight* a_light) | ||
| { | ||
| return func(a_property, a_light) && (a_light->portalStrict || !a_light->portalGraph); | ||
| } | ||
| static inline REL::Relocation<decltype(thunk)> func; | ||
| }; | ||
|
|
||
| struct ValidLight2 | ||
| { | ||
| static bool thunk(RE::BSShaderProperty* a_property, RE::BSLight* a_light) | ||
| { | ||
| return func(a_property, a_light) && (a_light->portalStrict || !a_light->portalGraph); | ||
| } | ||
| static inline REL::Relocation<decltype(thunk)> func; | ||
| }; | ||
|
|
||
| struct ValidLight3 | ||
| { | ||
| static bool thunk(RE::BSShaderProperty* a_property, RE::BSLight* a_light) | ||
| { | ||
| return func(a_property, a_light) && (a_light->portalStrict || !a_light->portalGraph); | ||
| } | ||
| static inline REL::Relocation<decltype(thunk)> func; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Eliminate code duplication by creating a template or shared implementation.
All three ValidLight structs contain identical logic, which violates the DRY principle and increases maintenance overhead. Consider refactoring to use a template or shared base implementation.
- struct ValidLight1
- {
- static bool thunk(RE::BSShaderProperty* a_property, RE::BSLight* a_light)
- {
- return func(a_property, a_light) && (a_light->portalStrict || !a_light->portalGraph);
- }
- static inline REL::Relocation<decltype(thunk)> func;
- };
-
- struct ValidLight2
- {
- static bool thunk(RE::BSShaderProperty* a_property, RE::BSLight* a_light)
- {
- return func(a_property, a_light) && (a_light->portalStrict || !a_light->portalGraph);
- }
- static inline REL::Relocation<decltype(thunk)> func;
- };
-
- struct ValidLight3
- {
- static bool thunk(RE::BSShaderProperty* a_property, RE::BSLight* a_light)
- {
- return func(a_property, a_light) && (a_light->portalStrict || !a_light->portalGraph);
- }
- static inline REL::Relocation<decltype(thunk)> func;
- };
+ template<int N>
+ struct ValidLight
+ {
+ static bool thunk(RE::BSShaderProperty* a_property, RE::BSLight* a_light)
+ {
+ return func(a_property, a_light) && (a_light->portalStrict || !a_light->portalGraph);
+ }
+ static inline REL::Relocation<decltype(thunk)> func;
+ };
+
+ using ValidLight1 = ValidLight<1>;
+ using ValidLight2 = ValidLight<2>;
+ using ValidLight3 = ValidLight<3>;🤖 Prompt for AI Agents
In src/Features/LightLimitFix.h between lines 281 and 306, the three ValidLight
structs have identical thunk functions causing code duplication. Refactor by
creating a single templated struct or a shared base struct that implements the
thunk function once, then instantiate or inherit it for ValidLight1,
ValidLight2, and ValidLight3 to eliminate redundancy and improve
maintainability.
|
✅ A pre-release build is available for this PR: |
…shaders#1185) * perf(llf): significantly improve scaling with many lights * style: 🎨 apply pre-commit.ci formatting 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>
2 participants
Re-adds old hooks that skips sorting the majority of lights per-object which leads to CPU improvements. Skips casting because that was slow.
Summary by CodeRabbit