fix(sky sync): early return for invalid cells

[SkrubbySkrubInAShrub]

Adds an early return for when the player is in a cell that is not set up to make use of sun shadows. This prevents sky sync from hijacking the directional light from the lighting templates as if it was the sun and moving it around, causing weird lighting behaviour. as shown in this video from isoprovophlex

2026-03-17_10-29-23.shortened.mp4.mov

And here is also an example from leostevano:
With Skysync

Without Skysync

Summary by CodeRabbit

• Bug Fixes

  • More consistent sun/moon lighting and sky rotation when moving between areas; shadows now update only on meaningful area changes.
  • Interiors without sunlight-shadow support no longer show incorrect lighting shifts.

• Performance

  • Sky updates skip unnecessary processing in many interior scenarios, reducing redundant per-frame work.

• Refactor

  • Improved area-transition tracking to avoid spurious lighting and shadow resets.

[github-actions]

No actionable suggestions for changed features.

[github-actions]

✅ A pre-release build is available for this PR:
Download

[alandtse]

@coderabbitai review please

[coderabbitai]

@alandtse

Comment images:

• The first Comment image shows a wooden interior (long tables, candles, barrels on the right). The far-right wall and barrels are in very deep shadow with a strong directional cast across the benches; the overall scene looks unnaturally contrasty for an interior.
• The second Comment image shows the same scene with noticeably more neutral wall lighting on the right and less aggressive directional darkening, consistent with avoiding a moving “sun” light in a cell not set up for it.

Based on the PR diff:

• In src/Features/SkySync.cpp, SkySync::Update now retrieves the player’s parent cell once, preserves the cell-change handling, and adds an early return so interiors without the Sunlight Shadows flag skip Sky Sync. Comment documents that exteriors always run; interiors require the flag. (github.com)
• In src/Features/SkySync.h, a private enum CellFlagExt is added with kSunlightShadows = 1 << 15, used via cellFlags.all(static_castRE::TESObjectCELL::Flag(…)). CommonLibSSE-NG doesn’t name this bit, so extending with the “unknown 15” flag here is reasonable. (github.com)

Review notes

• Correctness: The guard precisely targets the misbehavior described (hijacking template lights in interiors without Sunlight Shadows). Control flow is safe: SetSkyRotation/shadowFader reset still occur on cell transitions before the early return.
• Safety: The check is null-safe and only affects interiors; exteriors are unaffected as intended.
• Maintainability (optional nits, no blocker):
  • Add a brief comment by CellFlagExt explaining “unknown 15” corresponds to the Creation Kit’s “Sunlight Shadows” checkbox for interiors.
  • Consider a small helper for readability, e.g.:
    bool HasSunlightShadows(const RE::TESObjectCELL* c) {
    return c && c->cellFlags.all(static_castRE::TESObjectCELL::Flag(CellFlagExt::kSunlightShadows));
    }
    and then use it in Update().

Overall: Change is minimal, well-scoped, and aligns with the evidence shown in the screenshots. LGTM pending any project-style preferences on naming/comments.

---

✏️ Learnings added

Learnt from: alandtse
URL: https://github.com/doodlum/skyrim-community-shaders/pull/1991

Timestamp: 2026-03-17T15:52:57.331Z
Learning: In doodlum/skyrim-community-shaders, SkySync::Update skips running in interior cells unless the TESObjectCELL “Sunlight Shadows” flag is set. The flag is represented as bit 1<<15 via CellFlagExt::kSunlightShadows in src/Features/SkySync.h and checked with cell->cellFlags.all(static_cast<RE::TESObjectCELL::Flag>(CellFlagExt::kSunlightShadows)).

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

---

🧠 Learnings used

Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 1947
File: package/Shaders/DeferredCompositeCS.hlsl:234-243
Timestamp: 2026-03-14T08:35:37.608Z
Learning: In `package/Shaders/DeferredCompositeCS.hlsl` (doodlum/skyrim-community-shaders), the fallback non-IBL SKYLIGHTING specular path intentionally double-weights with `skylightingSpecular`: `dalcScaled` is computed as `IrradianceToGamma(IrradianceToLinear(directionalAmbientColorSpecular) * skylightingSpecular)`, and the final `finalIrradiance` is then `lerp(specularIrradiance, specularIrradianceReflections, skylightingSpecular)`. This is original engine logic preserved from before PR `#1947` and should not be flagged as a double-attenuation bug.

---

Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 1947
File: features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli:107-115
Timestamp: 2026-03-14T08:34:58.304Z
Learning: In `features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli`, the pattern of scaling `directionalAmbientColorSpecular` by `skylightingSpecular` (via `dalcScaled`) before the final `lerp(specularIrradiance, specularIrradianceReflections, skylightingSpecular)` is intentional pre-existing behavior preserved from the original implementation. Do not flag this as double-attenuation; the combined attenuation is by design.

