fix(grass collision): ignore hkpListShape#1661
Conversation
📝 WalkthroughWalkthroughAn additional validation guard is added to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
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: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Utils/ActorUtils.cpp
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Utils/ActorUtils.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Utils/ActorUtils.cpp
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
src/Utils/ActorUtils.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build plugin and addons
| RE::bhkRigidBody* bhkRigid = collisionObj->body.get() ? collisionObj->body.get()->AsBhkRigidBody() : nullptr; | ||
| RE::hkpRigidBody* hkpRigid = bhkRigid ? skyrim_cast<RE::hkpRigidBody*>(bhkRigid->referencedObject.get()) : nullptr; | ||
| if (bhkRigid && hkpRigid) { | ||
| if (bhkRigid && hkpRigid && !skyrim_cast<RE::hkpListShape*>(hkpRigid)) { // Ignore hkpListShape, unsupported |
There was a problem hiding this comment.
Test the change and consider adding logging.
The logic correctly skips hkpListShape by checking that the cast returns nullptr. However:
-
Testing required: The PR is marked as untested. Collision handling changes need verification to ensure objects with
hkpListShapedon't lose collision entirely or cause other regressions. -
Observability gap: Consider logging when shapes are skipped to aid debugging. Users or developers troubleshooting collision issues won't know if
hkpListShapewas encountered and ignored.
📋 Suggested improvement: Add logging
- if (bhkRigid && hkpRigid && !skyrim_cast<RE::hkpListShape*>(hkpRigid)) { // Ignore hkpListShape, unsupported
+ auto* listShape = skyrim_cast<RE::hkpListShape*>(hkpRigid);
+ if (listShape) {
+ // hkpListShape is unsupported; proper parsing of child shapes not implemented
+ logger::debug("Skipping unsupported hkpListShape in collision bounds calculation");
+ return false;
+ }
+ if (bhkRigid && hkpRigid) {Please verify this change doesn't cause collisions to fail for actors or objects that commonly use hkpListShape:
#!/bin/bash
# Search for any existing hkpListShape handling or references in the codebase
rg -n -C3 'hkpListShape' --type=cpp#!/bin/bash
# Check if there are tests for GetShapeBound or collision handling
fd -e cpp -e h test | xargs rg -l 'GetShapeBound|ActorUtils' |
✅ A pre-release build is available for this PR: |
|
can't reasonably merge until tested |
* build: bump eastl to 3.27.01 (community-shaders#1673) Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * fix(grass collision): ignore hkpListShape (community-shaders#1661) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * chore(upscaling): update SDKs (community-shaders#1684) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * chore(terrain shadows): modernize (community-shaders#1678) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * feat: add user wiki link to readme (community-shaders#1689) * feat(LLF): add debug overlay in llf visualiser (community-shaders#1666) * feat: linear lighting (community-shaders#1359) Co-authored-by: jiayev <jiayev@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * feat: weather and imagespace editor (community-shaders#1630) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix(ui): load previously selected theme on startup (community-shaders#1664) --------- Co-authored-by: Alan Tse <alandtse@users.noreply.github.com> Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> Co-authored-by: doodlum <15017472+doodlum@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: jiayev <l936249247@hotmail.com> Co-authored-by: jiayev <jiayev@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Ignore hkpListShape like found in https://www.nexusmods.com/skyrimspecialedition/mods/69701 to fix bad collisions.
It would be better for someone to add support for parsing this type of list, this is a quick fix.
Untested.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.