feat(character): apply profile stats to bodies#11
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a character statistics system, adding CharacterStatsDto to body profiles and synchronizing these stats across the backend and Unity client. Key updates include networked player properties for combat attributes, an AI agent 'attack' action, and a prototype HUD for stat visualization. Review feedback identifies performance issues with frequent FindObjectsByType calls in OnGUI and polling loops, a logic error in PrototypeAgentBrain causing exponential speed compounding, and a bug in NetworkPlayer where stat application could unintentionally revive dead players.
| return _cachedPlayer; | ||
| } | ||
|
|
||
| var players = FindObjectsByType<NetworkPlayer>(FindObjectsInactive.Exclude, FindObjectsSortMode.None); |
There was a problem hiding this comment.
Calling FindObjectsByType inside OnGUI is extremely expensive because OnGUI can be called multiple times per frame. Since this is a prototype HUD, you should at least throttle this search (e.g., once every second) or perform the resolution in Update and cache the result to avoid significant performance degradation when no player is found.
| private static bool TryApplyProfileEquipment(int equipmentVisualId) | ||
| private bool TryApplyProfileBody(BodyProfileDto body) | ||
| { | ||
| var players = Object.FindObjectsByType<NetworkPlayer>(FindObjectsInactive.Exclude); |
There was a problem hiding this comment.
This method is called within a while loop in ApplyProfileToLocalPlayerWhenAvailable, meaning Object.FindObjectsByType is executed every frame for up to 10 seconds until a player is found. This is inefficient. Consider caching the result or using a more direct way to reference the local authoritative player.
| var stats = body.stats; | ||
| if (stats != null) | ||
| { | ||
| _moveSpeed = Mathf.Clamp(_moveSpeed * Mathf.Clamp(stats.agility / 8f, 0.75f, 1.4f), 0.5f, 6f); |
There was a problem hiding this comment.
The _moveSpeed field is being modified in place by multiplying it with the agility factor. If ApplyContextToPrototypeBody is called more than once (e.g., during a context refresh), the speed multiplier will compound exponentially. It is safer to calculate the current speed from a base value or a constant default instead of overwriting the serialized field.
| Hp = previousMaxHealth > 0 | ||
| ? Mathf.Clamp(Mathf.Round(MaxHealth * healthRatio), 1f, MaxHealth) | ||
| : MaxHealth; | ||
| Stamina = previousMaxEnergy > 0 | ||
| ? Mathf.Clamp(Mathf.Round(MaxEnergy * energyRatio), 1f, MaxEnergy) | ||
| : MaxEnergy; |
There was a problem hiding this comment.
The use of a 1f floor in Mathf.Clamp will cause dead players (where Hp or Stamina is 0) to be revived with 1 HP/Stamina whenever profile stats are applied. Additionally, using Mathf.Round on a float HP value can lead to precision loss. Consider allowing 0 if the player was already dead and removing the rounding to maintain float precision.
Hp = previousMaxHealth > 0
? Mathf.Clamp(MaxHealth * healthRatio, Hp > 0f ? 1f : 0f, MaxHealth)
: MaxHealth;
Stamina = previousMaxEnergy > 0
? Mathf.Clamp(MaxEnergy * energyRatio, Stamina > 0f ? 1f : 0f, MaxEnergy)
: MaxEnergy;
|
Delta after Gemini review on PR #11:
Verification on latest head
Unity caveat:
Gemini (@gemini-code-assist) please delta re-review latest commit 149a06a. |
Local code-review skill passTarget: PR #11 Engine Specialist Findings: N/A - no engine configured
Testability: GAPS WITH EXTERNAL UNITY CAVEAT
ADR Compliance: NO ADRS FOUNDNo direct ADR reference was found in the implementation commit or file headers. The change follows the accepted architecture: Nakama owns durable profile stats, Unity applies profile-derived state to authoritative networked player state, and the client does not own economy or LLM provider secrets. Standards Compliance: PASS WITH CAVEAT
Architecture: CLEAN FOR PROTOTYPE
SOLID: APPROVED WITH SUGGESTIONSThe gateway client continues to accumulate Nakama, gateway, and auth responsibilities. Acceptable for prototype, but split once actor profile and NPC flows stabilize. Game-Specific Concerns
Positive Observations
Required ChangesNone from local code-review after Suggestions
Verdict: APPROVED WITH CAVEATVerification observed on latest head
Gemini (@gemini-code-assist) please include this local review context in the delta review. |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive character stats system, integrating it into the backend DTOs, networking layer, and AI agent brains. Key additions include the CharacterStatsDto, networked stat properties in NetworkPlayer, and agility-based movement speed scaling. The AI agent driver now supports attack actions and target detection. Additionally, a prototype IMGUI HUD was implemented to display these new stats, and documentation was updated to reflect the unified body profile model for both NPCs and players. Feedback highlights performance concerns regarding the use of FindObjectsByType in polling loops and identifies inconsistencies in agility scaling logic between player and NPC implementations. It was also noted that the HUD's time formatting is misleading for durations under one minute.
| private static bool TryApplyProfileEquipment(int equipmentVisualId) | ||
| private bool TryApplyProfileBody(BodyProfileDto body) | ||
| { | ||
| var players = Object.FindObjectsByType<NetworkPlayer>(FindObjectsInactive.Exclude); |
There was a problem hiding this comment.
Using Object.FindObjectsByType<NetworkPlayer> inside a polling loop (called every 0.25 seconds via ApplyProfileToLocalPlayerWhenAvailable) is an expensive operation as it scans the entire scene hierarchy. While acceptable for a prototype, this can cause performance degradation in scenes with many objects. Consider using a more efficient approach, such as having NetworkPlayer register itself with a manager or using a static reference for the local player character.
| var stats = body.stats; | ||
| if (stats != null) | ||
| { | ||
| _moveSpeed = Mathf.Clamp(_baseMoveSpeed * Mathf.Clamp(stats.agility / 8f, 0.75f, 1.4f), 0.5f, 6f); |
There was a problem hiding this comment.
The agility scaling logic here is inconsistent with the implementation in NetworkPlayer.GetAgilitySpeedMultiplier. Specifically, it lacks the check for agility <= 0 which returns a 1f multiplier in NetworkPlayer. Additionally, the magic numbers (8f, 0.75f, 1.4f) are duplicated across files. Consider centralizing this logic into a shared utility method to ensure consistent movement behavior between players and NPCs.
| return $"{days}d {hours}h"; | ||
| } | ||
|
|
||
| return hours > 0 ? $"{hours}h {minutes}m" : $"{minutes}m"; |
There was a problem hiding this comment.
The FormatSeconds method returns 0m for any duration less than 60 seconds when hours is 0. For a "BodyTime" display, which is a critical resource, showing 0 minutes when there are still up to 59 seconds remaining is misleading. It's better to show seconds when the remaining time is less than a minute.
return hours > 0 ? $"{hours}h {minutes}m" : (minutes > 0 ? $"{minutes}m" : $"{seconds}s");
|
Thank you for the detailed update and the comprehensive local code review. I have reviewed the latest commit |
|
Small delta after checking the latest Gemini summary against the current Unity code:
|
|
Clean delta summary after checking the latest Gemini summary against the current Unity code:
Verification on latest head
Unity caveat remains: clean-worktree Unity batchmode compile is blocked before script compilation by the existing Package Manager Gemini (@gemini-code-assist) please include latest commit 91bc026 in the delta review. |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive character stats system, integrating attributes like level, vitality, and agility into the backend DTOs, networking layer, AI behavior, and a prototype HUD. The changes allow for stat-based movement speed calculations and expanded AI actions like attacking. Review feedback identifies a potential architectural issue where client-side scripts attempt to mutate networked properties, which may conflict with a server-authoritative model. Additionally, logic inconsistencies were found in the agility-based speed multiplier calculation, where a value of zero incorrectly results in a higher speed than low positive values.
| if (_applyProfileStatsToLocalPlayer) | ||
| { | ||
| ApplyStats(player, body); | ||
| } | ||
|
|
||
| if (_applyProfileEquipmentToLocalPlayer) | ||
| { | ||
| loader.ApplyEquipmentVisual(equipmentVisualId); | ||
| ApplyEquipment(player, body); | ||
| } |
There was a problem hiding this comment.
This logic attempts to mutate networked state from what appears to be a client-side sync script. In a server-authoritative model (Pillar 4), clients cannot set [Networked] properties directly. If CharacterMemorySync runs on the client, player.ApplyProfileStats will fail the HasStateAuthority check and the stats will not replicate. Consider moving this logic to the server or using a secure RPC if the client must initiate the sync.
| if (agility <= 0) | ||
| { | ||
| return 1f; | ||
| } |
There was a problem hiding this comment.
The check if (agility <= 0) return 1f; creates a logic inconsistency where a character with 0 agility is faster (multiplier 1.0) than a character with 1 agility (multiplier 0.75). Since the Mathf.Clamp on line 386 already handles the lower bound safely, this special case should be removed or adjusted to return the minimum multiplier.
// Removed inconsistent agility <= 0 check as clamp handles the lower bound.| if (Agility <= 0) | ||
| { | ||
| return 1f; | ||
| } |
There was a problem hiding this comment.
Similar to the NPC brain logic, the if (Agility <= 0) return 1f; check makes 0-agility characters faster than 1-agility characters. The Mathf.Clamp below already provides a safe lower bound of 0.75.
private float GetAgilitySpeedMultiplier()
{
return Mathf.Clamp(Agility / 8f, 0.75f, 1.4f);
}
|
Delta after Gemini review on PR #11:
Verification on latest head
Unity caveat remains: clean-worktree Unity batchmode compile is still blocked by the known Package Manager issue, so Editor/MCP compile verification remains required before a no-caveat merge claim. Gemini (@gemini-code-assist) please delta re-review latest commit ca363a0. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
JOY (JOY)
changed the base branch from
feat/profile-bootstrap-agent-activity
to
dev
JOY (JOY)
force-pushed
the
feat/character-stats-animation
branch
from
ca363a0 to
c7d00a8
Compare
Local code-review delta passTarget: PR #11 latest head Engine Specialist Findings: N/A - no engine configured
Testability: TESTABLE WITH UNITY CAVEAT
ADR Compliance: COMPLIANTThe change follows ADR 0010 and project Hard Rule #2: profile stats are game-backend state applied only through state authority. No LLM output directly mutates authoritative state. Standards Compliance: PASS
Architecture: CLEANThe branch keeps Nakama as durable profile source, Fusion state authority as the network mutation boundary, and Unity UI/visual components as observers or prototype presenters. SOLID: APPROVED WITH SUGGESTIONSNo blocking issue found. The gateway client remains broad but acceptable for prototype. Game-Specific Concerns
Required ChangesNone. Suggestions
Verdict: APPROVED WITH CAVEATGemini delta review is currently blocked by daily quota, but this local pass verifies the latest Gemini findings were addressed and CI is green. |
1 participant
Summary
Verification
pm install in �ackend/nakama for this fresh worktree
pm run build in �ackend/nakama
pm test in �ackend/nakama
Unity note
Depends on PR #8.