added caching for duration until recheck #195
Conversation
WalkthroughAdds time-based caching for monster visible-target lookups and target-distance computations, introduces public InvalidateDistanceCache and GetCachedDistanceToTarget, adds public GetAttackTargetsUncached and GetCacheStats, increases monster tick interval, and applies a whitespace-only formatting change in an admin command handler. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant M as Monster
participant T as Timers
participant W as World (Perception)
Note over M: GetAttackTargets() — 0.5s TTL cache
M->>T: Read RunningTime
alt Cache valid (< 0.5s)
M-->>M: Return copy of _cachedVisibleTargets (cache hit)
else Recompute
M->>W: GetAttackTargetsUncached() — scan visible creatures
W-->>M: visible target list
M-->>M: Update _cachedVisibleTargets & _lastTargetCacheTime (cache miss)
M-->>M: Return new list
end
sequenceDiagram
autonumber
participant M as Monster
participant T as Timers
participant P as Physics
Note over M: GetCachedDistanceToTarget() — 0.25s TTL
M->>T: Read RunningTime
alt Cache valid (< 0.25s && cached >= 0)
M-->>M: Return _cachedDistanceToTarget (cache hit)
else Recompute
alt AttackTarget present & physics available
M->>P: get_distance_to_object(target.PhysicsObj, true)
P-->>M: distance
M-->>M: Cache distance & time
else No target or physics missing
M-->>M: Cache float.MaxValue & time
end
M-->>M: Return cached value (cache miss)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Poem
✨ Finishing Touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (1)
242-299: Fix target-filter precedence and guard cache-read before initialization.
- The filter allows non-attackable targets unless TargetingTactic == None due to operator precedence.
- The cache can be (briefly) used before any fill at server start.
Apply:
- if (currentTime - _lastTargetCacheTime < TARGET_CACHE_DURATION) + if (_lastTargetCacheTime > 0.0 && (currentTime - _lastTargetCacheTime) < TARGET_CACHE_DURATION) { return _cachedVisibleTargets; } @@ - if (!creature.Attackable && creature.TargetingTactic == TargetingTactic.None || creature.Teleporting) continue; + if (!creature.Attackable || creature.TargetingTactic == TargetingTactic.None || creature.Teleporting) + continue;
🧹 Nitpick comments (3)
Source/ACE.Server/WorldObjects/Monster_Tick.cs (1)
10-10: Tick interval doubled to 0.4s — consider aligning hot-path distance checks with new caching.To capture the intended CPU savings, replace frequent GetDistanceToTarget() usages in this loop with the new cached variant where acceptable (see Monster_Navigation.cs). Also consider making this interval configurable via server property.
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (1)
26-32: Add explicit cache invalidation hooks.To avoid stale reads when state flips quickly, invalidate on Sleep()/AttackTarget changes:
// In Sleep(): _cachedVisibleTargets.Clear(); _lastTargetCacheTime = 0.0; // Wherever AttackTarget is reassigned (after the assignment): _lastTargetCacheTime = 0.0;Source/ACE.Server/WorldObjects/Monster_Navigation.cs (1)
25-57: Good addition; now use it in hot paths.The cache implementation looks solid (init guard present). To realize the win, switch hot-path checks to GetCachedDistanceToTarget():
StartTurn (Line 131):
- var targetDist = GetCachedDistanceToTarget();
OnMoveComplete (Line 172):
- var targetDist = GetCachedDistanceToTarget();
Movement (Line 249):
- if (MonsterState == State.Awake && GetCachedDistanceToTarget() >= MaxChaseRange)
Optionally in IsMeleeRange/IsAttackRange/IsFacing if acceptable to be ≤0.25s stale.
Also invalidate on Sleep/target change for safety:
_cachedDistanceToTarget = -1.0f; _lastDistanceCacheTime = 0.0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Source/ACE.Server/Command/Handlers/AdminCommands.cs(1 hunks)Source/ACE.Server/WorldObjects/Monster_Awareness.cs(3 hunks)Source/ACE.Server/WorldObjects/Monster_Navigation.cs(1 hunks)Source/ACE.Server/WorldObjects/Monster_Tick.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Source/ACE.Server/WorldObjects/Monster_Navigation.cs (2)
Source/ACE.Server/Entity/Timers.cs (1)
Timers(7-48)Source/ACE.Server/Physics/PhysicsObj.cs (6)
PhysicsObj(29-3251)PhysicsObj(121-159)PhysicsObj(480-483)PhysicsObj(2325-2336)PhysicsObj(2338-2344)get_distance_to_object(1945-1957)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (3)
Source/ACE.Server/Physics/Common/ObjectMaint.cs (16)
List(153-164)List(166-177)List(179-190)List(234-245)List(292-303)List(305-316)List(318-329)List(331-350)List(355-378)List(383-411)List(491-512)List(656-675)List(717-728)List(730-741)List(743-762)List(764-783)Source/ACE.Server/WorldObjects/Monster_Navigation.cs (1)
Creature(17-685)Source/ACE.Server/Entity/Timers.cs (1)
Timers(7-48)
🔇 Additional comments (1)
Source/ACE.Server/Command/Handlers/AdminCommands.cs (1)
2203-2203: Whitespace-only change — no action needed.No functional impact.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Source/ACE.Server/WorldObjects/Monster_Navigation.cs (2)
25-31: Nit: align constant naming with codebase stylePrefer PascalCase for constants here to match nearby fields (e.g., MaxChaseRange). Rename DISTANCE_CACHE_DURATION -> DistanceCacheDuration.
- private const double DISTANCE_CACHE_DURATION = 0.25; // Cache for 0.25 seconds + private const double DistanceCacheDuration = 0.25; // Cache for 0.25 secondsAlso update its usages:
- if (currentTime - _lastDistanceCacheTime < DISTANCE_CACHE_DURATION && _cachedDistanceToTarget >= 0) + if (currentTime - _lastDistanceCacheTime < DistanceCacheDuration && _cachedDistanceToTarget >= 0)
533-534: IsFacing() uses AttackTarget distance while the parameter may differIf IsFacing(target) is ever called with target != AttackTarget, widening threshold based on AttackTarget’s distance can be wrong. Consider using distance to the provided target when it differs; fallback to cache only when target == AttackTarget.
- var dist = Math.Max(0, GetCachedDistanceToTarget()); + var dist = (target == AttackTarget) + ? Math.Max(0, GetCachedDistanceToTarget()) + : (PhysicsObj != null && target?.PhysicsObj != null + ? (float)PhysicsObj.get_distance_to_object(target.PhysicsObj, true) + : float.MaxValue);Please confirm all current call sites pass AttackTarget; otherwise apply the above.
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (1)
269-275: Return a copy of the cached list to avoid external mutationGetAttackTargets() returns the internal list when cache hits. Callers mutating it will corrupt the cache. Return a defensive copy.
- if (_lastTargetCacheTime > 0.0 && (currentTime - _lastTargetCacheTime) < TARGET_CACHE_DURATION) - { - return _cachedVisibleTargets; - } + if (_lastTargetCacheTime > 0.0 && (currentTime - _lastTargetCacheTime) < TARGET_CACHE_DURATION) + { + return new List<Creature>(_cachedVisibleTargets); + } ... - return visibleTargets; + return new List<Creature>(_cachedVisibleTargets);Also applies to: 315-318, 319-319
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs(7 hunks)Source/ACE.Server/WorldObjects/Monster_Navigation.cs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Source/ACE.Server/WorldObjects/Monster_Navigation.cs (3)
Source/ACE.Server/Entity/Timers.cs (1)
Timers(7-48)Source/ACE.Server/Physics/PhysicsObj.cs (6)
PhysicsObj(29-3251)PhysicsObj(121-159)PhysicsObj(480-483)PhysicsObj(2325-2336)PhysicsObj(2338-2344)get_distance_to_object(1945-1957)Source/ACE.Server/WorldObjects/Monster_Magic.cs (1)
GetSpellMaxRange(118-128)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (3)
Source/ACE.Server/Physics/Common/ObjectMaint.cs (17)
List(153-164)List(166-177)List(179-190)List(234-245)List(292-303)List(305-316)List(318-329)List(331-350)List(355-378)List(383-411)List(491-512)List(656-675)List(717-728)List(730-741)List(743-762)List(764-783)PhysicsObj(100-112)Source/ACE.Server/WorldObjects/Monster_Navigation.cs (2)
Creature(17-694)InvalidateDistanceCache(35-39)Source/ACE.Server/Entity/Timers.cs (1)
Timers(7-48)
🔇 Additional comments (6)
Source/ACE.Server/WorldObjects/Monster_Navigation.cs (2)
32-40: InvalidateDistanceCache(): LGTMClear and side-effect free. Public surface is appropriate given cross-file calls.
139-141: Switch to cached distance: LGTMGood coverage of hot call sites; should reduce physics queries materially.
Also applies to: 181-184, 207-208, 215-216, 258-263, 556-557
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (4)
26-32: Target list caching: LGTMSimple, bounded TTL; matches PR goal.
66-70: Sleep() clears caches: LGTMCorrectly resets both target and distance caches.
190-192: Invalidate caches on target change: LGTMRight place to reset both caches; avoids cross-target contamination.
Also applies to: 202-207, 213-218, 229-231, 237-239, 245-247
265-318: Restrict ‘None’ filter to non-player creatures
The currentifskips any creature whoseTargetingTactic == None—players default toNone—so attackable players will be ignored. Change to only excludeNonefor non‐players:- if (!creature.Attackable || creature.TargetingTactic == TargetingTactic.None || creature.Teleporting) + if (!creature.Attackable || creature.Teleporting + || (creature is not Player && creature.TargetingTactic == TargetingTactic.None))Confirm this matches the intended targeting rules for players.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (3)
26-32: Confirm cache TTL (0.6s) vs PR objective (0.5s); consider making it configurable.The PR description says 0.5s for target cache; code uses 0.6s. If 0.6s is intentional (to span 1–2 ticks at 0.4s), note it in the PR. Otherwise, align to 0.5s or make this a tunable property key.
Apply if you intended 0.5s:
- private const double TARGET_CACHE_DURATION = 0.6; // Cache for 0.6 seconds + private const double TARGET_CACHE_DURATION = 0.5; // Cache for 0.5 secondsOptional (outside this hunk): expose a PropertyManager key, e.g. mob_target_cache_seconds, defaulting to 0.5.
191-193: Deduplicate target-change invalidation.Same two-line invalidation repeats across branches. Extract a helper to reduce drift and ensure future paths also invalidate.
- AttackTarget = SelectWeightedDistance(targetDistances); - _lastTargetCacheTime = 0.0; // Invalidate cache when target changes - InvalidateDistanceCache(); // Also invalidate distance cache + AttackTarget = SelectWeightedDistance(targetDistances); + OnAttackTargetChanged(); - AttackTarget = lastDamager; - _lastTargetCacheTime = 0.0; // Invalidate cache when target changes - InvalidateDistanceCache(); // Also invalidate distance cache + AttackTarget = lastDamager; + OnAttackTargetChanged(); - AttackTarget = topDamager; - _lastTargetCacheTime = 0.0; // Invalidate cache when target changes - InvalidateDistanceCache(); // Also invalidate distance cache + AttackTarget = topDamager; + OnAttackTargetChanged(); - AttackTarget = lowestLevel; - _lastTargetCacheTime = 0.0; // Invalidate cache when target changes - InvalidateDistanceCache(); // Also invalidate distance cache + AttackTarget = lowestLevel; + OnAttackTargetChanged(); - AttackTarget = highestLevel; - _lastTargetCacheTime = 0.0; // Invalidate cache when target changes - InvalidateDistanceCache(); // Also invalidate distance cache + AttackTarget = highestLevel; + OnAttackTargetChanged(); - AttackTarget = nearest[0].Target; - _lastTargetCacheTime = 0.0; // Invalidate cache when target changes - InvalidateDistanceCache(); // Also invalidate distance cache + AttackTarget = nearest[0].Target; + OnAttackTargetChanged();Add once (outside selection):
private void OnAttackTargetChanged() { _lastTargetCacheTime = 0.0; InvalidateDistanceCache(); }Also applies to: 205-207, 216-218, 230-232, 238-240, 246-248
584-589: Simplify condition; the “not player” clause is unreachable here.Players are continued at Line 580–582, so the extra player check is redundant and obscures intent.
- if (!creature.Attackable && creature.TargetingTactic == TargetingTactic.None && !(creature is Player)) + if (!creature.Attackable && creature.TargetingTactic == TargetingTactic.None) continue;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs(8 hunks)Source/ACE.Server/WorldObjects/Monster_Navigation.cs(6 hunks)Source/ACE.Server/WorldObjects/Monster_Tick.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Source/ACE.Server/WorldObjects/Monster_Tick.cs
- Source/ACE.Server/WorldObjects/Monster_Navigation.cs
🧰 Additional context used
🧬 Code graph analysis (1)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (3)
Source/ACE.Server/Physics/Common/ObjectMaint.cs (17)
List(153-164)List(166-177)List(179-190)List(234-245)List(292-303)List(305-316)List(318-329)List(331-350)List(355-378)List(383-411)List(491-512)List(656-675)List(717-728)List(730-741)List(743-762)List(764-783)PhysicsObj(100-112)Source/ACE.Server/WorldObjects/Monster_Navigation.cs (2)
Creature(17-704)InvalidateDistanceCache(35-39)Source/ACE.Server/Entity/Timers.cs (1)
Timers(7-48)
🔇 Additional comments (2)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (2)
155-157: Good call using uncached data for target selection.Critical decisions bypass the cache. This avoids stale-target edge cases.
264-287: Ensure all AttackTarget setters invalidate the target cache
I couldn’t find anyAttackTarget = …assignments in the code—please manually verify that every code path which sets or changes the monster’s attack target (retaliation, emotes, damage handlers, etc.) callsOnAttackTargetChanged()so that theGetAttackTargets()cache is always invalidated.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (2)
66-70: Prefer reassigning a new list over Clear() to avoid capacity retention and any aliasing.Since you now return defensive copies, aliasing risk is mostly gone, but Clear() keeps capacity. Reassign to release memory and keep behavior consistent with prior suggestion.
- _cachedVisibleTargets.Clear(); + _cachedVisibleTargets = new List<Creature>(); _lastTargetCacheTime = 0.0; InvalidateDistanceCache(); // Also clear distance cache
288-347: Dead/hidden filters restored — nice. Add null-guards for PhysicsObj to prevent rare NREs.In busy cells, some objects may transiently lack PhysicsObj; skip them safely.
- foreach (var creature in listOfCreatures) + foreach (var creature in listOfCreatures) { // exclude dead creatures if (creature.IsDead) continue; @@ - if (PhysicsObj.get_distance_sq_to_object(creature.PhysicsObj, true) > chaseDistSq) + var myPhysics = PhysicsObj; + var targetPhysics = creature.PhysicsObj; + if (myPhysics == null || targetPhysics == null) + continue; + if (myPhysics.get_distance_sq_to_object(targetPhysics, true) > chaseDistSq) continue;
🧹 Nitpick comments (3)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (3)
191-193: DRY up repeated cache invalidations after AttackTarget changes.Extract a helper to centralize invalidation and reduce duplication.
Apply within the switch cases:
- AttackTarget = SelectWeightedDistance(targetDistances); - _lastTargetCacheTime = 0.0; // Invalidate cache when target changes - InvalidateDistanceCache(); // Also invalidate distance cache + AttackTarget = SelectWeightedDistance(targetDistances); + InvalidateTargetCaches(); - if (lastDamager != null) - { - AttackTarget = lastDamager; - _lastTargetCacheTime = 0.0; // Invalidate cache when target changes - InvalidateDistanceCache(); // Also invalidate distance cache - } + if (lastDamager != null) + { + AttackTarget = lastDamager; + InvalidateTargetCaches(); + } - if (topDamager != null) - { - AttackTarget = topDamager; - _lastTargetCacheTime = 0.0; // Invalidate cache when target changes - InvalidateDistanceCache(); // Also invalidate distance cache - } + if (topDamager != null) + { + AttackTarget = topDamager; + InvalidateTargetCaches(); + } - AttackTarget = lowestLevel; - _lastTargetCacheTime = 0.0; // Invalidate cache when target changes - InvalidateDistanceCache(); // Also invalidate distance cache + AttackTarget = lowestLevel; + InvalidateTargetCaches(); - AttackTarget = highestLevel; - _lastTargetCacheTime = 0.0; // Invalidate cache when target changes - InvalidateDistanceCache(); // Also invalidate distance cache + AttackTarget = highestLevel; + InvalidateTargetCaches(); - AttackTarget = nearest[0].Target; - _lastTargetCacheTime = 0.0; // Invalidate cache when target changes - InvalidateDistanceCache(); // Also invalidate distance cache + AttackTarget = nearest[0].Target; + InvalidateTargetCaches();Add once in the class:
private void InvalidateTargetCaches() { _lastTargetCacheTime = 0.0; InvalidateDistanceCache(); }Also applies to: 205-207, 215-219, 229-232, 237-240, 246-248
266-286: Avoid extra allocation on cache miss; return the freshly built list.You already copied into the cache; returning another copy adds unnecessary allocation.
- return new List<Creature>(visibleTargets); + return visibleTargets;
592-597: Comment clarity: players are already excluded above.Tweak comment to reflect actual intent and avoid confusion.
- // Don't skip players based on TargetingTactic - that's for monster behavior, not target validity + // Only apply TargetingTactic-based skip to non-players (players are excluded above) if (!creature.Attackable && creature.TargetingTactic == TargetingTactic.None && !(creature is Player)) continue;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (4)
Source/ACE.Server/Physics/Common/ObjectMaint.cs (17)
List(153-164)List(166-177)List(179-190)List(234-245)List(292-303)List(305-316)List(318-329)List(331-350)List(355-378)List(383-411)List(491-512)List(656-675)List(717-728)List(730-741)List(743-762)List(764-783)PhysicsObj(100-112)Source/ACE.Server/WorldObjects/Monster_Navigation.cs (2)
Creature(17-704)InvalidateDistanceCache(35-39)Source/ACE.Server/Entity/Timers.cs (1)
Timers(7-48)Source/ACE.Server/Physics/PhysicsObj.cs (5)
PhysicsObj(29-3251)PhysicsObj(121-159)PhysicsObj(480-483)PhysicsObj(2325-2336)PhysicsObj(2338-2344)
🔇 Additional comments (1)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (1)
155-157: Good call using uncached targets for critical target selection.Prevents stale visibility impacting target choice.
| /// <summary> | ||
| /// Cache for visible targets to reduce expensive lookups | ||
| /// </summary> | ||
| private List<Creature> _cachedVisibleTargets = new List<Creature>(); | ||
| private double _lastTargetCacheTime = 0.0; | ||
| private const double TARGET_CACHE_DURATION = 0.6; // Cache for 0.6 seconds | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm target-cache TTL (0.6s) vs PR summary (0.5s); align across caches.
PR description says 0.5s target cache, but code uses 0.6s. Also, distance cache elsewhere is 0.4s while the PR text mentions 0.25s. Please confirm intended values and align code/PR text accordingly.
Apply if 0.5s is desired:
- private const double TARGET_CACHE_DURATION = 0.6; // Cache for 0.6 seconds
+ private const double TARGET_CACHE_DURATION = 0.5; // Cache for 0.5 secondsRun to verify all TTLs:
🏁 Script executed:
#!/bin/bash
rg -nP --type=cs -C2 '(TARGET_CACHE_DURATION|DISTANCE_CACHE_DURATION)\s*=\s*[\d.]+'Length of output: 1007
Align cache TTLs with PR description.
- In Source/ACE.Server/WorldObjects/Monster_Awareness.cs (line 31), TARGET_CACHE_DURATION is 0.6 s but the PR summary says 0.5 s.
- In Source/ACE.Server/WorldObjects/Monster_Navigation.cs (line 30), DISTANCE_CACHE_DURATION is 0.4 s but the PR summary says 0.25 s.
Confirm the intended durations and update the constants or the PR text so they match.
🤖 Prompt for AI Agents
In Source/ACE.Server/WorldObjects/Monster_Awareness.cs around lines 26-32 and
Source/ACE.Server/WorldObjects/Monster_Navigation.cs around line 30, the cache
duration constants do not match the PR summary; update TARGET_CACHE_DURATION
from 0.6 to 0.5 and DISTANCE_CACHE_DURATION from 0.4 to 0.25 to align with the
PR description, or if the 0.6/0.4 values were intentional, change the PR summary
to reflect the actual values—ensure both the code constants and PR text
consistently state the intended durations.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Source/ACE.Server/WorldObjects/Monster_Navigation.cs (1)
149-171: NRE risk when AttackTarget or its PhysicsObj is null.TurnToObject/MoveToObject dereference AttackTarget.PhysicsObj without guards (teleport/despawn can null these). Add a fast null check and bail/reset.
Apply:
@@ public void StartTurn() - // send network actions - var targetDist = GetCachedDistanceToTarget(); + // send network actions + var targetDist = GetCachedDistanceToTarget(); + var tgt = AttackTarget; + var tgtPhys = tgt?.PhysicsObj; + if (tgt == null || tgtPhys == null) + { + ResetAttack(); + return; + } @@ - if (turnTo) - PhysicsObj.TurnToObject(AttackTarget.PhysicsObj, mvp); - else - PhysicsObj.MoveToObject(AttackTarget.PhysicsObj, mvp); + if (turnTo) + PhysicsObj.TurnToObject(tgtPhys, mvp); + else + PhysicsObj.MoveToObject(tgtPhys, mvp);Source/ACE.Server/WorldObjects/Monster_Awareness.cs (1)
295-350: Guard distance calls against null PhysicsObj.creature.PhysicsObj (or this.PhysicsObj) can be null during teleport/despawn; add a quick check before get_distance_sq_to_object.
Apply:
@@ public List<Creature> GetAttackTargetsUncached() - if (PhysicsObj.get_distance_sq_to_object(creature.PhysicsObj, true) > chaseDistSq) + if (PhysicsObj == null || creature.PhysicsObj == null) + continue; + if (PhysicsObj.get_distance_sq_to_object(creature.PhysicsObj, true) > chaseDistSq) continue;
♻️ Duplicate comments (1)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (1)
75-79: Prefer list replacement over Clear() to sever aliases.Past suggestion still applies here if any external aliasing ever reappears. Replacing avoids mutating a list someone might hold.
Apply:
- _cachedVisibleTargets.Clear(); + _cachedVisibleTargets = new List<Creature>(); _lastTargetCacheTime = 0.0; InvalidateDistanceCache(); // Also clear distance cache
🧹 Nitpick comments (6)
Source/ACE.Server/WorldObjects/Monster_Navigation.cs (3)
41-76: Auto-invalidate on AttackTarget change to harden the cache.Today you rely on callers to call InvalidateDistanceCache(); add a lightweight guard so the cache self-invalidates if AttackTarget changes between calls.
Apply:
@@ - public float GetCachedDistanceToTarget() + private Creature _cachedDistanceTargetRef; + public float GetCachedDistanceToTarget() { var currentTime = Timers.RunningTime; + + // Auto-invalidate if target changed + if (!ReferenceEquals(_cachedDistanceTargetRef, AttackTarget)) + { + _cachedDistanceTargetRef = AttackTarget; + _cachedDistanceToTarget = -1.0f; + _lastDistanceCacheTime = 0.0; + } @@ _lastDistanceCacheTime = currentTime; return _cachedDistanceToTarget; }Also consider Timers.PortalYearTicks if you want per-tick determinism.
542-544: Compute distance to the provided target, not implicitly to AttackTarget.IsFacing(WorldObject target) should respect its parameter. Use cached distance only when target == AttackTarget; otherwise compute directly.
Apply:
- var dist = Math.Max(0, GetCachedDistanceToTarget()); + var dist = target == AttackTarget + ? Math.Max(0, GetCachedDistanceToTarget()) + : (PhysicsObj != null && target?.PhysicsObj != null + ? (float)PhysicsObj.get_distance_to_object(target.PhysicsObj, true) + : float.MaxValue);
566-567: Minor: avoid recomputing distance in different layers.If perf ever matters here, consider threading the previously computed distance (from StartTurn) into GetMovementParameters() or passing a flag; not urgent.
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (3)
33-41: Option: clear the list, not just the timestamp, when invalidating.Not required, but freeing the list (or clearing) prevents stale contents from lingering and reduces ambiguity.
Apply either:
private void InvalidateTargetCaches() { - _lastTargetCacheTime = 0.0; + _cachedVisibleTargets.Clear(); + _lastTargetCacheTime = 0.0; InvalidateDistanceCache(); }
271-289: Unify return semantics and reduce allocations.Currently: cache-hit returns a copy; cache-miss returns the original list. Recommend always returning a copy and reusing the internal list to cut GC.
Apply:
- // Update cache with defensive copy - _cachedVisibleTargets = new List<Creature>(visibleTargets); + // Update cache reusing the list to reduce allocations + _cachedVisibleTargets.Clear(); + _cachedVisibleTargets.AddRange(visibleTargets); _lastTargetCacheTime = currentTime; - return visibleTargets; + return new List<Creature>(_cachedVisibleTargets);
26-32: Add lightweight observability for cache efficacy.Track hit/miss counters to validate the 0.5s/0.25s TTLs in production and tune later.
For example, increment counters on cache-hit vs refresh and expose via your existing ServerPerformanceMonitor.
Also applies to: 271-289
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs(8 hunks)Source/ACE.Server/WorldObjects/Monster_Navigation.cs(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Source/ACE.Server/WorldObjects/Monster_Navigation.cs (3)
Source/ACE.Server/Entity/Timers.cs (1)
Timers(7-48)Source/ACE.Server/Physics/PhysicsObj.cs (6)
PhysicsObj(29-3251)PhysicsObj(121-159)PhysicsObj(480-483)PhysicsObj(2325-2336)PhysicsObj(2338-2344)get_distance_to_object(1945-1957)Source/ACE.Server/WorldObjects/Monster_Magic.cs (1)
GetSpellMaxRange(118-128)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (4)
Source/ACE.Server/Physics/Common/ObjectMaint.cs (17)
List(153-164)List(166-177)List(179-190)List(234-245)List(292-303)List(305-316)List(318-329)List(331-350)List(355-378)List(383-411)List(491-512)List(656-675)List(717-728)List(730-741)List(743-762)List(764-783)PhysicsObj(100-112)Source/ACE.Server/WorldObjects/Monster_Navigation.cs (2)
Creature(17-704)InvalidateDistanceCache(35-39)Source/ACE.Server/Entity/Timers.cs (1)
Timers(7-48)Source/ACE.Server/Physics/PhysicsObj.cs (5)
PhysicsObj(29-3251)PhysicsObj(121-159)PhysicsObj(480-483)PhysicsObj(2325-2336)PhysicsObj(2338-2344)
🔇 Additional comments (9)
Source/ACE.Server/WorldObjects/Monster_Navigation.cs (5)
25-31: Distance cache: good addition and sane TTL (0.25s).Fields and default sentinel look fine.
191-194: Reasonable use of cached distance for immobile melee reset.Looks good.
217-217: Melee range check switch to cached distance is appropriate.Good CPU saver; preserves semantics.
225-225: Attack range check via cached distance is appropriate.LGTM.
41-76: All AttackTarget assignments properly invalidate caches
EveryAttackTarget = …site in Monster_Awareness.cs (lines 199–200, 212–213, 222–223, 235–236, 242–243, 249–250) immediately callsInvalidateTargetCaches(), which in turn callsInvalidateDistanceCache()(Monster_Awareness.cs 36–40; Monster_Navigation.cs 35–36).Source/ACE.Server/WorldObjects/Monster_Awareness.cs (4)
26-32: Target cache: good TTL (0.5s) and fields.Matches PR intent.
164-166: Using uncached targets for critical selection is the right call.Prevents stale choices.
200-206: Cache invalidation on target changes: good coverage.Nice touch ensuring both target and distance caches reset on change.
Also applies to: 211-215, 221-225, 235-237, 243-244, 249-251
595-601: Faction scan filters tightened: looks correct.Aligns with other paths; good.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (3)
33-41: Centralized invalidation is good; prefer using it everywhere.This helper is correct; see Sleep() duplication note below.
76-80: DRY: call InvalidateTargetCaches() instead of duplicating logic in Sleep().Keeps the behavior in one place and avoids divergence.
- // Clear target cache when sleeping - _cachedVisibleTargets.Clear(); - _lastTargetCacheTime = 0.0; - InvalidateDistanceCache(); // Also clear distance cache + // Clear both target & distance caches when sleeping + InvalidateTargetCaches();
268-291: Cache hit/miss paths return defensive copies; reuse internal list to reduce allocs.Solid balance of safety and performance.
If you ever want to avoid per-call copying, consider changing the return type to IReadOnlyList and keeping the internal list immutable to callers. Low priority since API ripple is larger.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs(8 hunks)Source/ACE.Server/WorldObjects/Monster_Navigation.cs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Source/ACE.Server/WorldObjects/Monster_Navigation.cs
🧰 Additional context used
🧬 Code graph analysis (1)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (3)
Source/ACE.Server/Physics/Common/ObjectMaint.cs (17)
List(153-164)List(166-177)List(179-190)List(234-245)List(292-303)List(305-316)List(318-329)List(331-350)List(355-378)List(383-411)List(491-512)List(656-675)List(717-728)List(730-741)List(743-762)List(764-783)PhysicsObj(100-112)Source/ACE.Server/WorldObjects/Monster_Navigation.cs (2)
Creature(17-709)InvalidateDistanceCache(35-39)Source/ACE.Server/Entity/Timers.cs (1)
Timers(7-48)
🔇 Additional comments (4)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (4)
26-32: Target-cache TTL matches PR (0.5s).Constants and fields look good.
165-167: Good call to bypass cache for critical target decisions.Stops stale targets from influencing selection.
293-352: Uncached path filtering looks correct and consistent with other logic.Dead/hidden/teleporting/attackable/faction/tolerance checks are in proper order.
188-253: No additional cache invalidation required
InvalidateTargetCaches is private to Monster_Awareness; AttackTarget assignments in other classes don’t use those caches.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (1)
76-80: Clearing caches on Sleep is correct; prior aliasing concern is now moot.Because GetAttackTargets() returns a defensive copy, mutating the internal list here no longer affects callers. Optionally, reassign a new List to drop capacity after large waves.
- _cachedVisibleTargets.Clear(); + _cachedVisibleTargets = new List<Creature>();
🧹 Nitpick comments (4)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (4)
31-31: Make TARGET_CACHE_DURATION configurable.Expose via PropertyManager/server setting so ops can tune without a rebuild.
195-253: DRY: factor AssignTarget+Invalidate helper.Reduce repetition and ensure all code paths invalidate consistently.
+// near other private helpers +private void SetAttackTargetAndInvalidate(Creature target) +{ + AttackTarget = target; + InvalidateTargetCaches(); +} @@ - var targetDistances = BuildTargetDistance(visibleTargets); - AttackTarget = SelectWeightedDistance(targetDistances); - InvalidateTargetCaches(); + var targetDistances = BuildTargetDistance(visibleTargets); + SetAttackTargetAndInvalidate(SelectWeightedDistance(targetDistances)); @@ - { - AttackTarget = lastDamager; - InvalidateTargetCaches(); - } + { + SetAttackTargetAndInvalidate(lastDamager); + } @@ - { - AttackTarget = topDamager; - InvalidateTargetCaches(); - } + { + SetAttackTargetAndInvalidate(topDamager); + } @@ - AttackTarget = lowestLevel; - InvalidateTargetCaches(); + SetAttackTargetAndInvalidate(lowestLevel); @@ - AttackTarget = highestLevel; - InvalidateTargetCaches(); + SetAttackTargetAndInvalidate(highestLevel); @@ - AttackTarget = nearest[0].Target; - InvalidateTargetCaches(); + SetAttackTargetAndInvalidate(nearest[0].Target);
268-291: Avoid double enumeration on cache refresh.On a miss, you traverse visibleTargets twice (AddRange + new List copy). Assign the cache to a copy and return the original list to cut one pass.
- // Update cache reusing the list to reduce allocations - _cachedVisibleTargets.Clear(); - _cachedVisibleTargets.AddRange(visibleTargets); - _lastTargetCacheTime = currentTime; - - return new List<Creature>(_cachedVisibleTargets); + // Update cache with a copy; return the fresh list + _cachedVisibleTargets = new List<Creature>(visibleTargets); + _lastTargetCacheTime = currentTime; + return visibleTargets;
268-291: Add cache hit/miss counters.Instrument GetAttackTargets() with ServerPerformanceMonitor (or similar) for hit/miss and average list size to validate the perf win and catch regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (3)
Source/ACE.Server/Physics/Common/ObjectMaint.cs (17)
List(153-164)List(166-177)List(179-190)List(234-245)List(292-303)List(305-316)List(318-329)List(331-350)List(355-378)List(383-411)List(491-512)List(656-675)List(717-728)List(730-741)List(743-762)List(764-783)PhysicsObj(100-112)Source/ACE.Server/WorldObjects/Monster_Navigation.cs (2)
Creature(17-709)InvalidateDistanceCache(35-39)Source/ACE.Server/Entity/Timers.cs (1)
Timers(7-48)
🔇 Additional comments (7)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (7)
26-41: LGTM: clean, minimal target-cache with explicit invalidation.The private cache fields and InvalidateTargetCaches() are straightforward and cohesive.
26-41: Confirm single-threaded access to Creature; otherwise guard cache.If GetAttackTargets()/InvalidateTargetCaches() can be called from multiple threads, add a lock or switch to copy-on-write to avoid races on _cachedVisibleTargets/_lastTargetCacheTime. If the actor model guarantees single-threaded access per entity, ignore.
165-167: Good call to bypass cache for critical target selection.Prevents stale decisions on a cache hit.
304-351: Uncached path filters look correct and consistent.Dead, non-attackable, hidden players, teleporting, same-faction-without-retaliate, tolerance gates, and distance checks are ordered sensibly.
596-603: Corrected filtering order prevents non-attackables from slipping through.Good separation of Attackable check from TargetingTactic-based skip.
31-31: TTL constants verified
BothTARGET_CACHE_DURATION(0.5 s) andDISTANCE_CACHE_DURATION(0.25 s) are correctly defined and align with the PR.
299-301: Physics API method name correct. GetVisibleTargetsValuesOfTypeCreature is defined in ObjectMaint and referenced throughout the codebase; no change required.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (1)
102-106: Use the new cache-invalidation helper in Sleep() (regression of earlier fix).Sleep duplicates invalidation logic and still mutates the list in place. Call InvalidateTargetCaches() for consistency and to avoid aliasing risk.
- // Clear target cache when sleeping - _cachedVisibleTargets.Clear(); - _lastTargetCacheTime = 0.0; - InvalidateDistanceCache(); // Also clear distance cache + // Clear both target and distance caches consistently + InvalidateTargetCaches();
🧹 Nitpick comments (3)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (3)
40-45: Prefer new list instance over Clear() in invalidation.Assigning a new List avoids any chance of aliasing if the cache list ever leaks again and can be marginally cheaper than Clear() for large lists.
- _cachedVisibleTargets.Clear(); + _cachedVisibleTargets = new List<Creature>();
288-312: Avoid torn reads of _lastTargetCacheTime; use Volatile if cross-thread access is possible.If Creature methods can run on different threads, guard double reads/writes with Volatile. If everything runs on a single world thread, you can ignore this.
- var currentTime = Timers.RunningTime; + var currentTime = Timers.RunningTime; + var last = System.Threading.Volatile.Read(ref _lastTargetCacheTime); // Check if cache is still valid - if (_lastTargetCacheTime > 0.0 && (currentTime - _lastTargetCacheTime) < TARGET_CACHE_DURATION) + if (last > 0.0 && (currentTime - last) < TARGET_CACHE_DURATION) { - System.Threading.Interlocked.Increment(ref _cacheHits); + System.Threading.Interlocked.Increment(ref _cacheHits); return new List<Creature>(_cachedVisibleTargets); } // Cache expired, refresh it - System.Threading.Interlocked.Increment(ref _cacheMisses); + System.Threading.Interlocked.Increment(ref _cacheMisses); var visibleTargets = GetAttackTargetsUncached(); // Update cache with a copy; return the fresh list to avoid double enumeration _cachedVisibleTargets = new List<Creature>(visibleTargets); - _lastTargetCacheTime = currentTime; + System.Threading.Volatile.Write(ref _lastTargetCacheTime, currentTime);
341-352: Defensive null check before distance math.If a creature’s PhysicsObj can be null transiently, this guards against NREs during enumeration.
- if (creature.Teleporting) + if (creature.Teleporting) continue; // ensure within 'detection radius' ? var chaseDistSq = creature == AttackTarget ? MaxChaseRangeSq : VisualAwarenessRangeSq; /*if (Location.SquaredDistanceTo(creature.Location) > chaseDistSq) continue;*/ - if (PhysicsObj.get_distance_sq_to_object(creature.PhysicsObj, true) > chaseDistSq) + if (creature.PhysicsObj == null) + continue; + if (PhysicsObj.get_distance_sq_to_object(creature.PhysicsObj, true) > chaseDistSq) continue;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (2)
Source/ACE.Server/Physics/Common/ObjectMaint.cs (17)
List(153-164)List(166-177)List(179-190)List(234-245)List(292-303)List(305-316)List(318-329)List(331-350)List(355-378)List(383-411)List(491-512)List(656-675)List(717-728)List(730-741)List(743-762)List(764-783)PhysicsObj(100-112)Source/ACE.Server/WorldObjects/Monster_Navigation.cs (2)
Creature(17-709)InvalidateDistanceCache(35-39)
🔇 Additional comments (4)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (4)
26-36: TTL and counters look good.TARGET_CACHE_DURATION = 0.5s matches the PR summary; cache hit/miss counters are scoped static and cheap via Interlocked.
191-193: Good call: bypass cache for target selection and centralize invalidation.Using GetAttackTargetsUncached() for critical decisions and SetAttackTargetAndInvalidate(...) across tactics keeps correctness and cache coherence.
Also applies to: 225-227, 235-240, 244-249, 258-266, 270-272
617-624: Faction scan filters look correct now.Separating dead/teleporting/attackable checks prevents non-attackables from slipping through.
31-31: TTL constants are correctly configured as per PR. TARGET_CACHE_DURATION remains 0.5 s and DISTANCE_CACHE_DURATION is 0.25 s—no updates needed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (4)
5-5: Interlocked import added — compile fix looks good.
38-47: Invalidation now severs aliases and clears both caches.
Reassigning a new List prevents external mutation; calling it from Sleep() keeps caches consistent.Also applies to: 103-105
314-318: Target filtering tightened — matches prior guidance.
Dead, non-attackable, hidden players, teleporting, and tactic checks are in the right order; null PhysicsObj guard included.Extract a shared predicate (e.g., IsValidAttackTarget(creature)) to reuse here and in CheckTargets_Inner/FactionMob paths to avoid drift.
Also applies to: 325-342, 350-351
619-626: Split conditions fix the “non-attackable slipping through” bug.
This aligns with the earlier suggestion and removes the coupled predicate bug.
🧹 Nitpick comments (3)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (3)
27-37: Target cache introduced with stats — TTL matches PR (0.5s).
Looks consistent with the PR summary. Consider making TTL a config knob if you want to tune live without rebuilds.
48-56: Avoid unnecessary invalidations when target is unchanged.
Micro-opt: skip work if the target didn’t change.private void SetAttackTargetAndInvalidate(Creature target) { - AttackTarget = target; - InvalidateTargetCaches(); + if (ReferenceEquals(AttackTarget, target)) + return; + AttackTarget = target; + InvalidateTargetCaches(); }
58-68: Cache stats helper is fine.
Optional: add a ResetCacheStats() for tests/ops.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (4)
Source/ACE.Server/Physics/Common/ObjectMaint.cs (17)
List(153-164)List(166-177)List(179-190)List(234-245)List(292-303)List(305-316)List(318-329)List(331-350)List(355-378)List(383-411)List(491-512)List(656-675)List(717-728)List(730-741)List(743-762)List(764-783)PhysicsObj(100-112)Source/ACE.Server/WorldObjects/Monster_Navigation.cs (2)
Creature(17-709)InvalidateDistanceCache(35-39)Source/ACE.Server/Entity/Timers.cs (1)
Timers(7-48)Source/ACE.Server/Physics/PhysicsObj.cs (5)
PhysicsObj(29-3251)PhysicsObj(121-159)PhysicsObj(480-483)PhysicsObj(2325-2336)PhysicsObj(2338-2344)
🔇 Additional comments (2)
Source/ACE.Server/WorldObjects/Monster_Awareness.cs (2)
190-191: Uncached selection for critical decisions + single-point invalidation is sound.
Good calls to SetAttackTargetAndInvalidate(...) across tactics; pairs well with the cached reader.Also applies to: 225-225, 236-238, 245-247, 258-259, 264-265, 270-271
287-312: Volatile.Read/Write on double are supported for net8.0 — no change required.
Project targets net8.0 (Source/ACE.Server/ACE.Server.csproj); System.Threading.Volatile.Read/Write include generic overloads that accept double, so the suggested Interlocked swap is unnecessary.Likely an incorrect or invalid review comment.
|
Tested successfully. @xxtindalexx can you load this on test server please? |
|
this one gon be gooooood |
2 participants
Monster tick rate: 0.2s → 0.4s (50% less CPU)
Target cache: Monsters remember who they can attack for 0.5s
Distance cache: Monsters remember how far targets are for 0.25s
Summary by CodeRabbit
New Features
Chores
Style