docs: add doxygen coverage and remove AI slop comments#2508
Conversation
- Add doxygen documentation to public methods across 118/119 header files (99.2% coverage, up from ~12%) - Remove ~200 AI-generated slop comments that restated what code does - Standardize comment style: convert all /// doxygen to /** */ blocks - Preserve comments that explain WHY, hidden constraints, workarounds, and non-obvious behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (3)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughDocumentation is expanded across many headers for buffer helpers, editor widgets, engine and menu APIs, feature contracts, weather registries, shader and material utilities, and supporting helper headers. HDR display implementation paths also add stricter UI redirection and copy guards. ChangesDocumentation and API comment expansion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
No actionable suggestions for changed features. |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove boilerplate /** @brief */ comments that restate method names (GetName, GetDisplayName, GetCategory, GetShaderDefineName, IsCore, LoadSettings, SaveSettings, RestoreDefaultSettings, GetSingleton, HasShaderDefine) across ~40 feature headers and the Feature base class. Keep only comments that add non-obvious behavior, constraints, or side effects. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/CSEditor/Widget.h (1)
452-461:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove the duplicate
SimpleFormWidgetdefinition.
src/CSEditor/Weather/SimpleFormWidget.halready defines this class, and that header also includes../Widget.h. Keeping a secondSimpleFormWidgetinWidget.hwill reintroduce the type in the same translation unit and break builds; please keep a single definition and reference it from the other header instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/CSEditor/Widget.h` around lines 452 - 461, The file contains a duplicate definition of class SimpleFormWidget (derived from Widget); remove this duplicate class block from the header so only the canonical SimpleFormWidget definition remains, and replace it with an `#include` of the dedicated header that contains the real definition where callers need it; ensure no other translation unit relies on the removed inline definition (update includes to pull in the single canonical header) so there is only one SimpleFormWidget type in the build.src/Feature.cpp (1)
77-87:⚠️ Potential issue | 🟡 MinorCheck unknown features before parsing
Info/Version(src/Feature.cpp lines 77-87).
REL::Version featureVersion(...)is constructed beforeFeature::IsFeatureKnown(...), so an unknown feature with a non-parseableInfo/Versioncan hit thecatchand be reported asVERSION_MISMATCHinstead ofUNKNOWN. Do theIsFeatureKnownlookup first and only parsefeatureVersionin the known-feature branch.src/ShaderFileWatcher.h (1)
61-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify direct vs. transitive dependents.
GetDependents()only returns the immediate entries inhlsliToHlsl, but the new doc says it returns files that “transitively include” the input. Either update the comment to say “direct dependents” or recurse through the graph if transitive invalidation is intended.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ShaderFileWatcher.h` around lines 61 - 75, The doc is wrong: GetDependents currently returns only direct dependents; update GetDependents to compute transitive dependents by performing a graph traversal (BFS/DFS) over the hlsliToHlsl adjacency map: normalize hlsliFile as now, then use a queue/stack plus a visited set to walk all reachable nodes from normalizedInc, accumulate each dependent into result without duplicates, keep the existing std::lock_guard(lock) on mutex while reading the map, and finally return the collected vector; reference symbols: GetDependents, hlsliToHlsl, mutex.src/Utils/Input.h (1)
110-133:⚠️ Potential issue | 🟡 MinorTighten
MatchesKeyboardCombocombo-input contract (or validate/normalize combos)
MatchesKeyboardComboonly usescombo.back()for the key and treats every earlier element as contributing to Ctrl/Shift/Alt. The hotkey combos created by the menu path are normalized to[optional Ctrl/Shift/Menu modifiers..., final pressed key], so non-modifier entries before the last element won’t come from there. However, combo deserialization insrc/Utils/Input.haccepts arbitrary arrays ofInputCombovalues without enforcing “modifier-only before last”, so a persisted/edited config containing non-modifier entries earlier could be incorrectly accepted.
- Make
MatchesKeyboardComboreject any earlier entries that aren’t Ctrl/Shift/Alt (or validate/normalize in the combo JSON parsing).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Utils/Input.h` around lines 110 - 133, MatchesKeyboardCombo currently assumes all combo entries except combo.back() are modifiers; update the function to validate that every element before the last is a modifier (Ctrl/Shift/Alt) and belongs to the keyboard device, and return false if any earlier element is non-modifier or non-keyboard. Concretely, inside MatchesKeyboardCombo iterate combo[0..combo.size()-2] (using InputCombo::GetKey and GetDevice and comparing to InputDeviceType::Keyboard) and check each modKey against VK_CONTROL/VK_LCONTROL/VK_RCONTROL, VK_SHIFT/VK_LSHIFT/VK_RSHIFT, or VK_MENU/VK_LMENU/VK_RMENU; if any entry fails those checks, return false before evaluating current key state. This tightens the contract here; alternatively you may also normalize/validate combos at JSON parse time, but implement the above guard in MatchesKeyboardCombo to prevent accepting malformed persisted combos.
🧹 Nitpick comments (1)
src/Features/PerformanceOverlay.cpp (1)
1-3: Consider a tighter conventional-commit title and explicit issue linkage when applicable.Current PR title is descriptive but exceeds the 50-char guideline. Suggested title:
docs(doxygen): expand api coverage and trim commentsIf any runtime-flow adjustments in this PR are intended to fix tracked issues, add PR keywords like
Closes #...orAddresses #...in the description.As per coding guidelines, "When reviewing PRs, please provide suggestions for Conventional Commit Titles ... Issue References ...".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Features/PerformanceOverlay.cpp` around lines 1 - 3, The PR title exceeds the 50-character conventional-commit guideline; please update the commit/PR title to a tighter conventional commit style such as "docs(doxygen): expand api coverage and trim comments" and, if this change addresses a tracked issue, add an explicit issue linkage in the PR description using keywords like "Closes `#123`" or "Addresses `#123`"; ensure the commit message and PR description follow the repo's Conventional Commit guidance so reviewers can quickly see the scope and any issue associations.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/SceneSettingsManager.h`:
- Around line 16-22: The class comment for SceneSettingsManager currently says
"mutations applied immediately" which conflicts with Update()'s doc stating
scene transitions are deferred until the next frame; update the class overview
to state that setting mutations are applied immediately but the actual scene
transition (the application of those settings to the scene) is deferred and
performed on the next frame by SceneSettingsManager::Update(), so callers
understand the immediate state change vs. next-frame application timing.
---
Outside diff comments:
In `@src/CSEditor/Widget.h`:
- Around line 452-461: The file contains a duplicate definition of class
SimpleFormWidget (derived from Widget); remove this duplicate class block from
the header so only the canonical SimpleFormWidget definition remains, and
replace it with an `#include` of the dedicated header that contains the real
definition where callers need it; ensure no other translation unit relies on the
removed inline definition (update includes to pull in the single canonical
header) so there is only one SimpleFormWidget type in the build.
In `@src/ShaderFileWatcher.h`:
- Around line 61-75: The doc is wrong: GetDependents currently returns only
direct dependents; update GetDependents to compute transitive dependents by
performing a graph traversal (BFS/DFS) over the hlsliToHlsl adjacency map:
normalize hlsliFile as now, then use a queue/stack plus a visited set to walk
all reachable nodes from normalizedInc, accumulate each dependent into result
without duplicates, keep the existing std::lock_guard(lock) on mutex while
reading the map, and finally return the collected vector; reference symbols:
GetDependents, hlsliToHlsl, mutex.
In `@src/Utils/Input.h`:
- Around line 110-133: MatchesKeyboardCombo currently assumes all combo entries
except combo.back() are modifiers; update the function to validate that every
element before the last is a modifier (Ctrl/Shift/Alt) and belongs to the
keyboard device, and return false if any earlier element is non-modifier or
non-keyboard. Concretely, inside MatchesKeyboardCombo iterate
combo[0..combo.size()-2] (using InputCombo::GetKey and GetDevice and comparing
to InputDeviceType::Keyboard) and check each modKey against
VK_CONTROL/VK_LCONTROL/VK_RCONTROL, VK_SHIFT/VK_LSHIFT/VK_RSHIFT, or
VK_MENU/VK_LMENU/VK_RMENU; if any entry fails those checks, return false before
evaluating current key state. This tightens the contract here; alternatively you
may also normalize/validate combos at JSON parse time, but implement the above
guard in MatchesKeyboardCombo to prevent accepting malformed persisted combos.
---
Nitpick comments:
In `@src/Features/PerformanceOverlay.cpp`:
- Around line 1-3: The PR title exceeds the 50-character conventional-commit
guideline; please update the commit/PR title to a tighter conventional commit
style such as "docs(doxygen): expand api coverage and trim comments" and, if
this change addresses a tracked issue, add an explicit issue linkage in the PR
description using keywords like "Closes `#123`" or "Addresses `#123`"; ensure the
commit message and PR description follow the repo's Conventional Commit guidance
so reviewers can quickly see the scope and any issue associations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c2c7250a-005f-4b9f-a802-5a5366133ba2
📒 Files selected for processing (125)
src/Buffer.hsrc/CSEditor/EditorWindow.cppsrc/CSEditor/EditorWindow.hsrc/CSEditor/InteriorOnlyPanel.hsrc/CSEditor/LightEditor.hsrc/CSEditor/PaletteWindow.hsrc/CSEditor/Weather/CellLightingWidget.cppsrc/CSEditor/Weather/CellLightingWidget.hsrc/CSEditor/Weather/ImageSpaceWidget.hsrc/CSEditor/Weather/LensFlareWidget.hsrc/CSEditor/Weather/LightingTemplateWidget.hsrc/CSEditor/Weather/PrecipitationWidget.hsrc/CSEditor/Weather/ReferenceEffectWidget.hsrc/CSEditor/Weather/SimpleFormWidget.hsrc/CSEditor/Weather/VolumetricLightingWidget.hsrc/CSEditor/Weather/WeatherWidget.cppsrc/CSEditor/Weather/WeatherWidget.hsrc/CSEditor/WeatherUtils.hsrc/CSEditor/Widget.hsrc/Deferred.hsrc/EngineFix.hsrc/EngineFixes/ShadowmapCascadeCullingFix.hsrc/EngineFixes/ShadowmapCascadeRasterizerFix.hsrc/Feature.cppsrc/Feature.hsrc/FeatureBuffer.hsrc/FeatureCategories.hsrc/FeatureConstraints.hsrc/FeatureIssues.cppsrc/FeatureIssues.hsrc/Features/CloudShadows.cppsrc/Features/CloudShadows.hsrc/Features/DynamicCubemaps.cppsrc/Features/DynamicCubemaps.hsrc/Features/ExponentialHeightFog.hsrc/Features/ExtendedMaterials.hsrc/Features/ExtendedTranslucency.hsrc/Features/GrassCollision.hsrc/Features/GrassLighting.hsrc/Features/HDRDisplay.cppsrc/Features/HDRDisplay.hsrc/Features/HairSpecular.hsrc/Features/IBL.cppsrc/Features/IBL.hsrc/Features/InteriorSun.hsrc/Features/InverseSquareLighting.hsrc/Features/InverseSquareLighting/Common.hsrc/Features/LODBlending.hsrc/Features/LightLimitFix.hsrc/Features/LinearLighting.hsrc/Features/PerformanceOverlay.cppsrc/Features/PerformanceOverlay/ABTesting/ABTestAggregator.hsrc/Features/PerformanceOverlay/ABTesting/ABTesting.hsrc/Features/ScreenSpaceGI.hsrc/Features/ScreenSpaceShadows.hsrc/Features/ScreenSpaceShadows/bend_sss_cpu.hsrc/Features/ScreenshotFeature.hsrc/Features/Skin.hsrc/Features/SkySync.hsrc/Features/Skylighting.hsrc/Features/SubsurfaceScattering.hsrc/Features/TerrainBlending.cppsrc/Features/TerrainBlending.hsrc/Features/TerrainHelper.cppsrc/Features/TerrainHelper.hsrc/Features/TerrainShadows.hsrc/Features/TerrainVariation.cppsrc/Features/TerrainVariation.hsrc/Features/UnifiedWater.hsrc/Features/UnifiedWater/Flowmap.hsrc/Features/UnifiedWater/WaterCache.hsrc/Features/Upscaling.hsrc/Features/Upscaling/FidelityFX.hsrc/Features/Upscaling/Streamline.hsrc/Features/VolumetricLighting.hsrc/Features/VolumetricShadows.cppsrc/Features/VolumetricShadows.hsrc/Features/WaterEffects.hsrc/Features/WetnessEffects.cppsrc/Features/WetnessEffects.hsrc/Globals.hsrc/Hooks.hsrc/I18n/I18n.hsrc/Menu.hsrc/Menu/AdvancedSettingsRenderer.hsrc/Menu/BackgroundBlur.hsrc/Menu/CursorLoader.hsrc/Menu/FeatureListRenderer.hsrc/Menu/HomePageRenderer.hsrc/Menu/IconLoader.hsrc/Menu/MenuHeaderRenderer.hsrc/Menu/ProfilingRenderer.hsrc/Menu/SettingsTabRenderer.hsrc/Profiler.hsrc/SceneSettingsManager.hsrc/SettingsOverrideManager.cppsrc/SettingsOverrideManager.hsrc/ShaderCache.hsrc/ShaderFileWatcher.hsrc/ShaderTools/BSShader.hsrc/ShaderTools/BSShaderHooks.hsrc/ShaderTools/ShaderCompiler.hsrc/State.hsrc/TruePBR.hsrc/TruePBR/BSLightingShaderMaterialPBR.hsrc/TruePBR/BSLightingShaderMaterialPBRLandscape.hsrc/Utils/D3D.cppsrc/Utils/D3D.hsrc/Utils/ExternalEmittance.hsrc/Utils/FileSystem.cppsrc/Utils/Form.cppsrc/Utils/Form.hsrc/Utils/Format.cppsrc/Utils/Game.cppsrc/Utils/Input.hsrc/Utils/LegitProfiler.hsrc/Utils/Moon.hsrc/Utils/Serialize.hsrc/Utils/SphericalHarmonics.hsrc/Utils/Subrect.hsrc/Utils/UI.cppsrc/Utils/UI.hsrc/Utils/WinApi.hsrc/WeatherManager.hsrc/WeatherVariableRegistry.h
💤 Files with no reviewable changes (18)
- src/Features/TerrainVariation.cpp
- src/CSEditor/Weather/CellLightingWidget.cpp
- src/Features/IBL.cpp
- src/Utils/Form.cpp
- src/Features/CloudShadows.cpp
- src/Utils/Format.cpp
- src/Utils/UI.cpp
- src/Features/TerrainHelper.cpp
- src/Utils/FileSystem.cpp
- src/Features/VolumetricShadows.cpp
- src/Features/WetnessEffects.cpp
- src/SettingsOverrideManager.cpp
- src/Features/DynamicCubemaps.cpp
- src/Features/TerrainBlending.cpp
- src/Utils/Game.cpp
- src/CSEditor/Weather/WeatherWidget.cpp
- src/CSEditor/EditorWindow.cpp
- src/Features/HDRDisplay.cpp
The doxygen coverage pass incorrectly deleted standalone // and /// comments from .cpp function bodies and .h section dividers that were not replaced by /** */ blocks. Restore them without reverting any legitimate code changes or added doxygen. - Restored section comments in EditorWindow.cpp, WeatherWidget.cpp, CellLightingWidget.cpp, PerformanceOverlay.cpp, and ~15 utility/ feature cpp files (pure comment-only deletions) - Restored // section group dividers and field annotations in Menu.h, State.h, Feature.h, FeatureIssues.h, Deferred.h - Restored // section headers in CSEditor/EditorWindow.h, bend_sss_cpu.h, HomePageRenderer.h - Restored // and /// comments in WetnessEffects.cpp, D3D.cpp that were alongside real code changes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- SceneSettingsManager: clarify that scene mutations are applied on the next frame by Update(), not immediately on event receipt - ShaderFileWatcher: correct GetDependents() doc from "transitively" to "directly" (only looks up immediate hlsliToHlsl entries) - Input: validate non-last combo entries are keyboard modifier keys; return false for any non-modifier or non-keyboard early entry Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
✅ A pre-release build is available for this PR: |
|
|
||
| // Non-copyable, non-movable for simplicity | ||
| ScopedD3DMap(const ScopedD3DMap&) = delete; | ||
| ScopedD3DMap(const ScopedD3DMap&) = delete; |
There was a problem hiding this comment.
Indentation regression (formatting). This line picked up an extra leading tab (3 tabs) while its sibling members (operator=, IsValid(), etc.) use 2. It misaligns inside the class and will fail clang-format / the FORMAT_CODE CI check. This looks like an accidental edit from the doxygen pass.
| ScopedD3DMap(const ScopedD3DMap&) = delete; | |
| ScopedD3DMap(const ScopedD3DMap&) = delete; |
There was a problem hiding this comment.
Fixed — removed the extra leading tab on the copy-ctor delete line to match its siblings.
|
|
||
| bool requiresCtrl = false, requiresShift = false, requiresAlt = false; | ||
| for (size_t i = 0; i < combo.size() - 1; ++i) { | ||
| if (combo[i].GetDevice() != InputDeviceType::Keyboard) |
There was a problem hiding this comment.
Behavioral change in a docs-only PR. This hunk adds real runtime logic to MatchesKeyboardCombo (reject combos whose prefix contains a non-keyboard device or a non-modifier key), yet the PR is titled docs: and the description states "comments-only change, no code modifications" with a test plan of only "verify build compiles."
The change itself looks correct (it stops combos like a mouse-button or plain-key prefix from spuriously matching on the final key alone), so I am not asking to revert it. But it is undisclosed logic that the stated test plan will not exercise, and reviewers approving a "docs" PR will not scrutinize it. Please either split this into its own fix: commit/PR with a proper test note, or call it out explicitly in the description so it gets a behavioral review.
There was a problem hiding this comment.
Updated the PR description to explicitly call out the MatchesKeyboardCombo logic fix and add a behavioral test note.
|
|
||
| // Disable checkbox if no HDR monitor detected AND HDR is not already enabled | ||
| // (Allow disabling HDR even on SDR if it's already on from saved settings) | ||
| if (!isHDRMonitor && !currentEnableHDR) { |
There was a problem hiding this comment.
Lost "why" comment. The doxygen pass deleted the rationale for this non-obvious gate: (Allow disabling HDR even on SDR if it's already on from saved settings). The condition !isHDRMonitor && !currentEnableHDR reads as arbitrary without it, and the PR description explicitly claims it "Preserved comments that explain WHY." This one slipped through. Suggest restoring it:
| if (!isHDRMonitor && !currentEnableHDR) { | |
| // Disable the checkbox only when no HDR monitor is detected AND HDR is not already on | |
| // (allow disabling HDR even on SDR if it was enabled from saved settings). | |
| if (!isHDRMonitor && !currentEnableHDR) { |
There was a problem hiding this comment.
Restored the why-comment explaining the HDR checkbox gate (allow disabling even on SDR if saved settings had it on).
- Fix indentation regression on ScopedD3DMap copy-ctor delete (extra tab) - Restore why-comment on HDRDisplay::DrawSettings HDR checkbox gate Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
Summary
/** @brief */doxygen documentation to public methods across 118/119 header files, bringing coverage from ~12% to 99.2%///doxygen-style comments to/** */block format for consistencyScope
Behavioral fix included
src/Utils/Input.h—MatchesKeyboardCombonow rejects combos whose prefix entries contain a non-keyboard device or a non-modifier key (previously such combos could spuriously match on the final key alone). This is a correctness fix surfaced during the doxygen review of that function, included here because it is a one-liner with no test infrastructure needed; it should receive a behavioral review.Test plan
MatchesKeyboardCombo: confirm a combo with a mouse-button prefix returns false; confirm a normalCtrl+Kcombo still matches🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Bug Fixes
New Features