---

Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 1966
File: package/Shaders/ISVolumetricLightingGenerateCS.hlsl:0-0
Timestamp: 2026-03-11T08:05:10.801Z
Learning: In doodlum/skyrim-community-shaders, the `sqrt()` applied to `CloudShadows::GetCloudShadowMult()` in `package/Shaders/ISVolumetricLightingGenerateCS.hlsl` is intentional. The maintainer (Dlizzio) confirmed it is used to increase cloud shadow strength at harsh angles for volumetric lighting, and should not be removed or replaced with a direct linear multiply.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Reworks Sky_Update to compute current cell before branching, adds a private CellFlagExt::kSunlightShadows flag, early-returns for interior cells lacking that flag (setting volumetricLightingIntensityFactor = 1.0f), and updates sky rotation / shadow-fader resets only when the player’s cell actually changes.

Changes

Cohort / File(s)	Summary
Header: enum addition src/Features/SkySync.h	Adds private nested enum CellFlagExt : uint16_t with kSunlightShadows = 1 << 15 to mark interior cells that support sunlight/shadows.
Implementation: control-flow refactor src/Features/SkySync.cpp	Moves current-cell retrieval outside inline guard, introduces per-frame interior guard that early-returns for interiors without kSunlightShadows (while setting volumetricLightingIntensityFactor = 1.0f), and applies SetSkyRotation / shadowFader.Reset only when prevCell → current-cell transitions (including interior/exterior or worldspace changes) require it.

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• fix(skysync): remove sunlight fade and volumetric lighting overrides #1966: Overlaps in volumetric lighting handling and use of volumetricLightingIntensityFactor.
• fix(skysync): effect mesh blackout during shadow caster transitions #1965: Related adjustments to shadow-fader and volumetric lighting intensity behavior.
• fix: using unused cell flag to toggle ISS #1082: Adds/uses the same kSunlightShadows flag to gate interior sunlight/shadow logic.

Suggested reviewers

• alandtse
• doodlum

Poem

🐰 I hopped where cells and shadows play,
A tiny flag to guide the day,
If rooms won't cast the sun's soft kiss,
I skip the spin and save the bliss,
Tiny paws, a brighter way.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(sky sync): early return for invalid cells' clearly summarizes the main change: adding an early return guard in SkySync for cells without proper sun shadow configuration.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

📝 Coding Plan

• Generate coding plan for human review comments

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/Features/SkySync.h (1)

86-90: Document the bit-to-flag mapping inline.

kSunlightShadows = 1 << 15 is fine, but a short inline note that this is the TESObjectCELL “Sunlight Shadows” CK flag would reduce future ambiguity.

