feat(ui): add subheading organisation to the features panel#1155
Conversation
|
""" WalkthroughA category system for features was introduced, adding a virtual Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Menu
participant Feature
participant Util
User->>Menu: Open feature menu
Menu->>Feature: Query GetCategory() for each feature
Menu->>Menu: Group features by category
Menu->>Util: DrawCategoryHeader(categoryName, isExpanded)
alt Category expanded
Menu->>Util: Render features under category
end
Note over Menu: Category expansion state persists
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
src/Features/SubsurfaceScattering.h (1)
69-69: Same centralisation concern as raised in InteriorSunShadows.hSee earlier comment about consolidating category literals.
src/Features/WetnessEffects.h (1)
19-19: Same centralisation concern as raised in InteriorSunShadows.hSee earlier comment about consolidating category literals.
src/Features/GrassCollision.h (1)
19-19: Same centralisation concern as raised in InteriorSunShadows.hSee earlier comment about consolidating category literals.
src/Features/GrassLighting.h (1)
19-19: Same centralisation concern as raised in InteriorSunShadows.hSee earlier comment about consolidating category literals.
src/Features/TerrainHelper.h (1)
18-18: Same “string-literal category” duplication as noted in HairSpecularPlease see the earlier comment about centralising category values to prevent drift.
src/Features/ScreenSpaceShadows.h (1)
19-19: Same “string-literal category” duplication as noted previouslyRefer to the suggestion in
HairSpecular.h.src/Features/TerrainShadows.h (1)
21-21: Same “string-literal category” duplication as noted previouslyRefer to the suggestion in
HairSpecular.h.
🧹 Nitpick comments (10)
src/Feature.h (1)
47-52: Consider centralising category literals and marking the accessor as[[nodiscard]].Having every feature return a raw string literal is safe but error-prone: a single typo in one override produces an extra, empty header in the menu.
A lightweight enum → string map, or at minimumconstexpr std::string_viewconstants collected in one header (e.g.Categories.h) would give compile-time checking and auto-completion support.While touching the signature, adding
[[nodiscard]]signals callers that the value is meant to be used.- virtual std::string_view GetCategory() const { return "Other"; } + [[nodiscard]] virtual std::string_view GetCategory() const { return CATEGORY_OTHER; }(where
CATEGORY_OTHERwould be defined in the proposed common header).src/Features/LODBlending.h (1)
14-14: Addinline(or a shared constant) for header-definedGetCategory()to keep ODR hygiene and avoid string typosAll other small methods in this header are declared
inline; omitting it here is an inconsistency that can trigger multiple-definition linker errors if the header is included in more than one TU without-fvisibility-inlines-hidden.- virtual std::string_view GetCategory() const override { return "Landscape & Textures"; } + virtual inline std::string_view GetCategory() const override { return "Landscape & Textures"; }Long-term, consider centralising category strings in a
constexpr std::string_view(or enum → lookup) to prevent typos and make refactors trivial.src/Features/TerrainBlending.h (1)
15-15: Consistency: mark the header-defined overrideinlineSame rationale as for
LODBlending: keep style uniform and eliminate the tiny risk of ODR violations.- virtual std::string_view GetCategory() const override { return "Landscape & Textures"; } + virtual inline std::string_view GetCategory() const override { return "Landscape & Textures"; }src/Features/InverseSquareLighting.h (1)
19-19: Inline qualifier missingFor uniformity with neighbouring one-liners (
GetName(),GetShortName()…) and safer linkage, addinline.- virtual std::string_view GetCategory() const override { return "Lighting"; } + virtual inline std::string_view GetCategory() const override { return "Lighting"; }src/Features/ScreenSpaceGI.h (1)
21-21: Inline qualifierSame minor consistency issue—worth fixing while the file is touched.
- virtual std::string_view GetCategory() const override { return "Lighting"; } + virtual inline std::string_view GetCategory() const override { return "Lighting"; }src/Features/VolumetricLighting.h (1)
35-35: Inline for header methodKeep all tiny accessors marked
inlineto avoid redundant symbol emission.- virtual std::string_view GetCategory() const override { return "Lighting"; } + virtual inline std::string_view GetCategory() const override { return "Lighting"; }src/Features/InteriorSunShadows.h (1)
14-14: Category string is hard-coded; consider centralising to avoid driftEvery feature now embeds its own string literal for the category. Typos or later renames will require touching dozens of files. A small
enum class FeatureCategory { Lighting, Grass, … }or at least a header with constexprstd::string_viewconstants would eliminate that risk and simplify refactors.No action is strictly required for this PR, but worth tracking before more features are added.
src/Utils/UI.cpp (1)
103-157: Well-implemented custom category header rendering.The function correctly handles interaction, dynamic sizing, and edge cases. Consider extracting the magic numbers (padding: 20.0f, vertical spacing: 8.0f) as named constants for better maintainability.
src/Menu.cpp (2)
366-367: Consider persisting category expansion states.The static map maintains expansion states only during the current session. Users might expect these UI preferences to persist across application restarts. Consider saving/loading these states as part of the menu settings.
614-614: Consider making category order configurable.The hardcoded category order works well but could be made configurable to allow users or modders to customize the menu organization according to their preferences.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
src/Feature.h(1 hunks)src/Features/CloudShadows.h(1 hunks)src/Features/DynamicCubemaps.h(1 hunks)src/Features/ExtendedMaterials.h(1 hunks)src/Features/GrassCollision.h(1 hunks)src/Features/GrassLighting.h(1 hunks)src/Features/HairSpecular.h(1 hunks)src/Features/IBL.h(1 hunks)src/Features/InteriorSunShadows.h(1 hunks)src/Features/InverseSquareLighting.h(1 hunks)src/Features/LODBlending.h(1 hunks)src/Features/LightLimitFix.h(1 hunks)src/Features/ScreenSpaceGI.h(1 hunks)src/Features/ScreenSpaceShadows.h(1 hunks)src/Features/SkySync.h(1 hunks)src/Features/Skylighting.h(1 hunks)src/Features/SubsurfaceScattering.h(1 hunks)src/Features/TerrainBlending.h(1 hunks)src/Features/TerrainHelper.h(1 hunks)src/Features/TerrainShadows.h(1 hunks)src/Features/TerrainVariation.h(1 hunks)src/Features/VolumetricLighting.h(1 hunks)src/Features/WaterEffects.h(1 hunks)src/Features/WetnessEffects.h(1 hunks)src/Menu.cpp(5 hunks)src/Utils/UI.cpp(2 hunks)src/Utils/UI.h(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/Menu.cpp (2)
src/Utils/UI.cpp (4)
DrawSectionHeader(159-192)DrawSectionHeader(159-159)DrawCategoryHeader(103-157)DrawCategoryHeader(103-103)src/Utils/UI.h (2)
DrawSectionHeader(126-126)DrawCategoryHeader(119-119)
src/Utils/UI.cpp (1)
src/Utils/UI.h (1)
hovered(22-22)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (15)
src/Features/SkySync.h (1)
14-15: Category override looks good.The override matches the new base signature (
const,override) and uses a safe string literal. No issues spotted.src/Features/TerrainVariation.h (1)
21-22: Category override looks good.Consistent with the rest of the codebase and the UI grouping strategy.
src/Features/LightLimitFix.h (1)
21-22: Category override looks good.The
"Lighting"literal is concise and will group correctly.src/Features/ExtendedMaterials.h (1)
14-15: Category override looks good.No further comments.
src/Features/WaterEffects.h (1)
22-22: Override GetCategory() for UI grouping
Correctly introduces theGetCategory()override returning"Water", aligning this feature with the new collapsible “Water” section in the UI.src/Features/IBL.h (1)
17-17: Override GetCategory() for UI grouping
TheGetCategory()override returns"Lighting", ensuring this feature appears under the Lighting section in the feature panel.src/Features/DynamicCubemaps.h (1)
105-105: Override GetCategory() for UI grouping
TheGetCategory()override returning"Lighting"is correct and will group this feature under the Lighting header.src/Features/CloudShadows.h (1)
26-26: Override GetCategory() for UI grouping
This override returns"Sky", which aligns CloudShadows with the Sky category in the collapsible UI menu.src/Features/Skylighting.h (1)
21-21: Override GetCategory() for UI grouping
TheGetCategory()override here returns"Sky", correctly grouping Skylighting under the Sky section of the features panel.src/Utils/UI.h (2)
3-3: Good call including<imgui.h>directlyThis makes
UI.hself-contained and prevents order-dependent build breaks.
113-126: AddUTIL_API/IMGUI_APIvisibility decorators (orinline) to new helpers
DrawCategoryHeader/DrawSectionHeaderwill be defined in UI.cpp.
Without a visibility/export macro they will have default internal linkage on MSVC-DLL builds, causing unresolved externals when the header is included from other translation units in a DLL.
If the project already uses an export macro (e.g.,UTIL_API), mark them:-bool DrawCategoryHeader(const char* categoryName, bool& isExpanded); -void DrawSectionHeader(const char* sectionName, bool useWhiteText = false); +UTIL_API bool DrawCategoryHeader(const char* categoryName, bool& isExpanded); +UTIL_API void DrawSectionHeader(const char* sectionName, bool useWhiteText = false);If you intend them to be header-only, add
inlineinstead.src/Utils/UI.cpp (2)
2-2: LGTM!The ImGui header include is necessary for the new UI utility functions.
159-192: Clean implementation of non-interactive section headers.The function appropriately reuses the visual style from DrawCategoryHeader while removing interaction logic. The conditional text coloring provides good flexibility.
src/Menu.cpp (2)
24-24: LGTM!Required include for the new UI utility functions.
393-410: Good integration of the new UI utilities.The visitor implementation properly handles both special section headers and interactive category headers, maintaining consistent styling across the UI.
davo0411
changed the title
davo0411
changed the title
|
✅ A pre-release build is available for this PR: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Menu.h (1)
69-80: Make the new color palette immutable & avoid per-instance memory overhead
UITemplateColorsis instantiated as a mutable data member (UITemplate).
BecauseMenu::Settings::Themealready lives as a data member inside everyMenu
instance, every copy incurs ~84 bytes for this palette. The values are compile-time
constants and should never be mutated at runtime, so you can both:
- Prevent accidental modification.
- Have the data emitted only once in the binary.
- struct UITemplateColors + struct UITemplateColors { … - } UITemplate; + }; + + // One read-only instance shared by all ThemeSettings + inline static constexpr UITemplateColors UITemplate{};This keeps call-sites unchanged (
theme.UITemplate.*) while making the
paletteconstexpr, deduplicating storage and improving clarity that the
colors are part of the theme’s constant definition rather than user-editable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Menu.h(1 hunks)src/Utils/UI.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Utils/UI.cpp
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/Menu.cpp (1)
108-115: ThemeSettings JSON layout now out of sync with previous savesAdding
FeatureHeadingin the middle of the aggregate struct changes the serialized order.
Oldsettings.jsonfiles will silently read the new field asStyle, shifting everything that follows.
Either:
- Add the new member at the end of the struct, or
- Bump the settings version and perform migration.
Failing to do so will corrupt all stored user themes.
🧹 Nitpick comments (4)
src/Menu.cpp (4)
36-41: Naming inconsistency in StatusPaletteColors
SuccessColor/InfoColorare the only palette entries with the redundant “Color” suffix, while peer fields are simplyDisable,Error, etc. This breaks the otherwise consistent naming scheme and will read oddly at all call-sites (StatusPalette.SuccessColor). Consider dropping the suffix to keep the API uniform.- SuccessColor, - InfoColor + Success, + Info
42-48: Implicit default construction may hide bugsBoth
LineColorHoveredandTextColorHovereddefault-construct to black/zero if the JSON omits them.
That makes the header unreadable in the UI yet fails silently. Provide sane defaults or validate the JSON when loading.+ LineColorHovered = { 1.0f, 1.0f, 1.0f, 0.25f }, + TextColorHovered = { 1.0f, 1.0f, 1.0f, 1.0f },
367-381: Static map for expansion state is fine, butstd::mapis over-killA dozen categories are stored;
unordered_mapgives O(1) and smaller codegen.
Also, no locking is required because rendering is single-threaded.- static std::map<std::string, bool> categoryExpansionStates; + static std::unordered_map<std::string, bool> categoryExpansionStates;
884-896: Color editor exposes new fields that are not consumed anywhere
FeatureHeading.*colors are editable but never applied inSetupImGuiStyle(). If they are only used insideUtil::DrawCategoryHeader, ensure that helper reads them; otherwise users will think the setting is broken.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Menu.cpp(8 hunks)src/Menu.h(1 hunks)src/Utils/UI.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Utils/UI.cpp
- src/Menu.h
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (1)
src/Menu.cpp (1)
602-646: Category collapse takes one frame to propagate
menuListis rebuilt before the header is drawn, so toggling a category hides/shows its contents only on the next frame, causing a perceptible flicker.
Consider rebuilding the list immediately after the toggle or, more simply, draw feature rows unconditionally and rely onImGui::BeginDisabled/spacing to hide them—avoiding the rebuild altogether.
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
Merge able once ci passes. Thanks.
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
2 participants
(ignore image 2 having duplicate terrain / Landscape & textures panels, the final code is only "Landscape & Textures", "Terrain" is gone.
Adds expandable subheadings to the left-hand features pane for better organisation.
Part of my ongoing UI changes.
Summary by CodeRabbit