chore: set extended translucency category to "lighting"#1264
Conversation
Co-authored-by: alandtse <7086117+alandtse@users.noreply.github.com>
|
@coderabbitai do you concur with the category choice based off the functionality? Please provide a suggestion if you think there are better alternatives. |
|
You are on the CodeRabbit Free Plan. In order to use the Chat feature, please upgrade to CodeRabbit Pro. |
alandtse
changed the title
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Join our Discord community 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
|
alandtse
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR updates the Extended Translucency feature so it appears under the “Lighting” category in the UI rather than “Other.”
- Adds a
GetCategory()override returning"Lighting" - Ensures the feature groups with other lighting-based rendering features
Comments suppressed due to low confidence (1)
src/Features/ExtendedTranslucency.h:13
- Add a unit test to verify that
ExtendedTranslucency::GetCategory()returns "Lighting" to prevent future regressions in feature categorization.
virtual std::string_view GetCategory() const override { return "Lighting"; }
| virtual inline std::string GetName() override { return "Extended Translucency"; } | ||
| virtual inline std::string GetShortName() override { return "ExtendedTranslucency"; } | ||
| virtual inline std::string_view GetShaderDefineName() override { return "EXTENDED_TRANSLUCENCY"; } | ||
| virtual std::string_view GetCategory() const override { return "Lighting"; } |
There was a problem hiding this comment.
[nitpick] Avoid hard-coded string literals by defining category names as shared constants or enums; this reduces typo risk and centralizes updates.
| virtual std::string_view GetCategory() const override { return "Lighting"; } | |
| static constexpr std::string_view CategoryName = "Lighting"; | |
| virtual std::string_view GetCategory() const override { return CategoryName; } |
There was a problem hiding this comment.
@coderabbitai please create an issue to track the suggestion to have the categories in an enum or centralized constants and to update all other features using this new central location. Do not assign to anyone yet but tag is as help-wanted and as a good first issue.
There was a problem hiding this comment.
🧩 Analysis chain
I'll create an issue to track this suggestion. Let me first search the codebase to understand the current scope and find other features that use similar patterns.
Now I'll create the GitHub issue with the information gathered:
@alandtse I've created an issue to track the suggestion for centralizing feature category names. The issue includes:
- Problem description: Explains the current hard-coded string literal approach and its drawbacks
- Current implementation: Shows the existing pattern with examples
- Proposed solution: Outlines options for centralization (enum vs constants approach)
- Scope: Based on the codebase analysis, multiple features would benefit from this change
- Labels: Added "help wanted" and "good first issue" as requested
- References: Links back to the original suggestion and PR
The issue provides enough context and implementation guidance for a contributor to tackle this improvement while being accessible for newcomers to the codebase.
🏁 Script executed:
#!/bin/bash
# Search for GetCategory() implementations across the codebase
rg "GetCategory.*override" -A 2 -B 1
# Also search for any existing category-related constants or enums
rg "Category.*=" --type cpp
rg "enum.*Category" --type cpp
Length of output: 10328
🏁 Script executed:
#!/bin/bash
# Search for all string literals returned by GetCategory methods
rg 'return.*".*".*}' --type cpp | rg -i category
# Find all files that contain GetCategory method
fd -e h -e cpp | xargs grep -l "GetCategory" | head -20
Length of output: 3739
🏁 Script executed:
#!/bin/bash
# Get more context on the Feature base class to understand the pattern
ast-grep --pattern 'virtual $$$GetCategory$$$'
# Look for the Feature base class definition
rg "class.*Feature" -A 5 -B 2
Length of output: 966
🏁 Script executed:
#!/bin/bash
# Create GitHub issue for centralizing category names
gh issue create \
--title "Centralize feature category names to avoid hard-coded string literals" \
--body "## Problem
Currently, feature categories are defined as hard-coded string literals in each feature's \`GetCategory()\` method. This approach has several drawbacks:
- Risk of typos and inconsistencies
- Difficult to maintain and update category names
- No central location to manage available categories
## Current Implementation
Features like \`ExtendedTranslucency\` define categories inline:
\`\`\`cpp
virtual std::string_view GetCategory() const override { return \"Lighting\"; }
\`\`\`
## Proposed Solution
1. **Create a centralized location** for category definitions, either:
- An enum class for category types
- Static constexpr string_view constants
- A dedicated CategoryNames namespace/class
2. **Update all features** to use the centralized category definitions instead of string literals
3. **Benefits**:
- Eliminates typo risks
- Centralizes category management
- Makes it easier to add/rename categories
- Improves code maintainability
## Implementation Ideas
Option 1 - Enum approach:
\`\`\`cpp
enum class FeatureCategory {
Lighting,
PostProcessing,
Rendering,
// ... other categories
};
\`\`\`
Option 2 - Constants approach:
\`\`\`cpp
namespace CategoryNames {
static constexpr std::string_view Lighting = \"Lighting\";
static constexpr std::string_view PostProcessing = \"PostProcessing\";
// ... other categories
}
\`\`\`
## Scope
Based on the codebase analysis, this change would affect multiple feature implementations that currently use hard-coded category strings.
## References
- Original suggestion: https://github.com/doodlum/skyrim-community-shaders/pull/1264#discussion_r2206719306
- PR: https://github.com/doodlum/skyrim-community-shaders/pull/1264" \
--label "help wanted" \
--label "good first issue"
Length of output: 1934
@alandtse I've successfully created issue #1265 to track the suggestion for centralizing feature category names.
Based on my analysis of the codebase, I found that there are 28+ features currently using hard-coded string literals for categories, with the following categories being used:
- "Lighting" (9 features): ExtendedTranslucency, ScreenSpaceGI, LightLimitFix, VolumetricLighting, ScreenSpaceShadows, InverseSquareLighting, InteriorSunShadows, IBL, DynamicCubemaps
- "Landscape & Textures" (6 features): ExtendedMaterials, TerrainBlending, TerrainVariation, TerrainHelper, TerrainShadows, LODBlending
- "Water" (2 features): WetnessEffects, WaterEffects
- "Characters" (2 features): SubsurfaceScattering, HairSpecular
- "Sky" (3 features): Skylighting, SkySync, CloudShadows
- "Grass" (2 features): GrassLighting, GrassCollision
- "Debug" (2 features): WeatherPicker, OverlayFeature
- "Other" (1 feature): Base Feature class default
The issue has been created with:
- ✅ "help wanted" and "good first issue" labels
- ✅ Comprehensive problem description and proposed solutions
- ✅ Implementation examples (enum vs constants approaches)
- ✅ References to the original PR and comment
- ✅ Clear scope showing 28+ features would benefit from this change
This refactoring would provide significant value by eliminating the risk of typos, centralizing category management, and making it easier to maintain consistency across all features.
🧬 Code Graph Analysis Results
Possibly relevant snippets from src/Feature.h (line 92-93)
Load(json& o_json)
Save(json& o_json)These appear to be base class methods related to loading and saving settings from/to JSON, which are overridden in ExtendedTranslucency as LoadSettings and SaveSettings. They are relevant for understanding how feature settings are managed and persisted.
Possibly relevant snippets from src/Features/WetnessEffects.h (line 109-110)
LoadSettings(json& o_json) override
SaveSettings(json& o_json) overrideThese override methods for loading and saving settings in another feature provide context on how similar features implement these functions, which may be useful when considering centralizing categories or constants.
Possibly relevant snippets from src/Features/TerrainVariation.h (line 18-21)
virtual inline bool HasShaderDefine(RE::BSShader::Type shaderType) override
{
return (shaderType == RE::BSShader::Type::Lighting);
}This snippet shows a similar implementation of HasShaderDefine as in ExtendedTranslucency, indicating a pattern across features that might be relevant when refactoring or centralizing constants.
Possibly relevant snippets from src/Features/TerrainVariation.h (line 47-48)
LoadSettings(json& o_json) override
SaveSettings(json& o_json) overrideThese methods are similar overrides for settings management in another feature, again useful for understanding common patterns in feature classes.
No other snippets directly reference or define categories or enums for feature categories, so these are the most relevant for understanding the current structure and potential refactoring points related to the user comment.
🧠 Learnings used
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
|
✅ A pre-release build is available for this PR: |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
alandtse
changed the title
…aders#1264) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: alandtse <7086117+alandtse@users.noreply.github.com> Co-authored-by: Alan Tse <alandtse@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Extended Translucency was defaulting to the "Other" category in the UI, but this feature is fundamentally about lighting - specifically how light transmits through translucent materials like fabric.
The feature provides:
This change moves Extended Translucency to the "Lighting" category alongside other lighting features like:
Change: Added
GetCategory()override to return "Lighting" instead of the default "Other".Fixes #1263.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.