feat: detect obsolete shader mods with no feature INIs#2074
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR extends the feature-issue detection system to identify obsolete shader mods (specifically VanillaHDR) that contain shader files but lack corresponding feature INIs, adds a new scanning function to detect these orphaned files, and tightens deletion UI eligibility by requiring file-info presence. The scanning is integrated into the state-loading flow. Changes
Sequence DiagramsequenceDiagram
participant State as State::Load
participant FI as FeatureIssues
participant FS as Filesystem
State->>FI: ScanForOrphanedFeatureINIs()
FI->>FS: Check for orphaned INIs
FI-->>State: Return
State->>FI: ScanForObsoleteShaderMods() [NEW]
FI->>FS: Scan shaders directory
loop For each obsolete trigger path
FS-->>FI: Check for trigger file<br/>(e.g., ISHDR/VanillaHDRSettings.fxh)
alt Trigger file found
FI->>FI: Create feature issue<br/>(OBSOLETE or UNKNOWN)
end
end
FI-->>State: Return
State->>State: Log "Loading Settings Complete"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
No actionable suggestions for changed features. |
|
✅ A pre-release build is available for this PR: |
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
I don't like hardcoding this. If we're going to keep data like this, we should probably just have it as a data file so we don't have to recompile every time to change it.
|
A data file seems inconsistent. It would be strange to implement this via data file and the rest of the feature issues system (min version, obsolete features, etc.) be hard-coded in source. |
|
To be clear, I'm not against the approach. But it seems like we should at least be consistent, so either the feature issues system should be driven by data files or should remain hard-coded. Similar hard-coded patterns exist, such as incompatible DLL detection. |
|
They're different. Features are actually cpp and part of this repo and must be registered in cpp to even interact with the dll. It's intended to allow cross linking between features that cause constraints, etc. Having a hardcoded list of unrelated third-party products in cpp has always been a bad pattern in this repo. There's been a lot of decisions that have resulted in tech debt. I don't believe we should've hard coded a crash logger requirement for example (despite the fact it's mine). If you strongly feel like we should overload an internal feature constraints system with random mods and you're willing to maintain it, then ok. |
|
I don't feel strongly about it. I can look into using a JSON file instead for third-party incompatibilities. Is it fine that I keep using the feature issues UI for this right? As long as I separate concerns from the internal feature constraints system? e.g., instead of using the |
|
I guess I see your point we have some infrastructure built up. Perhaps we should consider an architecture where compatibility issues are a parent class and feature issues a subclass? Maybe see what AI thinks is the better approach. The class of compatibility issues could expand indefinitely while the feature issues should be constrained. |
|
Ran through Opus. It argues that conceptually it makes sense but implementation-wise there's not much overlap, other than the UI being similar. I bothered it a bit to see if there was any bias on my end (from the chat and the existing branch) and it seemed to be pretty consistent on preferring it remain separate based on your feedback. So the approach I'm going to move forward with is implementing a completely separate |
|
If you think your time is spent better on other fixes, then do that. If a quick fix here is warranted, then we could do this with a follow-up for a real system after. I know pre-commit was forcing the upscaling.ini, but if you rebase it should stop forcing it and you can also just delete your changes. There was a bug in one of the scripts that impacted this. |
brucenguyen
force-pushed
the
detect-vanilla-hdr
branch
from
eb2b4dc to
d5a8fe7
Compare
|
No rush in my opinion. If a new user reports a error due to Vanilla HDR, the community seems pretty active on catching it and flagging, if not I will. So I'm totally okay with keeping this PR open until we're happy on the approach. At the end of the day, I think the critical issues with CS are the ones that stop people from using it, and this is unlikely one of them. |
|
Looks like Vanilla HDR was hidden on the Nexus, so that sort of solves the issue that this change was trying to solve. If you think the infra for a general compatibility system is worth it then feel free to get the rabbit to make an issue. Alternatively it could create an issue to refactor the current DLL compatibility system instead. |
2 participants
Open to feedback on the infrastructure or UX.
The purpose of this change is to detect Vanilla HDR, which is a mod that includes shader files but does not include a feature INI file. This causes shader compilation failures.
The idea behind this implementation is that a developer can easily add a new entry in
s_obsoleteShaderModTriggerPathswith a trigger path. This hypothetically can expand outside of theShadersdirectory and detect all sorts of mods.Summary by CodeRabbit
New Features
Bug Fixes