Based on learnings: In doodlum/skyrim-community-shaders, the TESObjectCELL “Sunlight Shadows” flag is represented as bit 1<<15 via CellFlagExt::kSunlightShadows.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Features/SkySync.h` around lines 86 - 90, Add an inline comment next to
the CellFlagExt::kSunlightShadows enum entry documenting that this value maps to
the TESObjectCELL "Sunlight Shadows" Creation Kit flag and corresponds to bit
1<<15 (as used in doodlum/skyrim-community-shaders); update the comment near the
enum definition (CellFlagExt::kSunlightShadows) so future readers see the CK
flag name and bit mapping immediately.

src/Features/SkySync.cpp (1)

119-119: Optional PR metadata improvement: add issue-closing keyword.

If this fixes a tracked bug, add Fixes #<issue-id> in the PR description for auto-linking and closure.

As per coding guidelines: Suggest adding GitHub keywords like “Fixes #123” for bug fixes.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Features/SkySync.cpp` at line 119, Add a GitHub issue-closing keyword to
the PR/commit that contains the change to SkySync::Update so the issue is
auto-linked and closed (e.g., add "Fixes #<issue-id>" to the PR description or
commit message that introduces the SkySync::Update change); ensure the
referenced issue ID is correct and that the keyword appears in the PR
description or the commit that includes the SkySync::Update modification.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/Features/SkySync.cpp`:
- Around line 130-135: The transition check compares `cell` to `currentCell`
after `SetSkyRotation` (which updates `currentCell`), so capture the previous
cell before calling `SetSkyRotation` and use that saved `prevCell` for the
interior/worldspace comparison; i.e., store `auto prevCell = currentCell;`
before `SetSkyRotation(sky, cell)` and then call `shadowFader.Reset()` when
`prevCell` exists and `cell->IsInteriorCell()` or `GetRuntimeData().worldSpace`
differ from `prevCell` (use `prevCell` instead of `currentCell` in the
condition).

---

Nitpick comments:
In `@src/Features/SkySync.cpp`:
- Line 119: Add a GitHub issue-closing keyword to the PR/commit that contains
the change to SkySync::Update so the issue is auto-linked and closed (e.g., add
"Fixes #<issue-id>" to the PR description or commit message that introduces the
SkySync::Update change); ensure the referenced issue ID is correct and that the
keyword appears in the PR description or the commit that includes the
SkySync::Update modification.

In `@src/Features/SkySync.h`:
- Around line 86-90: Add an inline comment next to the
CellFlagExt::kSunlightShadows enum entry documenting that this value maps to the
TESObjectCELL "Sunlight Shadows" Creation Kit flag and corresponds to bit 1<<15
(as used in doodlum/skyrim-community-shaders); update the comment near the enum
definition (CellFlagExt::kSunlightShadows) so future readers see the CK flag
name and bit mapping immediately.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 105275b6-63c1-4884-908e-909ca0ddb201

📥 Commits

Reviewing files that changed from the base of the PR and between 491e83e and 555f5c5.

📒 Files selected for processing (2)

• src/Features/SkySync.cpp
• src/Features/SkySync.h

[coderabbitai]

♻️ Duplicate comments (1)

src/Features/SkySync.cpp (1)

132-136: ⚠️ Potential issue | 🔴 Critical

Guard cell before dereferencing in the transition path.

Line 134 and Line 135 use cell without a null check. If GetParentCell() is null during transitions/load, this can crash.

🐛 Proposed fix

 if (cell != currentCell) {
 	const auto prevCell = currentCell;
-	SetSkyRotation(sky, cell);
-	if (prevCell && (cell->IsInteriorCell() != prevCell->IsInteriorCell() || cell->GetRuntimeData().worldSpace != prevCell->GetRuntimeData().worldSpace))
-		shadowFader.Reset();
+	if (cell) {
+		SetSkyRotation(sky, cell);
+		if (prevCell && (cell->IsInteriorCell() != prevCell->IsInteriorCell() || cell->GetRuntimeData().worldSpace != prevCell->GetRuntimeData().worldSpace))
+			shadowFader.Reset();
+	} else {
+		currentCell = nullptr;
+		if (prevCell)
+			shadowFader.Reset();
+	}
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Features/SkySync.cpp` around lines 132 - 136, The code dereferences cell
without a null check during transitions: before calling SetSkyRotation(sky,
cell) and using cell->IsInteriorCell()/cell->GetRuntimeData(), ensure cell is
non-null; modify the branch around currentCell change (variables cell,
currentCell, prevCell) to check if (cell) before calling SetSkyRotation and
before comparing cell->... to prevCell->..., or adjust the logic to handle a
null cell (e.g., skip SetSkyRotation and skip the interior/worldspace comparison
when cell is null) so that SetSkyRotation, the IsInteriorCell check, and
GetRuntimeData access are only performed when cell is valid.

🧹 Nitpick comments (1)

src/Features/SkySync.cpp (1)

119-142: Consider tightening PR metadata for traceability.

Suggested title: fix(skysync): skip interiors without sunlight shadows
If there is a tracking issue, add Closes #<id> (or Fixes #<id>) in the PR description.

As per coding guidelines: "When reviewing PRs, please provide suggestions for Conventional Commit Titles ... [and] Issue References ..."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Features/SkySync.cpp` around lines 119 - 142, Update the PR metadata to
follow Conventional Commits: change the title to "fix(skysync): skip interiors
without sunlight shadows" and, if there is a tracking issue, add "Closes #<id>"
(or "Fixes #<id>") to the PR description; reference the change in
SkySync::Update (src/Features/SkySync.cpp) where the interior-cell
sunlight-shadows check and shadowFader.Reset are handled so reviewers can trace
the commit to the relevant code.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/Features/SkySync.cpp`:
- Around line 132-136: The code dereferences cell without a null check during
transitions: before calling SetSkyRotation(sky, cell) and using
cell->IsInteriorCell()/cell->GetRuntimeData(), ensure cell is non-null; modify
the branch around currentCell change (variables cell, currentCell, prevCell) to
check if (cell) before calling SetSkyRotation and before comparing cell->... to
prevCell->..., or adjust the logic to handle a null cell (e.g., skip
SetSkyRotation and skip the interior/worldspace comparison when cell is null) so
that SetSkyRotation, the IsInteriorCell check, and GetRuntimeData access are
only performed when cell is valid.

---

