fix(grass collision): validate actor

[Dawntic]

Adds smart pointers and 3D loaded checks to grass coll. Fixes #1746

Summary by CodeRabbit

• Refactor
  • Improved grass collision handling for greater stability and reliability.
  • Reduced risk of crashes and glitches when interacting with grass and nearby actors.
  • Smoother collision behavior and modest performance improvements during actor updates.

[github-actions]

Using provided base ref: 27eb9e9
Using base ref: 27eb9e9
Base commit date: 2026-02-18T09:49:12-08:00 (Wednesday, February 18, 2026 09:49 AM)
No actionable suggestions for changed features.

[github-actions]

✅ A pre-release build is available for this PR:
Download

[coderabbitai]

📝 Walkthrough

Walkthrough

Replaced raw RE::Actor* usage with RE::ActorPtr in GrassCollision actor collection and processing; only push non-null, 3D-loaded actor pointers, insert player via its handle, updated sorting comparator and iteration to use RE::ActorPtr. (34 words)

Changes

Cohort / File(s)	Summary
Actor Pointer Type Safety src/Features/GrassCollision.cpp	Changed actorList from eastl::vector<RE::Actor*> to eastl::vector<RE::ActorPtr>. When collecting actors, push only non-null ActorPtr that pass Is3DLoaded(); player insertion uses player->GetHandle().get(); sort comparator and loops updated to operate on RE::ActorPtr and iterate by const auto&.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• fix(grass collision): catch trashed actor pointers #1765: Also modifies src/Features/GrassCollision.cpp actor-pointer handling (changes uses of get() to getsafe()), closely related to pointer-safety adjustments here.

Suggested reviewers

• alandtse
• davo0411
• doodlum

Poem

🐰
I hopped through code with careful paws,
Wrapped actors safe from dangling jaws.
Handles snug and checks in place,
No more crashes in that grassy space.
🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(grass collision): validate actor' clearly describes the main change: adding validation to actor handling in the grass collision feature to fix the CTD issue.
Linked Issues check	✅ Passed	The PR successfully addresses issue #1746 by replacing raw actor pointers with smart pointers (RE::ActorPtr), adding 3D loaded checks, and validating actors before use to prevent dereferencing invalid pointers.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the GrassCollision crash: smart pointer conversion, actor validation, and comparator updates. No unrelated modifications are present.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (3)

src/Features/GrassCollision.cpp (3)

48-52: Sort comparator takes ActorPtr by value — unnecessary ref-count overhead

Each comparison call copies two BSTSmartPointer instances, incrementing and decrementing the reference count O(N log N) times. Use const RE::ActorPtr& instead.

While fixing the player insertion (see comment above) eliminates null entries, adding a null guard in the comparator is cheap defensive coding that prevents the original crash class from reoccurring if any future code path pushes a null entry.

♻️ Proposed fix

