fix(weather-editor): write VR precipitation settings through VR runtime#2208
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refactors Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Utils/Game.h (1)
36-49: Optional: harmonize VR-detection between sibling macros.The new
GET_SHADER_PARTICLE_SETTINGusesglobals::game::isVR(a cached bool init'd inGlobals.cpp::OnPostLoad), while the adjacentGET_INSTANCE_MEMBER/GET_INSTANCE_MEMBER_PTRstill callREL::Module::IsVR(). They are semantically equivalent but the call-style differs across three closely-grouped macros in the same header, which is mildly confusing and means a singleApplyChanges(inPrecipitationWidget.cpp) ends up using both styles in the same function body.Consider switching all three macros to
globals::game::isVRfor consistency (and a marginally cheaper check) in a follow-up, or keep this as-is if you'd rather leave the existing macros untouched in this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Utils/Game.h` around lines 36 - 49, The macros in this header mix VR-detection styles: GET_SHADER_PARTICLE_SETTING uses the cached globals::game::isVR while the sibling macros GET_INSTANCE_MEMBER and GET_INSTANCE_MEMBER_PTR call REL::Module::IsVR(), which is inconsistent; to harmonize, change GET_INSTANCE_MEMBER and GET_INSTANCE_MEMBER_PTR to use globals::game::isVR (or alternatively switch GET_SHADER_PARTICLE_SETTING to REL::Module::IsVR() if you prefer keeping existing macros unchanged) so all three macros use the same VR check; update the conditional expressions in GET_INSTANCE_MEMBER and GET_INSTANCE_MEMBER_PTR to reference globals::game::isVR and ensure the semantics (flat vs VR storage selection) remain identical after the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Utils/Game.h`:
- Around line 36-49: The macros in this header mix VR-detection styles:
GET_SHADER_PARTICLE_SETTING uses the cached globals::game::isVR while the
sibling macros GET_INSTANCE_MEMBER and GET_INSTANCE_MEMBER_PTR call
REL::Module::IsVR(), which is inconsistent; to harmonize, change
GET_INSTANCE_MEMBER and GET_INSTANCE_MEMBER_PTR to use globals::game::isVR (or
alternatively switch GET_SHADER_PARTICLE_SETTING to REL::Module::IsVR() if you
prefer keeping existing macros unchanged) so all three macros use the same VR
check; update the conditional expressions in GET_INSTANCE_MEMBER and
GET_INSTANCE_MEMBER_PTR to reference globals::game::isVR and ensure the
semantics (flat vs VR storage selection) remain identical after the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e3391d2c-e2c3-4730-ae86-0caf44a036d7
📒 Files selected for processing (2)
src/Utils/Game.hsrc/WeatherEditor/Weather/PrecipitationWidget.cpp
|
addressed rabbits nitpick in [0cdfde6]. |
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
I meant we can use GET_INSTANCE_MEMBER to expose the value and then access it's index. No need for a PARTICLE only version.
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
Add GetSettingRef(DataID) to BGSShaderParticleGeometryData as a writable companion to GetSettingValue(DataID), mirroring its VR-aware dispatch. This removes the need for the particle-only GET_SHADER_PARTICLE_SETTING macro. Revert GET_INSTANCE_MEMBER/GET_INSTANCE_MEMBER_PTR to REL::Module::IsVR() and drop the globals::game::isVR forward declaration from Game.h. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ingRef" This reverts commit 6cf2719.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Utils/Game.h (1)
48-49: Optional: deduplicate the cast and parenthesize for defensive macro hygiene.Minor nit —
static_cast<uint32_t>(a_id)is repeated in both branches. You can lift it into a single subexpression and parenthesize the macro args/result for safety against unusual call sites. Non-blocking.♻️ Optional tweak
`#define` GET_SHADER_PARTICLE_SETTING(a_value, a_source, a_id) \ - auto& a_value = !globals::game::isVR ? (a_source)->GetRuntimeData().data[static_cast<uint32_t>(a_id)] : (a_source)->GetVRRuntimeData().data[static_cast<uint32_t>(a_id)].value; + const auto a_value##_idx = static_cast<uint32_t>(a_id); \ + auto& a_value = !globals::game::isVR ? (a_source)->GetRuntimeData().data[a_value##_idx] : (a_source)->GetVRRuntimeData().data[a_value##_idx].value;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Utils/Game.h` around lines 48 - 49, The macro GET_SHADER_PARTICLE_SETTING should lift the duplicated cast into a single parenthesized subexpression and add defensive parentheses to macro args/usage: introduce a private local identifier (e.g., const uint32_t __cast_id = static_cast<uint32_t>((a_id))) inside the macro, and then use ((a_source)->GetRuntimeData().data[__cast_id]) and ((a_source)->GetVRRuntimeData().data[__cast_id]). Also parenthesize the macro parameters and the declared variable name (e.g., auto& (a_value) = ...) to avoid surprises from complex call sites; keep the globals::game::isVR check as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Utils/Game.h`:
- Around line 48-49: The macro GET_SHADER_PARTICLE_SETTING should lift the
duplicated cast into a single parenthesized subexpression and add defensive
parentheses to macro args/usage: introduce a private local identifier (e.g.,
const uint32_t __cast_id = static_cast<uint32_t>((a_id))) inside the macro, and
then use ((a_source)->GetRuntimeData().data[__cast_id]) and
((a_source)->GetVRRuntimeData().data[__cast_id]). Also parenthesize the macro
parameters and the declared variable name (e.g., auto& (a_value) = ...) to avoid
surprises from complex call sites; keep the globals::game::isVR check as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fcf22e04-69f1-4023-a63f-77188bda9d7a
📒 Files selected for processing (2)
src/Utils/Game.hsrc/WeatherEditor/Weather/PrecipitationWidget.cpp
|
thx for clarification and implemention -should be then concluded now. |
Replace GET_SHADER_PARTICLE_SETTING macro with GetSettingRef() from CommonLib, which handles the flat/VR layout difference internally. Remove the particle-specific macro and the globals::game::isVR forward declaration. Revert GET_INSTANCE_MEMBER/GET_INSTANCE_MEMBER_PTR to REL::Module::IsVR(). Bump CommonLib to 4.15.0. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…me (community-shaders#2208) (cherry picked from commit 367375f)
3 participants
Issue:
Fixes giant snow flakes/ice crystals appearing in VR when Weather Editor is active in VR.
Reason:
BGSShaderParticleGeometryData uses different particle setting layouts in flat and VR. In flat, each entry is 4 bytes while in VR each entry is 8 bytes. The editor reads these values with GetSettingValue(), which already handles both layouts correctly. But PrecipitationWidget.cpp wrote changes back through GetRuntimeData().data[...], which is the flat 4-byte path. In VR this can corrupt precipitation values like particle size or density.
Fix:
Precipitation settings now write through VR-aware helper. In VR it writes GetVRRuntimeData().data[i].value; in flat it keeps using GetRuntimeData().data[i]. Particle texture access is also VR-aware now.
Pics (winterhold in front of frozen heath) - weather as indicated in pics
RC3:
this PR:
Summary by CodeRabbit