fix(util): SPID form identity helpers#2119
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughReplaces legacy Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Util as Util::SpidToFormId / FormIdToSpid
participant Handler as TESDataHandler
participant Lookup as TESForm::LookupByID
Caller->>Util: SpidToFormId("0xHEX~plugin")
Util->>Util: ParseSpid -> (localFormId, pluginName)
Util->>Handler: LookupForm(localFormId, pluginName)
alt found
Handler-->>Util: TESForm*
Util-->>Caller: form->GetFormID()
else not found
Util->>Util: Reconstruct full FormID from compile indices
Util->>Lookup: LookupByID(reconstructedID)
alt lookup success
Lookup-->>Util: TESForm*
Util-->>Caller: reconstructedID
else lookup fail
Util-->>Caller: 0 (not found)
end
end
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 |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
|
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/WeatherEditor/Widget.cpp (1)
244-269:⚠️ Potential issue | 🟡 MinorPotential null pointer dereference in
GetFolderName().
GetFolderName()accessesform->GetFormType()without a null check. WhileGetSaveFilePath()may typically be called on valid widgets,HasSavedFile()isconstand could theoretically be invoked on a widget with a null form.Consider adding a guard or documenting the precondition:
🛡️ Proposed defensive fix
std::string Widget::GetFolderName() const { + if (!form) + return "Other Editor Widgets"; switch (form->GetFormType()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Widget.cpp` around lines 244 - 269, GetFolderName() dereferences form via form->GetFormType() without checking for null; add a defensive null check at the top of Widget::GetFolderName() (and optionally in GetSaveFilePath()) to handle a null form by returning the default folder string (e.g., "Other Editor Widgets") so callers like HasSavedFile() or other const callers won't dereference a null pointer; locate the check in the GetFolderName() method (and adjust GetSaveFilePath() to be resilient if you prefer) and keep the method const-correct while returning the same default value used in the current default case.
🧹 Nitpick comments (2)
src/Utils/Form.cpp (2)
38-44: Edge case:localFormId == 0treated as invalid, but warning logged for empty plugin too.The condition at line 41 logs a warning and returns 0 if either
pluginName.empty()ORlocalFormId == 0. While FormID 0 is typically the null form, the warning message could be misleading if only the plugin name is empty (which is the more likely failure case from a SPID without~).This is minor since both cases are effectively invalid SPIDs, but consider separating the conditions for clearer diagnostics if needed later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Utils/Form.cpp` around lines 38 - 44, In Util::SpidToFormId, the combined check using ParseSpid(…) for components.pluginName.empty() || components.localFormId == 0 produces a single misleading warning; split the validation into two distinct checks so you log one warning when pluginName is empty (indicating a malformed SPID) and a separate (or no) warning/handling when localFormId == 0 (the null FormID case), then return 0 after the appropriate check so diagnostics reference ParseSpid, components.pluginName and components.localFormId clearly.
95-115: Consider caching or avoiding repeated calls toGetFormEditorIDwith map fallback.The fallback path at lines 104-112 performs a linear scan of the entire EditorID map under a read lock. While this is thread-safe, it has O(n) complexity and could become a performance concern if
GetFormEditorIDis called frequently (e.g., during widget list rendering or transitions).The current usage in
CacheFormData()caches the result, which mitigates this. Ensure that callers avoid repeated lookups for the same form.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Utils/Form.cpp` around lines 95 - 115, GetFormEditorID currently falls back to a linear scan of the global EditorID map (using RE::TESForm::GetAllFormsByEditorID()) which is O(n) and costly if called frequently; update callers to avoid repeated lookups by caching results and/or add an internal cache inside Util (e.g., a thread-safe unordered_map keyed by form pointer or form ID) and have Util::GetFormEditorID check that cache before doing the expensive map scan; ensure the cache is updated/invalidated where forms change (reference CacheFormData or the function(s) that mutate form data) and use RE::BSReadLockGuard only around the fallback scan as currently implemented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/WeatherEditor/Widget.h`:
- Around line 46-53: GetSaveFilePath is using cachedSaveKey (returned by
GetSaveKey) to build filenames but the SPID-formatted key can contain characters
like '~' that should be sanitized; fix this by applying
FileSystem::SanitizeFileName to the SPID before it is used in paths—either
sanitize when caching the value stored in cachedSaveKey (where the SPID is
computed) or call SanitizeFileName inside GetSaveFilePath before composing the
file path so the returned path never contains unsafe characters; update
references to cachedSaveKey/GetSaveKey accordingly and ensure you include the
SanitizeFileName symbol from FileSystem.h.
---
Outside diff comments:
In `@src/WeatherEditor/Widget.cpp`:
- Around line 244-269: GetFolderName() dereferences form via form->GetFormType()
without checking for null; add a defensive null check at the top of
Widget::GetFolderName() (and optionally in GetSaveFilePath()) to handle a null
form by returning the default folder string (e.g., "Other Editor Widgets") so
callers like HasSavedFile() or other const callers won't dereference a null
pointer; locate the check in the GetFolderName() method (and adjust
GetSaveFilePath() to be resilient if you prefer) and keep the method
const-correct while returning the same default value used in the current default
case.
---
Nitpick comments:
In `@src/Utils/Form.cpp`:
- Around line 38-44: In Util::SpidToFormId, the combined check using
ParseSpid(…) for components.pluginName.empty() || components.localFormId == 0
produces a single misleading warning; split the validation into two distinct
checks so you log one warning when pluginName is empty (indicating a malformed
SPID) and a separate (or no) warning/handling when localFormId == 0 (the null
FormID case), then return 0 after the appropriate check so diagnostics reference
ParseSpid, components.pluginName and components.localFormId clearly.
- Around line 95-115: GetFormEditorID currently falls back to a linear scan of
the global EditorID map (using RE::TESForm::GetAllFormsByEditorID()) which is
O(n) and costly if called frequently; update callers to avoid repeated lookups
by caching results and/or add an internal cache inside Util (e.g., a thread-safe
unordered_map keyed by form pointer or form ID) and have Util::GetFormEditorID
check that cache before doing the expensive map scan; ensure the cache is
updated/invalidated where forms change (reference CacheFormData or the
function(s) that mutate form data) and use RE::BSReadLockGuard only around the
fallback scan as currently implemented.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 16b34bc2-cc46-4bec-b9bc-813125fb3961
📒 Files selected for processing (5)
src/Utils/Form.cppsrc/Utils/Form.hsrc/WeatherEditor/Widget.cppsrc/WeatherEditor/Widget.hsrc/WeatherManager.cpp
|
✅ A pre-release build is available for this PR: |
|
Does this break any configs we've previously released? Or is this only impacting dev builds? If this breaks old saves, this is a breaking change and will increment us to 2.0.0. |
This is a breaking change. It was largely advised throughout the community to not use the weather editor until the next update because of its unfinished state. The current implementation is not a proper implementation. |
Dlizzio
changed the title
|
you can bump the weather editor to 2.0.0 just fine |
SkrubbySkrubInAShrub
left a comment
SkrubbySkrubInAShrub
left a comment
There was a problem hiding this comment.
Added some comments, code works fine, but here are some plausible edgecases. verify if this is not slop, some of this is what the AI called out so be skeptical.
|
2.0.0 would be the CS version bump. Not the feature |
|
The major version tells downloaders if they have to manually change anything. If you're telling me the change to spid IDs will break things for prior users, it's a major CS bump. |
alandtse
changed the title
alandtse
changed the title
|
Can we not try to make a system that upgrades the files? |
|
i understand the semver perspective and importance to adhere to it, but if people see CS 2.0 and its essentially a nothing burger update just bug fixing, people are gonna crack it. especially when as far as im aware there are no mods currently shipping weather editor jsons instead of esps. this is really more of an architectural fix. |
|
Version numbers don't mean anything. We could bump to 100.0.0 if we really wanted. We really shouldn't care what the random numbers mean. But it's a signal to people on whether something is breaking on their load order. We broke people going from 0.x to 1.x. If you all really strongly believe we should break weather editing without telling people then, I can accept that. I never understood why we embedded an editor in a shader project so I frankly don't really care about that audience anyway. |
|
I think for this specific circumstance it is acceptable. |
alandtse
changed the title
…-identity-dev # Conflicts: # features/Weather Editor/Shaders/Features/WeatherEditor.ini
This reverts commit 24c22be.
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> (cherry picked from commit 24c22be)
5 participants
Adds shared SPID utilities for load order portable form identification. Replaces the old pipe delimited format. Weather editor widgets and weather manager now save and load using SPID keys instead of EditorIDs or raw FormIDs.
Summary by CodeRabbit
Refactor
Bug Fixes
Chore