Nitpick comments:
In `@src/Features/SkySync.cpp`:
- Around line 119-142: Update the PR metadata to follow Conventional Commits:
change the title to "fix(skysync): skip interiors without sunlight shadows" and,
if there is a tracking issue, add "Closes #<id>" (or "Fixes #<id>") to the PR
description; reference the change in SkySync::Update (src/Features/SkySync.cpp)
where the interior-cell sunlight-shadows check and shadowFader.Reset are handled
so reviewers can trace the commit to the relevant code.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 775b9720-f700-4555-9406-dbae559541a6

📥 Commits

Reviewing files that changed from the base of the PR and between 555f5c5 and e103ed3.

📒 Files selected for processing (1)

• src/Features/SkySync.cpp

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/Features/SkySync.cpp (1)

119-143: Add an issue-closing reference in the PR description.

Since this is a bug fix, please add Fixes #<id> / Closes #<id> if there is a tracking issue.

As per coding guidelines **/*: “Suggest adding appropriate GitHub keywords: ‘Fixes #123’ or ‘Closes #123’ for bug fixes.”

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Features/SkySync.cpp` around lines 119 - 143, The PR is missing a GitHub
issue-closing keyword for this bug fix; update the PR description to include a
line like "Fixes #<id>" or "Closes #<id>" referencing the tracking issue for
this change that touches SkySync::Update (and related behavior such as
SetSkyRotation and shadowFader.Reset) so merging will automatically close the
issue.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/Features/SkySync.cpp`:
- Around line 140-142: The early return in the interior-cell check
(cell->IsInteriorCell() &&
!cell->cellFlags.all(static_cast<RE::TESObjectCELL::Flag>(CellFlagExt::kSunlightShadows)))
can skip updating shadowFader, leaving volumetricLightingIntensityFactor stale
and still applied in
ApplyVolumetricLighting_VolumetricLightingDescriptor_Get::thunk; to fix, ensure
you call the same shadow fader update/reset before returning (e.g., invoke
shadowFader.Update(...) with the current player position and deltaTime or
explicitly reset volumetricLightingIntensityFactor/shadowFader state to its
neutral value) so no carry-over intensity persists when skipping interior cells.

---

Nitpick comments:
In `@src/Features/SkySync.cpp`:
- Around line 119-143: The PR is missing a GitHub issue-closing keyword for this
bug fix; update the PR description to include a line like "Fixes #<id>" or
"Closes #<id>" referencing the tracking issue for this change that touches
SkySync::Update (and related behavior such as SetSkyRotation and
shadowFader.Reset) so merging will automatically close the issue.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 04ceabe9-8ed1-4884-9078-8ee71bdb4f01

📥 Commits

Reviewing files that changed from the base of the PR and between e103ed3 and e57c1f8.

📒 Files selected for processing (1)

• src/Features/SkySync.cpp

[coderabbitai]

🧹 Nitpick comments (1)

src/Features/SkySync.cpp (1)

139-140: Optional readability cleanup: extract the sunlight-shadow eligibility check.

Consider a small helper (e.g., HasSunlightShadows(cell)) to centralize the flag cast/check and keep Update(...) easier to scan.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Features/SkySync.cpp` around lines 139 - 140, Extract the sunlight-shadow
eligibility check into a helper to improve readability: create a function named
e.g. HasSunlightShadows(RE::TESObjectCELL* cell) that performs the current
condition (handles null check, calls cell->IsInteriorCell() if needed, and
checks
cell->cellFlags.all(static_cast<RE::TESObjectCELL::Flag>(CellFlagExt::kSunlightShadows))).
Then replace the inline expression in Update(...) with a call to
HasSunlightShadows(cell) so the Update method is easier to scan and the flag
cast/logic is centralized.

🤖 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/Features/SkySync.cpp`:
- Around line 139-140: Extract the sunlight-shadow eligibility check into a
helper to improve readability: create a function named e.g.
HasSunlightShadows(RE::TESObjectCELL* cell) that performs the current condition
(handles null check, calls cell->IsInteriorCell() if needed, and checks
cell->cellFlags.all(static_cast<RE::TESObjectCELL::Flag>(CellFlagExt::kSunlightShadows))).
Then replace the inline expression in Update(...) with a call to
HasSunlightShadows(cell) so the Update method is easier to scan and the flag
cast/logic is centralized.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 494d265b-2ae7-426a-83b9-c73c254a18ef

📥 Commits

Reviewing files that changed from the base of the PR and between 825a9f5 and fcf7f10.

📒 Files selected for processing (2)

• src/Features/SkySync.cpp
• src/Features/SkySync.h

🚧 Files skipped from review as they are similar to previous changes (1)

• src/Features/SkySync.h