feat(sky-sync): add sun offset and MinElevation sliders#1691
Conversation
adds sliders to control sun position via climatetimings and cap minelevation to limit shadow depth
Adds settings here that sliders reference
|
Warning Rate limit exceeded@BigBullmoose has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 14 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds five settings to SkySync (four sunrise/sunset offsets and MinShadowElevation), exposes them in the UI, serializes and clamps them on load, and applies them to climate sunrise/sunset timings and shadow-direction clamping in rendering. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Settings
participant Climate
participant Renderer
User->>UI: adjust sunrise/sunset offsets or min-shadow slider
UI->>Settings: update and serialize settings
Settings->>Climate: provide offsets for timing calculations
Climate->>Climate: apply offsets and enforce ordering/gaps
Climate->>Renderer: provide updated sun position & timing
Renderer->>Renderer: clamp shadow direction using MinShadowElevation
Renderer-->>User: render frame with updated sun/shadow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
|
Using provided base ref: ee0158a Actionable Suggestions
|
![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
🤖 Fix all issues with AI agents
In @src/Features/SkySync.cpp:
- Around line 30-34: The UI shows the Sunrise/Sunset offset sliders only when
UseAlternateSunPath is true, but ClimateTimings::Update still applies
SunriseBeginOffset, SunriseEndOffset, SunsetBeginOffset and SunsetEndOffset
unconditionally; either make the sliders visible always (move the
ImGui::SliderFloat calls out of the UseAlternateSunPath conditional) if offsets
are intended to always apply, or add the same guard in ClimateTimings::Update
(wrap the code that applies those four offsets with a check of
settings.UseAlternateSunPath) so offsets are only applied when the UI exposes
them; modify whichever path matches intended behavior and ensure you reference
the settings members (SunriseBeginOffset, SunriseEndOffset, SunsetBeginOffset,
SunsetEndOffset) and the UseAlternateSunPath flag consistently.
🧹 Nitpick comments (3)
src/Features/SkySync.h (1)
155-155: Dead code:MinElevationconstant is no longer used.This constant has been superseded by the configurable
settings.MinShadowElevation. Consider removing it to avoid confusion.🧹 Suggested removal
- static constexpr float MinElevation = 0.25f;src/Features/SkySync.cpp (2)
36-37: Minor format string nit.
"%.01f deg"works, but the leading zero is superfluous. Standard format would be"%.1f deg".✏️ Suggested fix
- ImGui::SliderFloat("Min Shadow Elevation", &settings.MinShadowElevation, 0.0f, 45.0f, "%.01f deg"); + ImGui::SliderFloat("Min Shadow Elevation", &settings.MinShadowElevation, 0.0f, 45.0f, "%.1f deg");
40-46: Consider validating new settings on load.
MoonLightSourceandSunPathare clamped after loading, but the new offset and elevation settings are not. Users editing the JSON directly could introduce out-of-range values.✏️ Suggested fix
void SkySync::LoadSettings(json& o_json) { settings = o_json; settings.MoonLightSource = std::clamp(settings.MoonLightSource, static_cast<int32_t>(MoonLightSource::Brightest), static_cast<int32_t>(MoonLightSource::Secunda)); settings.SunPath = std::clamp(settings.SunPath, static_cast<int32_t>(SunPath::Southern), static_cast<int32_t>(SunPath::Custom)); + settings.SunriseBeginOffset = std::clamp(settings.SunriseBeginOffset, -5.0f, 5.0f); + settings.SunriseEndOffset = std::clamp(settings.SunriseEndOffset, -5.0f, 5.0f); + settings.SunsetBeginOffset = std::clamp(settings.SunsetBeginOffset, -5.0f, 5.0f); + settings.SunsetEndOffset = std::clamp(settings.SunsetEndOffset, -5.0f, 5.0f); + settings.MinShadowElevation = std::clamp(settings.MinShadowElevation, 0.0f, 45.0f); SetSunAngle(); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Features/SkySync.cppsrc/Features/SkySync.h
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Features/SkySync.cppsrc/Features/SkySync.h
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/SkySync.cppsrc/Features/SkySync.h
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
src/Features/SkySync.cppsrc/Features/SkySync.h
🧠 Learnings (1)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:260-260
Timestamp: 2025-08-05T18:13:03.123Z
Learning: In the skyrim-community-shaders SnowCover feature, the time calculation uses division by 61.0 instead of 60.0 for seconds conversion in the perFrame.Month calculation. The original author ThePagi indicated this was intentional and makes no discernible difference to the snow cover functionality, suggesting it may be related to game-specific timing mechanics or balance considerations.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (4)
src/Features/SkySync.h (1)
34-38: LGTM!The new settings fields are well-structured with sensible default values. The offset defaults of
0.0fensure no change in behavior for existing users, andMinShadowElevationdefaulting to0.25fmaintains backward compatibility with the previous hardcoded constant.src/Features/SkySync.cpp (3)
3-14: LGTM!The serialization macro correctly includes all new settings. Using
NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULTensures backward compatibility with existing configuration files that don't have these new fields.
405-422: LGTM!Clean integration of the configurable
MinShadowElevationsetting. The conversion from degrees (user-friendly UI) to radians (internal calculation) is handled correctly.
432-449: Edge case: extreme offsets could invert timing order.With ±5 hour offset ranges, users could theoretically set
SunriseBeginOffset>SunriseEndOffsetin ways that result insunriseBegin > sunriseEnd. This could cause unexpected behavior in calculations likeCalculateAlternateSunDirectionAndDistancewhich divides by(sunset - sunrise).Consider adding a post-offset sanity check or documenting this constraint for users.
BigBullmoose
changed the title
|
✅ A pre-release build is available for this PR: |
davo0411
changed the title
davo0411
left a comment
davo0411
left a comment
There was a problem hiding this comment.
Please address AI concerns, they seem valid.
- UI visibility doesn't match behavior: offsets are applied even when UseAlternateSunPath is disabled.
- 155-155: Dead code: MinElevation constant is no longer used.
This constant has been superseded by the configurable settings.MinShadowElevation. Consider removing it to avoid confusion.
3.36-37: Minor format string nit.
"%.01f deg" works, but the leading zero is superfluous. Standard format would be "%.1f deg".
this makes sense, 0.01 degree is an incredibly small value.
- 40-46: Consider validating new settings on load.
MoonLightSource and SunPath are clamped after loading, but the new offset and elevation settings are not. Users editing the JSON directly could introduce out-of-range values.
These fixes are all minor, you can vibe code them or just directly replace with what coderabbit/github copilot have already provided. Just make sure to test afterwards.
Good first attempt at conventional commit titling. See my fixes for that and take on board for next time :)
As this is your first PR i want to be more accomodating, but in future comments will be less fleshed out, just like "address ai concerns" and it will be expected titling is correct.
Cleaned up UI with treenode and tooltip to keep things organized and nice looking. Added new settings to LoadSettings. Fixed format string nit.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Removed old constant value MinElevation, been replaced by MinShadowElevation float
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/Features/SkySync.cpp:
- Around line 39-48: The ImGui::TreePop() is being called unconditionally even
when ImGui::TreeNodeEx("Sun Position Offsets", ImGuiTreeNodeFlags_DefaultOpen)
returns false, which corrupts the ImGui ID/stack; move the ImGui::TreePop() call
inside the if-block that surrounds the sliders (i.e., after the last
ImGui::SliderFloat for Sunset End) so TreePop is only invoked when TreeNodeEx
returned true; ensure any ImGui::Spacing() calls remain outside or are likewise
adjusted to preserve intended layout.
🧹 Nitpick comments (1)
src/Features/SkySync.cpp (1)
450-458: Consider more descriptive variable names for clarity.The offset application logic is correct, but the abbreviated variable names (
srbegin,srend,ssbegin,ssend) could be more readable.♻️ Optional: Use more descriptive variable names
- float srbegin = globals::features::skySync.settings.SunriseBeginOffset; - float srend = globals::features::skySync.settings.SunriseEndOffset; - float ssbegin = globals::features::skySync.settings.SunsetBeginOffset; - float ssend = globals::features::skySync.settings.SunsetEndOffset; + float sunriseBeginOffset = globals::features::skySync.settings.SunriseBeginOffset; + float sunriseEndOffset = globals::features::skySync.settings.SunriseEndOffset; + float sunsetBeginOffset = globals::features::skySync.settings.SunsetBeginOffset; + float sunsetEndOffset = globals::features::skySync.settings.SunsetEndOffset; - sunriseBegin = (climate->timing.sunrise.begin / 6.0f) + srbegin; - sunriseEnd = (climate->timing.sunrise.end / 6.0f) + srend; - sunsetBegin = (climate->timing.sunset.begin / 6.0f) + ssbegin; - sunsetEnd = (climate->timing.sunset.end / 6.0f) + ssend; + sunriseBegin = (climate->timing.sunrise.begin / 6.0f) + sunriseBeginOffset; + sunriseEnd = (climate->timing.sunrise.end / 6.0f) + sunriseEndOffset; + sunsetBegin = (climate->timing.sunset.begin / 6.0f) + sunsetBeginOffset; + sunsetEnd = (climate->timing.sunset.end / 6.0f) + sunsetEndOffset;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Features/SkySync.cppsrc/Features/SkySync.h
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Features/SkySync.cppsrc/Features/SkySync.h
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/SkySync.cppsrc/Features/SkySync.h
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
src/Features/SkySync.cppsrc/Features/SkySync.h
🧠 Learnings (1)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (4)
src/Features/SkySync.h (1)
34-38: LGTM! New settings fields are well-defined.The new fields have sensible defaults, and
MinShadowElevationmaintains backward compatibility by using the same default value (0.25f) as the removedMinElevationconstant.src/Features/SkySync.cpp (3)
1-14: LGTM! Serialization properly handles new settings.All five new settings fields are correctly included in the serialization macro.
57-61: LGTM! Settings validation is correct.The clamping ranges properly match the UI slider constraints, ensuring settings remain valid even if loaded from manually edited configuration files.
423-425: LGTM! Correctly uses configurable MinShadowElevation.The hardcoded constant has been properly replaced with the settings value, enabling runtime configuration of the minimum shadow elevation.
Removed some accidental spacing
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Features/SkySync.cpp (2)
48-59: ClampCustomAngleon load (and do it beforeSetSunAngle()).Right now offsets/min elevation are clamped, but
CustomAnglecan be out-of-range if a user edits JSON, andSetSunAngle()is called before any clamp pass.Proposed fix
void SkySync::LoadSettings(json& o_json) { settings = o_json; settings.MoonLightSource = std::clamp(settings.MoonLightSource, static_cast<int32_t>(MoonLightSource::Brightest), static_cast<int32_t>(MoonLightSource::Secunda)); settings.SunPath = std::clamp(settings.SunPath, static_cast<int32_t>(SunPath::Southern), static_cast<int32_t>(SunPath::Custom)); - SetSunAngle(); + settings.CustomAngle = std::clamp(settings.CustomAngle, -90.0f, 90.0f); settings.SunriseBeginOffset = std::clamp(settings.SunriseBeginOffset, -5.0f, 5.0f); settings.SunriseEndOffset = std::clamp(settings.SunriseEndOffset, -5.0f, 5.0f); settings.SunsetBeginOffset = std::clamp(settings.SunsetBeginOffset, -5.0f, 5.0f); settings.SunsetEndOffset = std::clamp(settings.SunsetEndOffset, -5.0f, 5.0f); settings.MinShadowElevation = std::clamp(settings.MinShadowElevation, 0.0f, 45.0f); + SetSunAngle(); }
445-462: Validate sunrise/sunset ordering after applying offsets (avoid div-by-zero/NaNs in alternate sun path).Offsets can make
sunrise >= sunset(or invert begin/end), which can later explode inCalculateAlternateSunDirectionAndDistance()due to(sunset - sunrise)in the denominator. At minimum, enforce a small monotonic separation after offsets are applied.Example hardening (minimal, non-wraparound)
void SkySync::ClimateTimings::Update(const RE::TESClimate* climate) { - float srbegin = globals::features::skySync.settings.SunriseBeginOffset; - float srend = globals::features::skySync.settings.SunriseEndOffset; - float ssbegin = globals::features::skySync.settings.SunsetBeginOffset; - float ssend = globals::features::skySync.settings.SunsetEndOffset; + const float srbegin = globals::features::skySync.settings.SunriseBeginOffset; + const float srend = globals::features::skySync.settings.SunriseEndOffset; + const float ssbegin = globals::features::skySync.settings.SunsetBeginOffset; + const float ssend = globals::features::skySync.settings.SunsetEndOffset; sunriseBegin = (climate->timing.sunrise.begin / 6.0f) + srbegin; sunriseEnd = (climate->timing.sunrise.end / 6.0f) + srend; sunsetBegin = (climate->timing.sunset.begin / 6.0f) + ssbegin; sunsetEnd = (climate->timing.sunset.end / 6.0f) + ssend; + + // Basic ordering guarantees (prevents divide-by-zero / negative duration paths). + constexpr float kMinGapHours = 0.01f; + if (sunriseEnd <= sunriseBegin) sunriseEnd = sunriseBegin + kMinGapHours; + if (sunsetEnd <= sunsetBegin) sunsetEnd = sunsetBegin + kMinGapHours; + if (sunsetBegin <= sunriseEnd) sunsetBegin = sunriseEnd + kMinGapHours; + if (sunsetEnd <= sunsetBegin) sunsetEnd = sunsetBegin + kMinGapHours; sunrise = (sunriseBegin + sunriseEnd) * 0.5f - 0.25f; sunset = (sunsetBegin + sunsetEnd) * 0.5f + 0.25f;
🤖 Fix all issues with AI agents
In @src/Features/SkySync.cpp:
- Around line 33-45: The Tree opened by ImGui::TreeNodeEx("Sun Position
Offsets", ...) is not closed, causing ImGui stack corruption; after the block
that adds the four sliders (SunriseBeginOffset, SunriseEndOffset,
SunsetBeginOffset, SunsetEndOffset) add a matching ImGui::TreePop() call when
TreeNodeEx returned true. While there, update the ImGui::SliderFloat calls for
those four settings to include ImGuiSliderFlags_AlwaysClamp to ensure consistent
clamping behavior with the integer sliders.
🧹 Nitpick comments (2)
src/Features/SkySync.cpp (2)
418-435: Defensive clamp at use-site forMinShadowElevation(protects against config edits / future callers).Even though UI/load clamp it today,
ClampDirection()is a hot, safety-critical leaf; it’s worth ensuringMinShadowElevationcan’t go negative/NaN if settings are mutated elsewhere later.Proposed tweak
inline void SkySync::ShadowFader::ClampDirection(RE::NiPoint3& dir) { - float minDegrees = globals::features::skySync.settings.MinShadowElevation; - - float minElev = DirectX::XMConvertToRadians(minDegrees); + const float minDegrees = std::clamp(globals::features::skySync.settings.MinShadowElevation, 0.0f, 45.0f); + const float minElev = DirectX::XMConvertToRadians(minDegrees); const float elev = DirectX::XMScalarASinEst(dir.z); if (elev >= minElev) return;
1-1: PR metadata (optional): tighten title + add issue linkage if applicable.
- Conventional Commit title suggestion (≤50 chars):
feat(sky-sync): add sun offsets and min shadow elev- If there’s a tracking issue, consider adding
Implements #NNNN/Addresses #NNNNin the PR description.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Features/SkySync.cpp
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Features/SkySync.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/SkySync.cpp
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
src/Features/SkySync.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
🔇 Additional comments (1)
src/Features/SkySync.cpp (1)
3-14: Serialization: good addition; consider config/backward-compat note in PR description.
NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULTis a good fit for adding new fields without breaking older configs. Given the PR description says “two new UI sliders”, consider clarifying it’s 5 settings total (4 offsets + 1 min elevation).
ImGui TreePop placed in correct spot, Added ImGuiSliderFlags_AlwaysClamp to new sliders, Ensured MinShadowElevation can't be negative, Clamped settings.CustomAngle, Validate sunrise/sunset ordering after applying offsets to prevent sunrise/sunset lengths from being negative.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/Features/SkySync.cpp:
- Line 33: The MinShadowElevation slider (ImGui::SliderFloat with
settings.MinShadowElevation) lacks the ImGuiSliderFlags_AlwaysClamp flag,
allowing keyboard input to exceed the [0.0f, 45.0f] bounds; update the
ImGui::SliderFloat call for "Min Shadow Elevation" to pass
ImGuiSliderFlags_AlwaysClamp as the flags argument (after the format string) so
the value is clamped consistently like the other sliders.
🧹 Nitpick comments (1)
src/Features/SkySync.cpp (1)
422-423: Consider removing redundant clamping.The
std::clamphere is defensive but redundant sinceLoadSettings()already clampsMinShadowElevationto[0.0, 45.0]. SinceClampDirectionis called per-frame in the rendering path, you could remove this extra clamp for a minor efficiency gain.Proposed fix
- const float minDegrees = std::clamp(globals::features::skySync.settings.MinShadowElevation, 0.0f, 45.0f); + const float minDegrees = globals::features::skySync.settings.MinShadowElevation; const float minElev = DirectX::XMConvertToRadians(minDegrees);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Features/SkySync.cpp
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Features/SkySync.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/SkySync.cpp
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
src/Features/SkySync.cpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
📚 Learning: 2025-08-05T18:13:03.123Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:260-260
Timestamp: 2025-08-05T18:13:03.123Z
Learning: In the skyrim-community-shaders SnowCover feature, the time calculation uses division by 61.0 instead of 60.0 for seconds conversion in the perFrame.Month calculation. The original author ThePagi indicated this was intentional and makes no discernible difference to the snow cover functionality, suggesting it may be related to game-specific timing mechanics or balance considerations.
Applied to files:
src/Features/SkySync.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (3)
src/Features/SkySync.cpp (3)
3-14: LGTM!The JSON serialization macro correctly includes all new settings fields. The
WITH_DEFAULTvariant ensures backward compatibility with existing config files that lack these new fields.
54-59: LGTM!The clamping logic correctly validates all new settings after deserialization. The ranges match the UI slider bounds, ensuring consistency between loaded settings and user-adjustable values.
446-466: LGTM!The ordering guarantees correctly handle cascading edge cases. For example, if user offsets cause
sunsetBegin <= sunriseEnd, fixing that on line 464 may pushsunsetBeginpastsunsetEnd, requiring the second check on lines 465-466. The minimum gap of 0.1 hours (6 minutes) prevents divide-by-zero in downstream calculations.Minor style note: the local variables on lines 448-451 use PascalCase (
SunriseBeginOffset) which typically denotes class members or types in C++. Consider using camelCase for locals, but this is purely cosmetic.
clamped minshadowelevation as well Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Removed defensive clamp for minor efficiency
accidental line duplicated (i am fumbling my way through this oh my god)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Features/SkySync.cpp (2)
420-437: Fix compile error: duplicateminElevdeclaration; also clampasininput to avoid NaNs.Proposed fix (removes duplicate decl + hardens math)
inline void SkySync::ShadowFader::ClampDirection(RE::NiPoint3& dir) { - const float minDegrees = globals::features::skySync.settings.MinShadowElevation; - const float minElev = DirectX::XMConvertToRadians(minDegrees); - const float minElev = DirectX::XMConvertToRadians(minDegrees); - const float elev = DirectX::XMScalarASinEst(dir.z); + const float minDegrees = + std::clamp(globals::features::skySync.settings.MinShadowElevation, 0.0f, 89.9f); + const float minElev = DirectX::XMConvertToRadians(minDegrees); + + // Avoid NaNs from minor numeric drift (dir is expected normalized, but be defensive). + const float z = std::clamp(dir.z, -1.0f, 1.0f); + const float elev = DirectX::XMScalarASinEst(z); if (elev >= minElev) return; const float heading = std::atan2(dir.y, dir.x); float sinElev, cosElev, sinHeading, cosHeading; DirectX::XMScalarSinCosEst(&sinElev, &cosElev, minElev); DirectX::XMScalarSinCosEst(&sinHeading, &cosHeading, heading); dir.x = cosElev * cosHeading; dir.y = cosElev * sinHeading; dir.z = sinElev; }
447-474: Offsets can push sunrise/sunset past midnight; current logic isn’t wrap-aware.With offsets up to ±5 hours, it’s easy to end up with e.g.
sunsetEnd > 24. Sincetimeis in [0, 24),isDayTime/fade windows and alternate sun-path math won’t behave intuitively once the “day” crosses midnight. Consider either:
- clamping computed timings to [0, 24] (disallow crossing midnight), or
- implementing wrap-aware comparisons (treat timings as a circular 24h domain).
At minimum, I’d rename the locals to avoid UpperCamel for non-constants and clamp computed hours into a safe domain before enforcing ordering.
🧹 Nitpick comments (2)
src/Features/SkySync.cpp (2)
33-46: UI: consider making “requires weather refresh” actionable and reduce spacer noise.
- The “Reset weather to see changes” note is good, but consider adding a small inline hint like “(via weather change / reset)” or a one-click “Recalculate timings” button if you can safely trigger
timings.Update(sky->currentClimate)from here.- Two consecutive
ImGui::Spacing()calls read a bit accidental—one is probably enough.
54-60: Clamps look good; consider clamping again at point-of-use for safety.
LoadSettings()clamps cover JSON loads, and ImGui sliders clamp interactive changes; for extra robustness (e.g., external mutation), it’s still worth clampingMinShadowElevationinsideClampDirection()as well.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Features/SkySync.cpp
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Features/SkySync.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/SkySync.cpp
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
src/Features/SkySync.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
📚 Learning: 2025-06-24T07:17:36.604Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Applied to files:
src/Features/SkySync.cpp
📚 Learning: 2025-08-05T18:13:03.123Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:260-260
Timestamp: 2025-08-05T18:13:03.123Z
Learning: In the skyrim-community-shaders SnowCover feature, the time calculation uses division by 61.0 instead of 60.0 for seconds conversion in the perFrame.Month calculation. The original author ThePagi indicated this was intentional and makes no discernible difference to the snow cover functionality, suggesting it may be related to game-specific timing mechanics or balance considerations.
Applied to files:
src/Features/SkySync.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (2)
src/Features/SkySync.cpp (2)
1-1: Conventional commit message suggestion.The commit message “Update SkySync.cpp” isn’t Conventional Commits; consider something like:
feat(sky-sync): add sun timing offsets and min shadow elevation
3-14: VerifyMinShadowElevationdefault preserves prior behavior.The
NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULTmacro correctly lists all new fields. The offset fields (SunriseBeginOffset,SunriseEndOffset,SunsetBeginOffset,SunsetEndOffset) default to0.0f, which maintains backward compatibility by applying no offset. However, confirm thatMinShadowElevation = 0.25fmatches the previous hardcoded minimum elevation value, as this will silently affect shadow behavior for existing users if it differs.
3 participants
Adds new sliders that control sun position by adding climatetimings offsets, and can change the MinElevation value to limit shadow depth. Changing sun position requires a weather refresh, MinElevation works in realtime.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.