feat: add raindrop ripples on water#577
Conversation
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
What's the source of the dds file?
It is just a blank 32x32 image that I created. It overwrites the one vanilla uses to show the default rain ripple effect. |
|
I feel that this needs more work |
WalkthroughThe wetness effects shader function Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WetnessEffectsFeature
participant WaterSystem
User->>WetnessEffectsFeature: Toggle EnableVanillaRipples checkbox
WetnessEffectsFeature->>WetnessEffectsFeature: UpdateSettings() caches ripple states
WetnessEffectsFeature->>WaterSystem: ToggleWaterSplashes thunk intercepts splash toggling
WaterSystem-->>WetnessEffectsFeature: Splash toggle modified based on cached settings
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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
Documentation and Community
|
…ders into pr/TheRiverwoodModder/577
|
Ok current status:
Not ready to merge yet, but much closer. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package/Shaders/Water.hlslis excluded by!**/*.hlsl
📒 Files selected for processing (3)
package/Shaders/Common/SharedData.hlsli(1 hunks)src/Features/WetnessEffects.cpp(5 hunks)src/Features/WetnessEffects.h(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (6)
src/Features/WetnessEffects.cpp (6)
33-74: Well-implemented runtime hooking with proper attribution.The Ripples namespace implementation correctly:
- Caches settings to avoid performance overhead
- Properly intercepts water splash toggling through thunk hooking
- Includes appropriate attribution to the original MIT-licensed code
76-86: Excellent mod compatibility handling.The implementation properly detects the "Splashes of Storms" mod and conditionally installs hooks to avoid conflicts. The logging provides clear feedback about the compatibility state.
91-93: Proper cache synchronization on settings change.Good practice to update the cached values immediately when settings change.
119-131: Excellent UI implementation with clear user feedback.The implementation provides great user experience by:
- Disabling the control when managed by external mod
- Showing descriptive label explaining why it's disabled
- Maintaining proper cache synchronization
349-349: Correct cache synchronization after loading settings.
362-376: Well-designed unloaded feature UI.The UI clearly communicates that the feature is not installed and provides a good overview of what it offers.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
|
✅ A pre-release build is available for this PR: |
3 similar comments
|
✅ A pre-release build is available for this PR: |
|
✅ A pre-release build is available for this PR: |
|
✅ A pre-release build is available for this PR: |
|
@doodlum Ok, confirmed vanilla rain ripples can be disabled by the setting. Enabling is like instant and depends on the water. Disabling may take a change of location/weather to see. I also tweaked the lifetime. The .15 looked really unrealistic to me, but .5 secs seems better. May want to based it off a vanilla weather setting though. |
|
✅ A pre-release build is available for this PR: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli (2)
112-118: Deterministic size randomisation is biased
sizeRandom = frac(dot(floatHash.xy, float2(12.9898, 78.233)));
floatHash.xyare already high-quality random values. A linear-dot/frac pair collapses the 2-D entropy to 1-D and can introduce visible banding on large water bodies.Use the existing
Random::pcgutilities (or hash the timestep) to keep full 32-bit entropy:uint sizeHash = Random::iqint3(hash.xy); float sizeRandom = (float)sizeHash * uintToFloat;
178-258: Debug helpers: markinlineto avoid code bloatThese helpers are great, but each call currently emits a standalone function body.
Prefix them withinline(orstatic inline) to ensure they are inlined and discarded
whenDEBUG_WETNESS_EFFECTSis undefined.-float2 GetDebugEffectIntensities(...) +inline float2 GetDebugEffectIntensities(...) -float3 GetDebugWetnessColor(...) +inline float3 GetDebugWetnessColor(...) ...
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package/Shaders/Water.hlslis excluded by!**/*.hlsl
📒 Files selected for processing (2)
features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli(4 hunks)src/Features/WetnessEffects.h(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Features/WetnessEffects.h
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (3)
features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli (3)
146-156: Guard change looks correctAdding
!defined(WATER)prevents the dynamic cubemap path from firing for water
passes, reducing GPU cost and eliminating self-reflection artefacts.
Good catch.
59-61: ```shell
#!/bin/bash
set -eLocate the HLSL file
fd WetnessEffects.hlsli
Show context around worldPos usage
echo "=== worldPos usage ==="
rg -n 'worldPos' -C 5 "features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli"Show context around ripple logic
echo "=== ripple usage ==="
rg -n 'ripple' -C 5 "features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli"Show context around splash logic
echo "=== splash usage ==="
rg -n 'splash' -C 5 "features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli"--- `274-291`: ```shell #!/bin/bash rg -n "GetFlowAwareRippleOffset" . rg -n "reflectionTimingScale" .
|
I'm going to look at whether we can extend ripples/splashes longer on water since you're right they don't last long enough and can seem weird. |
doodlum
left a comment
doodlum
left a comment
There was a problem hiding this comment.
Approved! Flow I think has some grid alignment issues but its barely noticeable
* feat: add raindrop ripples on water * style: 🎨 apply clang-format changes * feat: ripple effect now supports water parallax * style: 🎨 apply clang-format changes * fix: fix compilation errors * fix: fix wrong type Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore: sync cache on defaults Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. * fix: vanilla ripple display * refactor: add flowmap functions * feat: add flow maps to ripples * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. * fix: fix ripple and splash flows * fix: remove dupe GetFeatureModLink * style: fix lint * refactor: address further ai comments * fix: avoid entropy collapse in ripple hash * style: restore function names and lines --------- Co-authored-by: TheRiverwoodModder <TheRiverwoodModder@users.noreply.github.com> Co-authored-by: Alan Tse <alandtse@users.noreply.github.com> Co-authored-by: Alan Tse <alandtse@gmail.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* feat: add raindrop ripples on water * style: 🎨 apply clang-format changes * feat: ripple effect now supports water parallax * style: 🎨 apply clang-format changes * fix: fix compilation errors * fix: fix wrong type Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore: sync cache on defaults Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. * fix: vanilla ripple display * refactor: add flowmap functions * feat: add flow maps to ripples * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. * fix: fix ripple and splash flows * fix: remove dupe GetFeatureModLink * style: fix lint * refactor: address further ai comments * fix: avoid entropy collapse in ripple hash * style: restore function names and lines --------- Co-authored-by: TheRiverwoodModder <TheRiverwoodModder@users.noreply.github.com> Co-authored-by: Alan Tse <alandtse@users.noreply.github.com> Co-authored-by: Alan Tse <alandtse@gmail.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* feat: add raindrop ripples on water * style: 🎨 apply clang-format changes * feat: ripple effect now supports water parallax * style: 🎨 apply clang-format changes * fix: fix compilation errors * fix: fix wrong type Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore: sync cache on defaults Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. * fix: vanilla ripple display * refactor: add flowmap functions * feat: add flow maps to ripples * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. * fix: fix ripple and splash flows * fix: remove dupe GetFeatureModLink * style: fix lint * refactor: address further ai comments * fix: avoid entropy collapse in ripple hash * style: restore function names and lines --------- Co-authored-by: TheRiverwoodModder <TheRiverwoodModder@users.noreply.github.com> Co-authored-by: Alan Tse <alandtse@users.noreply.github.com> Co-authored-by: Alan Tse <alandtse@gmail.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
4 participants
Added the raindrop ripple effect to water:
Summary by CodeRabbit