-	std::sort(actorList.begin(), actorList.end(), [&cameraPosition](RE::ActorPtr a, RE::ActorPtr b) {
-		float distA = cameraPosition.GetSquaredDistance(a->GetPosition());
-		float distB = cameraPosition.GetSquaredDistance(b->GetPosition());
+	std::sort(actorList.begin(), actorList.end(), [&cameraPosition](const RE::ActorPtr& a, const RE::ActorPtr& b) {
+		float distA = a ? cameraPosition.GetSquaredDistance(a->GetPosition()) : FLT_MAX;
+		float distB = b ? cameraPosition.GetSquaredDistance(b->GetPosition()) : FLT_MAX;
		return distA < distB;
	});

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Features/GrassCollision.cpp` around lines 48 - 52, The sort comparator
for actorList currently takes RE::ActorPtr by value causing excessive
ref-counting and lacks null guards; change the lambda signature to take
parameters as const RE::ActorPtr& (e.g., [&cameraPosition](const RE::ActorPtr&
a, const RE::ActorPtr& b) { ... }) and add cheap defensive null checks (if a or
b is null, define a stable ordering, e.g., treat null as farther) before calling
GetPosition/ GetSquaredDistance so the comparator never dereferences a null
pointer.

---

35-35: Redundant double null check on actorPtr

RE::BSTSmartPointer::operator bool() checks _ptr != nullptr, and get() returns that same _ptr. So actorPtr && actorPtr.get() tests the same condition twice; the second check adds no safety.

♻️ Proposed simplification

-				if (actorPtr && actorPtr.get() && actorPtr.get()->Is3DLoaded()) {
+				if (actorPtr && actorPtr->Is3DLoaded()) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Features/GrassCollision.cpp` at line 35, The condition performs a
redundant null check by testing both actorPtr (which uses
RE::BSTSmartPointer::operator bool()) and actorPtr.get(); remove the duplicate
by keeping a single pointer truthiness check and then call Is3DLoaded() via the
smart pointer; replace the if condition `if (actorPtr && actorPtr.get() &&
actorPtr.get()->Is3DLoaded())` with a single check such as `if (actorPtr &&
actorPtr->Is3DLoaded())` (or `if (actorPtr && actorPtr.get()->Is3DLoaded())`) to
avoid duplicated null testing on actorPtr while preserving the Is3DLoaded()
call.

---

1-1: Add a closing keyword for issue #1746 to the PR description

The PR description references issue #1746 but does not include a GitHub auto-close keyword. Per project convention, add one of the following to the PR body so the issue is automatically closed on merge:

Fixes `#1746`

As per coding guidelines: "Suggest adding appropriate GitHub keywords: 'Fixes #123' or 'Closes #123' for bug fixes."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Features/GrassCollision.cpp` at line 1, The PR description currently
references issue `#1746` but lacks a GitHub auto-close keyword; update the PR body
to include a closing keyword such as "Fixes `#1746`" (or "Closes `#1746`") so the
issue will be closed automatically when this PR is merged—edit the PR
description field and add that exact phrase to the top or bottom of the PR body.

🤖 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/Features/GrassCollision.cpp`:
- Around line 42-43: The player singleton is called twice and a null ActorPtr
from GetHandle().get() can be pushed into actorList and later dereferenced in
the sort comparator; fix by reusing the local player variable (from auto player
= RE::PlayerCharacter::GetSingleton()), check player->Is3DLoaded() like the NPC
path, retrieve the handle once into a local auto actor =
player->GetHandle().get(), null-check actor before pushing into actorList, and
only push if actor is non-null; this prevents null pointers reaching the
std::sort comparator.

---

Nitpick comments:
In `@src/Features/GrassCollision.cpp`:
- Around line 48-52: The sort comparator for actorList currently takes
RE::ActorPtr by value causing excessive ref-counting and lacks null guards;
change the lambda signature to take parameters as const RE::ActorPtr& (e.g.,
[&cameraPosition](const RE::ActorPtr& a, const RE::ActorPtr& b) { ... }) and add
cheap defensive null checks (if a or b is null, define a stable ordering, e.g.,
treat null as farther) before calling GetPosition/ GetSquaredDistance so the
comparator never dereferences a null pointer.
- Line 35: The condition performs a redundant null check by testing both
actorPtr (which uses RE::BSTSmartPointer::operator bool()) and actorPtr.get();
remove the duplicate by keeping a single pointer truthiness check and then call
Is3DLoaded() via the smart pointer; replace the if condition `if (actorPtr &&
actorPtr.get() && actorPtr.get()->Is3DLoaded())` with a single check such as `if
(actorPtr && actorPtr->Is3DLoaded())` (or `if (actorPtr &&
actorPtr.get()->Is3DLoaded())`) to avoid duplicated null testing on actorPtr
while preserving the Is3DLoaded() call.
- Line 1: The PR description currently references issue `#1746` but lacks a GitHub
auto-close keyword; update the PR body to include a closing keyword such as
"Fixes `#1746`" (or "Closes `#1746`") so the issue will be closed automatically when
this PR is merged—edit the PR description field and add that exact phrase to the
top or bottom of the PR body.

[coderabbitai]

🧹 Nitpick comments (3)

src/Features/GrassCollision.cpp (3)

48-52: Sort comparator takes RE::ActorPtr by value — unnecessary ref-count churn

RE::ActorPtr (NiPointer<RE::Actor>) is a ref-counted smart pointer. Passing by value inside the comparator bumps and releases the ref-count on every comparison call, yielding O(N log N) unnecessary atomic operations. Use const RE::ActorPtr& instead.

♻️ Proposed fix

-	std::sort(actorList.begin(), actorList.end(), [&cameraPosition](RE::ActorPtr a, RE::ActorPtr b) {
+	std::sort(actorList.begin(), actorList.end(), [&cameraPosition](const RE::ActorPtr& a, const RE::ActorPtr& b) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Features/GrassCollision.cpp` around lines 48 - 52, The comparator for
std::sort is taking RE::ActorPtr (NiPointer<RE::Actor>) by value causing
unnecessary ref-count increments; update the lambda parameters in the actorList
sort to take const RE::ActorPtr& (e.g., [&cameraPosition](const RE::ActorPtr& a,
const RE::ActorPtr& b)) so comparisons use references and avoid atomic
bump/release overhead while leaving cameraPosition and the body logic unchanged.

---

35-36: Redundant double null-check on ActorPtr

NiPointer<T>::operator bool() already checks the underlying raw pointer, so actorPtr.get() in the compound condition is redundant — actorPtr && actorPtr.get() reduces to actorPtr.

♻️ Proposed simplification

-			if (actorPtr && actorPtr.get() && actorPtr.get()->Is3DLoaded()) {
+			if (actorPtr && actorPtr->Is3DLoaded()) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Features/GrassCollision.cpp` around lines 35 - 36, The condition is doing
a redundant double null-check on the NiPointer (actorPtr && actorPtr.get());
update the guard in the block using the pointer's existing boolean operator and
member access instead: only check actorPtr and then call Is3DLoaded via the
smart pointer (so the compound actorPtr.get() check is removed) before pushing
into actorList (affects the if that leads to actorList.push_back(actorPtr) and
uses actorPtr->Is3DLoaded()).

---

1-1: Conventional commit scope should not contain a space

The PR title fix(grass collision): validate actor uses a space in the scope. Per conventional commits convention, scopes are typically hyphenated: fix(grass-collision): validate actor. As per coding guidelines, the format should follow https://www.conventionalcommits.org/.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Features/GrassCollision.cpp` at line 1, The PR's conventional commit
scope contains a space; rename the scope from "grass collision" to a hyphenated
form "grass-collision" in the commit title (i.e., change "fix(grass collision):
validate actor" to "fix(grass-collision): validate actor"). Fix this by amending
the commit or interactive rebasing (e.g., git commit --amend or git rebase -i)
to update the commit message, then force-push the branch; ensure any related
release notes or changelog entries use the same hyphenated scope.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/Features/GrassCollision.cpp`:
- Around line 42-43: The player actor handle from player->GetHandle().get() can
be null or not 3D-loaded and must be filtered before being inserted into
actorList and before calling std::sort (the comparator dereferences actors via
a->GetPosition()); update the code that currently does
actorList.push_back(player->GetHandle().get()) to check the returned ActorPtr is
non-null and that the actor->Is3DLoaded() returns true (or otherwise resolve the
handle) before pushing into actorList so only valid, 3D-loaded actors reach the
sort comparator in GrassCollision.cpp; ensure the same null/Is3DLoaded guard is
applied for other actors inserted into actorList so the later loop check at
Is3DLoaded() is not the sole defense.

---

Nitpick comments:
In `@src/Features/GrassCollision.cpp`:
- Around line 48-52: The comparator for std::sort is taking RE::ActorPtr
(NiPointer<RE::Actor>) by value causing unnecessary ref-count increments; update
the lambda parameters in the actorList sort to take const RE::ActorPtr& (e.g.,
[&cameraPosition](const RE::ActorPtr& a, const RE::ActorPtr& b)) so comparisons
use references and avoid atomic bump/release overhead while leaving
cameraPosition and the body logic unchanged.
- Around line 35-36: The condition is doing a redundant double null-check on the
NiPointer (actorPtr && actorPtr.get()); update the guard in the block using the
pointer's existing boolean operator and member access instead: only check
actorPtr and then call Is3DLoaded via the smart pointer (so the compound
actorPtr.get() check is removed) before pushing into actorList (affects the if
that leads to actorList.push_back(actorPtr) and uses actorPtr->Is3DLoaded()).
- Line 1: The PR's conventional commit scope contains a space; rename the scope
from "grass collision" to a hyphenated form "grass-collision" in the commit
title (i.e., change "fix(grass collision): validate actor" to
"fix(grass-collision): validate actor"). Fix this by amending the commit or
interactive rebasing (e.g., git commit --amend or git rebase -i) to update the
commit message, then force-push the branch; ensure any related release notes or
changelog entries use the same hyphenated scope.

[coderabbitai]

🧹 Nitpick comments (1)

src/Features/GrassCollision.cpp (1)

48-52: Sort comparator captures RE::ActorPtr by value — prefer const RE::ActorPtr&

Each comparison invocation copies two smart pointers, incurring reference-count increments and decrements O(n log n) times. Capturing by const RE::ActorPtr& eliminates this overhead entirely.

♻️ Proposed refactor

-	std::sort(actorList.begin(), actorList.end(), [&cameraPosition](RE::ActorPtr a, RE::ActorPtr b) {
+	std::sort(actorList.begin(), actorList.end(), [&cameraPosition](const RE::ActorPtr& a, const RE::ActorPtr& b) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Features/GrassCollision.cpp` around lines 48 - 52, The lambda comparator
used in std::sort over actorList currently captures parameters by value
(RE::ActorPtr a, RE::ActorPtr b), causing needless smart-pointer copy/ref-count
churn; change the comparator signature to take const RE::ActorPtr& (e.g., const
RE::ActorPtr& a, const RE::ActorPtr& b) while still using cameraPosition inside
the lambda to avoid copies and preserve behavior in the sort call that uses
actorList and cameraPosition.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/Features/GrassCollision.cpp`:
- Around line 42-43: The player ActorPtr is pushed unconditionally and can be
null/unresolved, causing the std::sort comparator to dereference it (via
GetPosition()) and crash; update the player path in the block using
RE::PlayerCharacter::GetSingleton() so it mirrors the NPC path: obtain the
ActorPtr from player->GetHandle().get(), check that the pointer is non-null and
3D-loaded (same predicate used for NPCs, e.g., check Get3D() / Is3DLoaded or the
exact guard used near line 63) before pushing into actorList, ensuring only
valid actors reach the sort/comparator.

---

Nitpick comments:
In `@src/Features/GrassCollision.cpp`:
- Around line 48-52: The lambda comparator used in std::sort over actorList
currently captures parameters by value (RE::ActorPtr a, RE::ActorPtr b), causing
needless smart-pointer copy/ref-count churn; change the comparator signature to
take const RE::ActorPtr& (e.g., const RE::ActorPtr& a, const RE::ActorPtr& b)
while still using cameraPosition inside the lambda to avoid copies and preserve
behavior in the sort call that uses actorList and cameraPosition.