chore: add nexusmod links#1137
Conversation
WalkthroughA new virtual inline method, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FeatureStruct
Client->>FeatureStruct: GetFeatureModLink()
FeatureStruct-->>Client: Return Nexus Mods URL as string
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ 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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
Adds Nexus Mods links for each feature by introducing a GetFeatureModLink override in multiple feature headers.
- Introduces
GetFeatureModLink()in eachFeaturesubclass to return its Nexus Mods URL. - Applies the same pattern across 12 feature header files.
- No interface or shared utility updates to support the new method.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Features/WetnessEffects.h | Added GetFeatureModLink returning the mod URL. |
| src/Features/WaterEffects.h | Added GetFeatureModLink returning the mod URL. |
| src/Features/TerrainShadows.h | Added GetFeatureModLink returning the mod URL. |
| src/Features/SubsurfaceScattering.h | Added GetFeatureModLink returning the mod URL. |
| src/Features/Skylighting.h | Added GetFeatureModLink returning the mod URL. |
| src/Features/ScreenSpaceShadows.h | Added GetFeatureModLink returning the mod URL. |
| src/Features/ScreenSpaceGI.h | Added GetFeatureModLink returning the mod URL. |
| src/Features/LightLimitFix.h | Added GetFeatureModLink returning the mod URL. |
| src/Features/GrassLighting.h | Added GetFeatureModLink returning the mod URL. |
| src/Features/GrassCollision.h | Added GetFeatureModLink returning the mod URL. |
| src/Features/CloudShadows.h | Added GetFeatureModLink returning the mod URL. |
Comments suppressed due to low confidence (2)
src/Features/WetnessEffects.h:14
- [nitpick] There are no tests verifying the returned URLs for the new
GetFeatureModLinkmethods; consider adding unit tests to ensure each feature returns the correct mod link.
virtual inline std::string GetFeatureModLink() override { return "https://www.nexusmods.com/skyrimspecialedition/mods/112739"; }
src/Features/WetnessEffects.h:14
- Ensure that the base
Featureclass declaresGetFeatureModLinkas a virtual method or provides a default implementation; without this, the overrides will cause compilation errors.
virtual inline std::string GetFeatureModLink() override { return "https://www.nexusmods.com/skyrimspecialedition/mods/112739"; }
|
✅ A pre-release build is available for this PR: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
src/Features/LightLimitFix.h (1)
16-16: Consistent addition of Nexus Mods link viaGetFeatureModLink().The new inline override cleanly surfaces the mod’s URL, matching the pattern in other feature structs.
Optional nit: since this always returns the same literal, you could return astd::string_vieworconst char*(and mark itnoexcept) to avoid allocating a newstd::stringon each call.src/Features/ScreenSpaceShadows.h (1)
13-13: Consider usingstd::string_viewfor the static mod link
GetFeatureModLink()returns a fixed C-string; switching its signature to returnstd::string_view(as you do inGetShaderDefineName()) avoids heap allocations and improves performance:- virtual inline std::string GetFeatureModLink() override { return "https://www.nexusmods.com/skyrimspecialedition/mods/93209"; } + virtual inline std::string_view GetFeatureModLink() override { return "https://www.nexusmods.com/skyrimspecialedition/mods/93209"; }src/Features/WetnessEffects.h (1)
14-14: Suggest returning a string_view for static literals
Since the URL is a compile-time constant, consider changing the return type tostd::string_viewto avoid heap allocations on each call. You would also need to#include <string_view>.- virtual inline std::string GetFeatureModLink() override { return "https://www.nexusmods.com/skyrimspecialedition/mods/112739"; } + virtual inline std::string_view GetFeatureModLink() override { return "https://www.nexusmods.com/skyrimspecialedition/mods/112739"; }src/Features/GrassCollision.h (1)
13-13: Returnstd::string_view(orconst char*) to avoid repeated heap allocations
GetFeatureModLink()always returns the same compile-time constant. Returning it as a freshstd::stringon every call forces an unnecessary allocation/copy each time the metadata is queried.
Prefer a non-owning view to the literal (consistent withGetShaderDefineName()on L14):- virtual inline std::string GetFeatureModLink() override { return "https://www.nexusmods.com/skyrimspecialedition/mods/87816"; } + virtual inline std::string_view GetFeatureModLink() override { return "https://www.nexusmods.com/skyrimspecialedition/mods/87816"; }(Adjust the base
Featureinterface accordingly if it currently returnsstd::string.)src/Features/TerrainShadows.h (1)
15-15: Consider avoiding per-call string allocations
GetFeatureModLink()returns a freshstd::stringeach invocation, triggering a heap allocation every time the method is called.
If the URL is immutable (it usually is) and this accessor is queried repeatedly (e.g. for every settings draw), switching to a non-allocating representation will save a small but completely avoidable cost.Two low-impact options:
-virtual inline std::string GetFeatureModLink() override { return "https://www.nexusmods.com/skyrimspecialedition/mods/135817"; } +virtual inline std::string_view GetFeatureModLink() override { return "https://www.nexusmods.com/skyrimspecialedition/mods/135817"; }or, if the base signature locks you to
std::string:static constexpr std::string_view kModUrl = "https://www.nexusmods.com/skyrimspecialedition/mods/135817"; virtual inline std::string GetFeatureModLink() override { return std::string(kModUrl); } // avoids re-parsing the literalEither approach removes the repeat allocation while keeping the public contract intact.
(If the base class can be updated, prefer the first variant.)src/Features/CloudShadows.h (1)
21-21: Optimize feature mod link storage
CurrentlyGetFeatureModLink()returns astd::stringby value for a fixed literal, incurring an allocation on each call. Since the URL is a compile-time constant and this method is inline, consider:
- Returning
std::string_viewinstead ofstd::string.- Defining the URL as
static constexpr std::string_view kModLinkand returning that.- Removing the redundant
inlinespecifier (methods defined in-class are implicitly inline).If you choose to stay with
std::string, at least move the literal into a static constant to avoid repeated allocations.src/Features/GrassLighting.h (1)
13-13: Add Nexus Mods link method
The newGetFeatureModLink()correctly returns the feature’s Nexus Mods URL and aligns with other metadata methods.
Consider returning astd::string_viewor using astatic constexprstring to eliminate the cost of allocating a newstd::stringon every call, if the base interface permits.src/Features/WaterEffects.h (1)
18-18: Add unit tests for the new method.
There are currently no tests coveringGetFeatureModLink(). Please add a unit test to assert the returned URL matches the expected string.Do you want me to draft a simple test case for this?
src/Features/ScreenSpaceGI.h (1)
15-15: Use std::string_view for static URLs & centralize mod‐link overrides
Since this method always returns a fixed literal, changing the return type tostd::string_viewwould avoid unnecessary heap allocations (see howGetShaderDefineName()usesstd::string_view). Also, you’re overriding the same method in every feature struct—consider moving the mapping of feature identifiers to Nexus URLs into the baseFeatureclass (or a shared registry) to DRY up these overrides.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/Features/CloudShadows.h(1 hunks)src/Features/GrassCollision.h(1 hunks)src/Features/GrassLighting.h(1 hunks)src/Features/LightLimitFix.h(1 hunks)src/Features/ScreenSpaceGI.h(1 hunks)src/Features/ScreenSpaceShadows.h(1 hunks)src/Features/Skylighting.h(1 hunks)src/Features/SubsurfaceScattering.h(1 hunks)src/Features/TerrainShadows.h(1 hunks)src/Features/WaterEffects.h(1 hunks)src/Features/WetnessEffects.h(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (4)
src/Features/WetnessEffects.h (1)
14-14: Add GetFeatureModLink to expose Nexus Mods URL
The new inline override cleanly augments the feature metadata with a direct link to the corresponding Nexus Mods page, consistent with other feature structs.src/Features/Skylighting.h (1)
15-15: AddGetFeatureModLink()override
This inline virtual method cleanly exposes the Nexus Mods URL for the Skylighting feature, in line with other feature structs.src/Features/SubsurfaceScattering.h (1)
64-64: Consistent metadata accessor—looks good.The added
GetFeatureModLink()matches the pattern of other metadata methods and provides the correct Nexus Mods URL. Please verify that the mod ID (114114) is accurate and that the baseFeatureinterface includes a corresponding pure‐virtual declaration.src/Features/WaterEffects.h (1)
18-18: Verify the Nexus Mods URL for accuracy.
Ensure that the hard-coded link
"https://www.nexusmods.com/skyrimspecialedition/mods/112762"
actually points to the intended Water Effects mod.
@coderabbitai please draft a PR that switches to string_view and also addresses this comment's suggestion to have a static base string for the common parts of the nexus link. |
alandtse
force-pushed
the
links
branch
2 times, most recently
from
f591628 to
e70bd7c
Compare
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/Feature.h (3)
7-9: Preferconstexpr std::string_viewfor the base URL, but include<string_view>explicitlyNice centralisation. However,
NEXUS_BASE_URLrelies on<string_view>yet the header only includes"Menu.h".
Explicitly including<string_view>keeps this header self-contained and avoids relying on transitive includes.+#include <string_view> #include "Menu.h"
20-24: Avoid repeated allocations insideMakeNexusModURL
std::string(NEXUS_BASE_URL) + std::string(modId)creates two temporaries and an extra allocation each invocation.
Given every feature calls this once per UI draw, consider one of:
- Store the full URL as a
static constexpr std::string_viewin each feature and simply return it; or- Reserve once:
-static std::string MakeNexusModURL(std::string_view modId) -{ - return std::string(NEXUS_BASE_URL) + std::string(modId); -} +static std::string MakeNexusModURL(std::string_view modId) +{ + std::string url; + url.reserve(NEXUS_BASE_URL.size() + modId.size()); + url.append(NEXUS_BASE_URL); + url.append(modId); + return url; +}If allocations are negligible for your workload, feel free to ignore.
20-24: Mark helper asnoexceptThe function cannot throw unless allocation fails; marking it
noexceptclarifies intent and may allow minor optimisations.-static std::string MakeNexusModURL(std::string_view modId) +static std::string MakeNexusModURL(std::string_view modId) noexceptsrc/Features/TerrainHelper.h (1)
46-46: Unnecessary trailing semicolon after function body
virtual std::string GetFeatureModLink() override { ... };compiles, but the semicolon is a stray null statement.
Removing it reduces visual noise and matches prevailing style across other features.- virtual std::string GetFeatureModLink() override { return MakeNexusModURL(MOD_ID); }; + virtual std::string GetFeatureModLink() override { return MakeNexusModURL(MOD_ID); }src/Features/HairSpecular.h (1)
31-31: Consider returning a cached string_view to avoid per-call allocationEvery call to
GetFeatureModLink()triggers a new allocation viaMakeNexusModURL.
If this method is polled frequently (e.g., every frame for tooltips), cache once:-virtual inline std::string GetFeatureModLink() override { return MakeNexusModURL(MOD_ID); } +virtual inline std::string GetFeatureModLink() override +{ + static const std::string url = MakeNexusModURL(MOD_ID); + return url; +}Minor, but aligns with earlier performance feedback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.gitignore(1 hunks)src/Feature.h(2 hunks)src/Features/CloudShadows.h(2 hunks)src/Features/GrassCollision.h(2 hunks)src/Features/GrassLighting.h(2 hunks)src/Features/HairSpecular.h(2 hunks)src/Features/LightLimitFix.h(2 hunks)src/Features/ScreenSpaceGI.h(2 hunks)src/Features/ScreenSpaceShadows.h(2 hunks)src/Features/Skylighting.h(2 hunks)src/Features/SubsurfaceScattering.h(2 hunks)src/Features/TerrainHelper.h(2 hunks)src/Features/TerrainShadows.h(2 hunks)src/Features/TerrainVariation.h(2 hunks)src/Features/WaterEffects.h(2 hunks)src/Features/WetnessEffects.h(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- src/Features/TerrainVariation.h
🚧 Files skipped from review as they are similar to previous changes (11)
- src/Features/ScreenSpaceShadows.h
- src/Features/LightLimitFix.h
- src/Features/SubsurfaceScattering.h
- src/Features/WaterEffects.h
- src/Features/Skylighting.h
- src/Features/ScreenSpaceGI.h
- src/Features/WetnessEffects.h
- src/Features/TerrainShadows.h
- src/Features/GrassLighting.h
- src/Features/GrassCollision.h
- src/Features/CloudShadows.h
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Features/TerrainHelper.h (13)
src/Features/GrassCollision.h (1)
MakeNexusModURL(17-17)src/Features/CloudShadows.h (1)
MakeNexusModURL(25-25)src/Features/SubsurfaceScattering.h (1)
MakeNexusModURL(67-67)src/Features/GrassLighting.h (1)
MakeNexusModURL(17-17)src/Features/HairSpecular.h (1)
MakeNexusModURL(31-31)src/Features/ScreenSpaceShadows.h (1)
MakeNexusModURL(17-17)src/Features/TerrainVariation.h (1)
MakeNexusModURL(17-17)src/Features/Skylighting.h (1)
MakeNexusModURL(19-19)src/Features/ScreenSpaceGI.h (1)
MakeNexusModURL(19-19)src/Features/TerrainShadows.h (1)
MakeNexusModURL(19-19)src/Features/WaterEffects.h (1)
MakeNexusModURL(21-21)src/Features/WetnessEffects.h (1)
MakeNexusModURL(17-17)src/Features/LightLimitFix.h (1)
MakeNexusModURL(19-19)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (2)
src/Features/TerrainHelper.h (1)
5-7: Good move putting the Mod ID behindconstexprEncapsulating the ID avoids magic literals and matches the new pattern.
No issues here.src/Features/HairSpecular.h (1)
5-7: Consistent use ofconstexpr std::string_viewforMOD_IDLooks good and in line with other features.
Improves string concatenation efficiency by pre-allocating memory and using append operations instead of multiple string constructors. Adds noexcept specification to indicate the method doesn't throw exceptions.
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>
Summary by CodeRabbit