fix: sync feature IsCore() with CORE marker file#2266
Conversation
📝 WalkthroughWalkthroughThe PR introduces core feature detection by adding a ChangesCore Feature Detection Pipeline
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
|
No actionable suggestions for changed features. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CMakeLists.txt (1)
229-248:⚠️ Potential issue | 🟠 MajorCORE marker file changes don't trigger
FeatureVersions.hregeneration.
IsCoremembership is determined by checking for the CORE marker file at line 236, but CMake's reconfigure dependencies at line 244 only include*.inifiles. Adding or removing a CORE marker file won't trigger a rebuild ofFeatureVersions.huntil CMake is manually reconfigured.Add a separate glob for CORE marker files and include them in
CMAKE_CONFIGURE_DEPENDS:Proposed CMake fix
file( GLOB_RECURSE FEATURE_CONFIG_FILES LIST_DIRECTORIES false CONFIGURE_DEPENDS "features/*/Shaders/Features/*.ini" ) +file( + GLOB FEATURE_CORE_MARKER_FILES + LIST_DIRECTORIES false + CONFIGURE_DEPENDS + "features/*/CORE" +) foreach(FEATURE_PATH ${FEATURE_CONFIG_FILES}) @@ endforeach() set_property( DIRECTORY APPEND - PROPERTY CMAKE_CONFIGURE_DEPENDS "${FEATURE_CONFIG_FILES}" + PROPERTY CMAKE_CONFIGURE_DEPENDS + "${FEATURE_CONFIG_FILES}" + "${FEATURE_CORE_MARKER_FILES}" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 229 - 248, The CMake logic detects CORE marker files by inspecting the parent directories of FEATURE_PATH and appends names to FEATURE_CORE_NAMES, but CMAKE_CONFIGURE_DEPENDS only includes FEATURE_CONFIG_FILES so adding/removing a CORE marker won't trigger regeneration of FeatureVersions.h; update the build dependencies by creating a separate glob/variable that collects the CORE marker paths (e.g., by globbing features/*/CORE or deriving from FEATURE_PATH) and then APPEND that variable to the DIRECTORY PROPERTY CMAKE_CONFIGURE_DEPENDS alongside FEATURE_CONFIG_FILES so changes to CORE marker files cause CMake to reconfigure and regenerate FeatureVersions.h (update references around FEATURE_PATH, FEATURE_CORE_NAMES, FEATURE_CONFIG_FILES, and the existing set_property CMAKE_CONFIGURE_DEPENDS).
🧹 Nitpick comments (2)
CMakeLists.txt (1)
229-239: Please add an issue-closing reference in the PR description.Since this is a bug fix, add a keyword like
Fixes #<id>orCloses #<id>if there is a tracked issue.As per coding guidelines
**/*: "Issue References (if PR fixes bugs or implements features): 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 `@CMakeLists.txt` around lines 229 - 239, This PR is missing an issue-closing keyword in its description; update the PR description to include a GitHub issue reference such as "Fixes #<id>" or "Closes #<id>" so the bug fix is automatically linked/closed (add the keyword line to the PR body before merging).src/Feature.h (1)
62-65: MakeGetShortName()const across the hierarchy to eliminateconst_castinIsCore().All implementations return string literals with no side effects. Add
constto the base virtual declaration in Feature.h (line 31) and to all ~36 override implementations throughout the Features/ directory. This removes the unsafe const_cast and improves const-correctness of the API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Feature.h` around lines 62 - 65, Update the virtual GetShortName() declaration to be const in the Feature base (change virtual const char* GetShortName() to a const-qualified method) and update every override implementation in the Features/ hierarchy to match that const signature (about ~36 overrides), then remove the const_cast from IsCore() and call GetShortName() directly; ensure all signatures match exactly so the code compiles and run unit tests to verify no regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@CMakeLists.txt`:
- Around line 229-248: The CMake logic detects CORE marker files by inspecting
the parent directories of FEATURE_PATH and appends names to FEATURE_CORE_NAMES,
but CMAKE_CONFIGURE_DEPENDS only includes FEATURE_CONFIG_FILES so
adding/removing a CORE marker won't trigger regeneration of FeatureVersions.h;
update the build dependencies by creating a separate glob/variable that collects
the CORE marker paths (e.g., by globbing features/*/CORE or deriving from
FEATURE_PATH) and then APPEND that variable to the DIRECTORY PROPERTY
CMAKE_CONFIGURE_DEPENDS alongside FEATURE_CONFIG_FILES so changes to CORE marker
files cause CMake to reconfigure and regenerate FeatureVersions.h (update
references around FEATURE_PATH, FEATURE_CORE_NAMES, FEATURE_CONFIG_FILES, and
the existing set_property CMAKE_CONFIGURE_DEPENDS).
---
Nitpick comments:
In `@CMakeLists.txt`:
- Around line 229-239: This PR is missing an issue-closing keyword in its
description; update the PR description to include a GitHub issue reference such
as "Fixes #<id>" or "Closes #<id>" so the bug fix is automatically linked/closed
(add the keyword line to the PR body before merging).
In `@src/Feature.h`:
- Around line 62-65: Update the virtual GetShortName() declaration to be const
in the Feature base (change virtual const char* GetShortName() to a
const-qualified method) and update every override implementation in the
Features/ hierarchy to match that const signature (about ~36 overrides), then
remove the const_cast from IsCore() and call GetShortName() directly; ensure all
signatures match exactly so the code compiles and run unit tests to verify no
regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 801bef8d-f0b7-4a6e-8cd9-1742ba7bb664
📒 Files selected for processing (3)
CMakeLists.txtcmake/FeatureVersions.h.insrc/Feature.h
|
✅ A pre-release build is available for this PR: |
|
We need to reevaluate how we do this. The 2 level check was stupid. However, I don't know if the file system check is the right way to drive this. Does CS need to know something is core within game? If so, it should be a cpp mechanism. I'm thinking we may want to replace the current core system with an enum for feature level:
Then we can just runtime disable whatever is left instead of this bastardized cmake distribution system based on a file existing that may or may not be included. |
|
Per Claude:
Nothing mission critical needs
I'd imagine the definition of a core feature is one that is distributed with the based Community Shaders install. So the CORE file, which is the one that determines that, would be the source of truth. So the file must be included where required anyway if a developer wants to mark something as core. EDIT: I don't think we can safely remove the 2 level approach without removing functionality. Either remove cpp references so CORE file is the only system or remove CORE files but lose the ability to bundle core features into the zip. This PR makes it so CORE files are the source of truth, which in a sense they are since they drive the build zip. The alternative is to make the cpp the source of truth. But either way, we need some cmake "bastardization" to keep the two systems in sync. |
|
Or alternatively I make no architectural changes and simply just add the 4 overrides. |
|
Sorry, I realize you may think I'm trying to be contrarian. I'm not. We need to think about what the goal of the system is instead of applying another band aid, ensuring we've designed an appropriate system that handles where the project is going. I think the multiple features being separately updated is fragile and problematic. It also introduces N!/2 complexity for testing/debugging. If we move to a enum system for each type, we can then ship everything together and let the runtime disable based on use case instead of having to use cmake to split it apart. |
|
I guess to summarize is that you think CS should shift toward the AIO approach. So to relate back to my previous response, yes, it does seem like your opinion is to get rid of the zip splitting altogether. Even though I agree with you that the AIO is the correct direction given how intertwined features are nowadays (Linear Lighting, HDR, etc), I think the choice warrants some discussion with the other maintainers. I'll simplify this PR to just add the overrides for the 4 features missing it. |
|
By no means am I trying to push the work on the other maintainers (or even you), this just seems like a big change not just to the code but to the project as a whole. Though ultimately I am trying to solve a problem here. A user reported that Grass Collision needed an update when they updated to CS 1.5, but if Grass Collision was correctly marked as core, it would have the clearer error messaging I previously added. So I still think some sort of PR needs to be made to fix that. |
alandtse
changed the title
|
Ok fine. Bandaid is fine. I don't really care. |
2 participants
Right now, core features need two things:
IsCoreThe latter was forgotten for the 4 features that are now core as of 1.5. This PR makes it so
IsCoreno longer needs to be manually overridden if the CORE file is already set. If for some reason a developer wants to makeIsCorereturn true without packaging it in core, then they can still override (I cannot ever foresee a developer wanting the reverse).Testing
Tested locally with core build + Grass Collision 3.0.2 install from Nexus.
Summary by CodeRabbit