fix: clear room nodes on update

[onymic]

This should address #1039. I believe the intent was always to clear this before updating the lights. Tested locally, the lights were visible throughout 9 hours of playtime.

Summary by CodeRabbit

• Bug Fixes
  • Resolved an issue where outdated lighting data could persist between frames, ensuring more accurate and consistent lighting updates.

[coderabbitai]

Walkthrough

The update modifies the LightLimitFix::UpdateLights() function by replacing a call to roomNodes.empty(); with roomNodes.clear();. This change ensures that the roomNodes container is cleared at the start of each light update cycle, preventing the retention of outdated data.

Changes

File(s)	Change Summary
src/Features/LightLimitFix.cpp	Replaced roomNodes.empty(); with roomNodes.clear(); in UpdateLights() to clear the container before processing.

Suggested reviewers

• alandtse

Poem

A bunny hops where shadows play,
Clearing rooms for lights today.
No more clutter, no more gloom,
Fresh and bright in every room!
With every frame, the past erased—
The code is clean, the lights are placed.
🌟🐇✨

---

📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6479240 and af3d84e.

📒 Files selected for processing (1)

• src/Features/LightLimitFix.cpp (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms (1)

• GitHub Check: Build plugin and addons

🔇 Additional comments (1)

src/Features/LightLimitFix.cpp (1)

755-755: Excellent bug fix!

This change correctly replaces a no-op empty() call with the intended clear() operation. The original code was just checking if roomNodes was empty and discarding the result, while the fix properly clears stale room node data before rebuilding it for the current frame. This prevents accumulation of outdated data between light update cycles.

✨ Finishing Touches

• 📝 Generate Docstrings

---

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

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Explain this complex logic.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai explain this code block.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and explain its main purpose.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[github-actions]

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

[alandtse]

@coderabbitai are there any other places where an empty() may be misused instead of clear()?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@alandtse Great question! Let me search the codebase for other potential misuses of empty() where clear() might have been intended.

---

🏁 Script executed:

#!/bin/bash
# Search for standalone .empty() calls that might be misused instead of .clear()
# Look for lines ending with .empty(); where the result is not used
rg '\.empty\(\)\s*;' -A 2 -B 2

echo "=== Searching for .empty() in assignments or conditionals (these are likely correct) ==="
# Also search for .empty() used correctly in conditionals/assignments
rg '\.empty\(\)' --type cpp -A 1 -B 1 | grep -E '(if|while|return|=|&&|\|\|)'

echo "=== Searching for all .empty() usage to get full context ==="
# Get all .empty() usage for comprehensive analysis
rg '\.empty\(\)' --type cpp -n

Length of output: 23710

---

Perfect! Based on my comprehensive search of the codebase, I found no other instances where empty() is misused instead of clear().

Here's what the analysis revealed:

✅ No standalone .empty(); calls found - The search for the problematic pattern (\.empty\(\)\s*;) returned zero results, meaning the bug you fixed was the only instance.

✅ All other .empty() usage is correct - Every other occurrence of .empty() in the codebase is properly used in contexts like:

• Conditionals: if (!container.empty())
• Return statements: return !container.empty()
• Boolean assignments: bool hasData = !container.empty()
• Ternary operators: container.empty() ? nullptr : container[0]
• Logical expressions: && !container.empty()

The original bug (roomNodes.empty();) was a standalone statement that just checked if the container was empty and discarded the result - essentially a no-op. Your fix to roomNodes.clear(); was spot-on and appears to be the only instance of this particular bug pattern in the codebase.

This suggests it was likely a typo or copy-paste error that happened during development, rather than a systematic issue throughout the codebase.

  ∩―――――∩
 (  ◕   ◕ )
o/     ∪     \o  Great detective work finding that bug!
 |  \_____/  |
  \__) | (__/

[alandtse]

@coderabbitai are there any other instances where a non void function return is being discarded?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@alandtse Excellent question! Let me search for other instances where non-void function returns might be discarded, which could indicate similar bugs.

---

🏁 Script executed:

#!/bin/bash
echo "=== Searching for potentially discarded return values ==="

# Look for common functions that return values but might have them discarded
echo "--- Checking for discarded size(), length(), count() calls ---"
rg '\.(size|length|count)\(\)\s*;' --type cpp -n -A 1 -B 1

echo "--- Checking for discarded find(), search() calls ---"
rg '\.(find|search)\([^)]*\)\s*;' --type cpp -n -A 1 -B 1

echo "--- Checking for discarded comparison/check functions ---"
rg '\.(compare|equals?|contains?|has|is[A-Z])\([^)]*\)\s*;' --type cpp -n -A 1 -B 1

echo "--- Checking for discarded error/status return functions ---"
rg '\.(get|create|init|setup|configure)\([^)]*\)\s*;' --type cpp -n -A 1 -B 1

echo "--- Checking for discarded standard library functions that return values ---"
rg '(std::|::)(insert|emplace|push|pop|erase|remove)\([^)]*\)\s*;' --type cpp -n -A 1 -B 1

echo "--- Looking for suspicious standalone function calls (not assignments/conditions) ---"
rg '^[[:space:]]*[a-zA-Z_][a-zA-Z0-9_]*\([^)]*\)\s*;' --type cpp -n | head -20

echo "--- Checking for discarded HRESULT/error code patterns ---"
rg '(HRESULT|DWORD|int|bool)[[:space:]]+\w+\([^)]*\)\s*;' --type cpp -n -A 1 -B 1

echo "--- Looking for method calls that typically return important values ---"
rg '\.(reserve|resize|assign|replace|substr|at)\([^)]*\)\s*;' --type cpp -n -A 1 -B 1

Length of output: 47564

---

Great analysis! I found several instances where std::filesystem::remove() return values are being discarded, which could indicate potential error handling issues. Let me search for more specific patterns:

---

🏁 Script executed:

#!/bin/bash
echo "=== Focusing on potentially problematic discarded returns ==="

echo "--- std::filesystem::remove() calls (returns bool for success/failure) ---"
rg 'std::filesystem::remove\(' --type cpp -n -A 2 -B 2

echo "--- Other filesystem operations that return bool/error codes ---"
rg 'std::filesystem::(create|copy|rename|exists)\([^)]*\)\s*;' --type cpp -n -A 1 -B 1

echo "--- CreateDirectory, DeleteFile, and other Win32 API calls ---"
rg '(CreateDirectory|DeleteFile|CopyFile|MoveFile)\([^)]*\)\s*;' --type cpp -n -A 1 -B 1

echo "--- HRESULT-returning function calls being discarded ---"
rg '^[[:space:]]*[A-Za-z_][A-Za-z0-9_]*\([^)]*\)\s*;' --type cpp -n | rg -i '(create|get|load|init|setup)' | head -10

echo "--- Device/Context method calls that might return HRESULT ---"
rg '(device|context)->[A-Za-z]+\([^)]*\)\s*;' --type cpp -n -A 1 -B 1 | head -15

echo "--- DirectX/D3D function calls that typically return HRESULT ---"
rg '(D3D|DirectX|ID3D)[A-Za-z0-9]*::[A-Za-z]+\([^)]*\)\s*;' --type cpp -n -A 1 -B 1 | head -10

Length of output: 6140

---

Based on my comprehensive analysis, I found several instances where std::filesystem::remove() return values are being discarded, which could indicate potential error handling issues:

🚨 Potentially Problematic Cases:

1. ShaderCache.cpp:1926

if (std::filesystem::exists(filePath)) {
    std::filesystem::remove(filePath);  // ⚠️ Bool return discarded
    logger::debug("Deleted {}", filePathString);
}

Issue: Logs "Deleted" regardless of whether removal actually succeeded.

2. FeatureIssues.cpp:1363

if (std::filesystem::exists(testInfo.testIniPath)) {
    std::filesystem::remove(testInfo.testIniPath);  // ⚠️ Bool return discarded  
    logger::debug("Removed test INI: {}", testInfo.testIniPath);
}

Issue: Similar problem - logs "Removed" without verifying success.

3. FeatureIssues.cpp:1401

if (std::filesystem::exists(stateFilePath)) {
    std::filesystem::remove(stateFilePath);  // ⚠️ Bool return discarded
}

Issue: In a try-catch block, but doesn't distinguish between exceptions and failed removal.

✅ Good News:

• No other significant instances of discarded return values that indicate bugs
• Most other .get(), context-> calls, and function calls are either:
  • Used correctly in assignments/conditions
  • Called for side effects (like D3D context methods)
  • Return void (like DirectX::XMScalarSinCosEst)

Recommendation:

Consider checking the return values of std::filesystem::remove() calls to ensure proper error handling, especially where logging suggests the operation should succeed.

  ∩―――――∩
 (  ◕   ◕ )
o/     ∪     \o  Found some filesystem operations that could use better error checking!
 |  \_____/  |
  \__) | (__/

[alandtse]

@coderabbitai please open an issue to document these for remediation.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@alandtse I'll create a GitHub issue to track these std::filesystem::remove() return value issues for future remediation.

---

🏁 Script executed:

#!/bin/bash
# Create a GitHub issue to track discarded std::filesystem::remove() return values

gh issue create \
  --title "Improve error handling: Check return values of std::filesystem::remove() calls" \
  --body "## Description

Several instances in the codebase discard the return value of \`std::filesystem::remove()\`, which returns a boolean indicating success/failure. This can lead to incorrect logging or missed error conditions.

## Affected Files and Lines

1. **ShaderCache.cpp:1926** - Logs \"Deleted\" regardless of actual removal success
2. **FeatureIssues.cpp:1363** - Logs \"Removed test INI\" without verifying success  
3. **FeatureIssues.cpp:1401** - In try-catch block but doesn't distinguish between exceptions and failed removal

## Impact

- Misleading log messages that indicate success when operation may have failed
- Potential silent failures in file cleanup operations
- Inconsistent error handling patterns

## Background

This issue was identified during code review analysis in PR #1177 which fixed a similar issue with \`.empty()\` vs \`.clear()\`.

## References

- Original discussion: https://github.com/doodlum/skyrim-community-shaders/pull/1177#issuecomment-$(gh api repos/doodlum/skyrim-community-shaders/pulls/1177/comments --jq '.[-1].id')
- Related PR: https://github.com/doodlum/skyrim-community-shaders/pull/1177

Reported by: @alandtse" \
  --assignee alandtse

Length of output: 1359

---

Perfect! ✅ I've successfully created GitHub issue #1180 to track the std::filesystem::remove() return value issues:

🔗 Issue Link: #1180

The issue documents all three instances where return values are being discarded:

• ShaderCache.cpp:1926
• FeatureIssues.cpp:1363
• FeatureIssues.cpp:1401

It's been assigned to you (@alandtse) and includes references back to this PR discussion for context. This will help ensure these error handling improvements don't get lost and can be addressed in a future PR.

  ∩―――――∩
 (  ◕   ◕ )
o/     ∪     \o  Issue #1180 created and ready for future remediation!
 |  \_____/  |
  \__) | (__/