feat(unity): bind scene actors to profiles#14
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Local code-review skill passTarget: PR #14 eat(unity): bind scene actors to profiles Engine Specialist Findings: N/A - no engine configured.Codex/docs/technical-preferences.md is not present in this worktree, so engine-specialist routing is skipped per the local code-review skill. Testability: TESTABLE WITH UNITY CAVEAT
ADR Compliance: NO ADRS FOUNDNo direct ADR reference was found in the implementation commit or file header. The implementation follows ADR 0010 direction by keeping actor state in Nakama and adding no separate game API gateway or client-authoritative state mutation. Standards Compliance: PASS WITH SUGGESTIONS
Architecture: CLEAN FOR PROTOTYPE
SOLID: APPROVED WITH SUGGESTIONSSingle responsibility is acceptable: bind one scene actor to one profile and optional seed memory. Longer-term, memory seeding and visual/name application could split if this becomes a general NPC spawn framework. Game-Specific Concerns
Positive Observations
Required ChangesNone from local code-review. Suggestions
Verdict: APPROVED WITH SUGGESTIONSCodex (@codex) review was requested in the PR body, but prior PRs indicate the Codex Cloud connector may still be at usage limit. Gemini review is requested as the independent external reviewer. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request introduces the ActorProfileBinder class, a MonoBehaviour designed to link scene-based NPCs with Nakama actor profiles, handling profile retrieval, memory seeding, and basic data application. The review feedback highlights several areas for improvement: ensuring the LoadActorProfile coroutine is private and robust against synchronous completion to maintain accurate loading states, defaulting _actorId to an empty string to allow fallback logic to function correctly, and reconsidering the runtime renaming of GameObjects to prevent breaking external script references.
| public IEnumerator LoadActorProfile() | ||
| { |
There was a problem hiding this comment.
The LoadActorProfile method is public, which allows it to be started externally, bypassing the _loadRoutine tracking in Refresh(). Additionally, if the coroutine finishes synchronously (e.g., if _gateway is null), Refresh() will assign the Coroutine handle to _loadRoutine after the method has finished, causing IsLoading to stay true indefinitely. It is recommended to make this method private and add a yield return null; at the start to ensure the handle is assigned before any potential early exit.
private IEnumerator LoadActorProfile()
{
yield return null;| public sealed class ActorProfileBinder : MonoBehaviour | ||
| { | ||
| [SerializeField] private bool _loadOnStart = true; | ||
| [SerializeField] private string _actorId = "npc-guide"; |
There was a problem hiding this comment.
The default value "npc-guide" for _actorId prevents the fallback logic in ResolveActorId() from using gameObject.name unless the field is manually cleared in the Inspector. This can lead to ID collisions if multiple NPCs are placed in a scene without unique configuration. Consider defaulting this to an empty string to allow the fallback to work by default.
[SerializeField] private string _actorId = "";| if (!string.IsNullOrWhiteSpace(profile.display_name)) | ||
| { | ||
| _displayName = profile.display_name.Trim(); | ||
| gameObject.name = _displayName; |
There was a problem hiding this comment.
Renaming the GameObject at runtime to match the profile's display name can have unintended side effects, such as breaking external scripts that rely on GameObject.Find or making the hierarchy difficult to navigate during play mode. Consider if this renaming is strictly necessary or if the display name should be managed by a dedicated UI component instead.
|
Delta after Gemini review on
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 47370b3. |
There was a problem hiding this comment.
Code Review
This pull request introduces the ActorProfileBinder component, which synchronizes Unity NPC objects with Nakama actor profiles and handles initial memory seeding. The review feedback identifies opportunities to improve state reliability by explicitly resetting the loading flag when a coroutine is stopped, and recommends normalizing string identifiers like actor types and memory kinds to lowercase to ensure compatibility with case-sensitive backend logic.
| if (_loadRoutine != null) | ||
| { | ||
| StopCoroutine(_loadRoutine); | ||
| } |
There was a problem hiding this comment.
When StopCoroutine is called, the LoadActorProfileRoutine is immediately terminated, and the _isLoading = false; line (line 66) is skipped. If Refresh is called while a load is in progress, _isLoading remains true until the new routine finishes. However, if the routine is stopped and not restarted (e.g., if the object is disabled), the state will be stuck. It's safer to reset the loading state explicitly when stopping the routine.
if (_loadRoutine != null)
{
StopCoroutine(_loadRoutine);
_loadRoutine = null;
_isLoading = false;
}| yield return _gateway.AddNakamaActorMemory(new ActorMemoryAddRequestDto | ||
| { | ||
| actor_id = profile.actor_id, | ||
| kind = string.IsNullOrWhiteSpace(_seedMemoryKind) ? "system" : _seedMemoryKind.Trim(), |
There was a problem hiding this comment.
The backend normalizeMemoryKind function (in backend/gateway/internal/character/store.go) uses a case-sensitive switch statement for validation. If a value like "Combat" or "System" is entered in the Unity inspector, it will fail to match the backend constants and default to "system". Normalizing to lowercase ensures the intended category is correctly applied.
kind = string.IsNullOrWhiteSpace(_seedMemoryKind) ? "system" : _seedMemoryKind.Trim().ToLowerInvariant(),| actor_type = string.IsNullOrWhiteSpace(_actorType) ? "npc" : _actorType.Trim(), | ||
| display_name = string.IsNullOrWhiteSpace(_displayName) ? gameObject.name : _displayName.Trim(), | ||
| archetype_id = string.IsNullOrWhiteSpace(_archetypeId) ? "prototype-npc" : _archetypeId.Trim(), | ||
| visual_prefab_key = string.IsNullOrWhiteSpace(_visualPrefabKey) ? "prototype-npc" : _visualPrefabKey.Trim() |
There was a problem hiding this comment.
Identifiers like actor_type, archetype_id, and visual_prefab_key are typically treated as case-sensitive keys on the backend. To prevent issues caused by accidental capitalization in the Unity inspector, it's recommended to normalize these strings to lowercase before sending the request.
actor_type = string.IsNullOrWhiteSpace(_actorType) ? "npc" : _actorType.Trim().ToLowerInvariant(),
display_name = string.IsNullOrWhiteSpace(_displayName) ? gameObject.name : _displayName.Trim(),
archetype_id = string.IsNullOrWhiteSpace(_archetypeId) ? "prototype-npc" : _archetypeId.Trim().ToLowerInvariant(),
visual_prefab_key = string.IsNullOrWhiteSpace(_visualPrefabKey) ? "prototype-npc" : _visualPrefabKey.Trim().ToLowerInvariant()bdf5940 to
ca4087e
Compare
47370b3 to
755e672
Compare
|
Delta after Gemini review and stack rebase:
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 755e672. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Local code-review delta passTarget: PR #14 latest head Engine Specialist Findings: N/A - no engine configured
Testability: TESTABLE WITH UNITY CAVEAT
ADR Compliance: COMPLIANTThe component keeps actor state in Nakama and does not grant gameplay authority, provider keys, inventory, BodyTime, or combat effects. Standards Compliance: PASS
Architecture: CLEANThe binder maps one scene actor object to one Nakama actor profile and exposes 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. |
Summary
Why
SECOND SPAWN needs many NPC-like bodies and player-inhabited bodies where each actor can own separate stats, traits, soul, memory, runtime, and activity state. This component gives scene objects a small bridge to the actor profile registry from PR #12.
Verification
pm install in �ackend/nakama for this fresh worktree so the pinned local TypeScript version is used.
pm run build in �ackend/nakama passed.
pm test in �ackend/nakama passed.
Unity Caveat
This PR adds C# only. Clean-worktree Unity batchmode compile is still blocked in this repo family by the existing Package Manager path undefined issue, so Editor/MCP compile verification is still required before a no-caveat merge claim.
Gemini (@gemini-code-assist) review
Codex (@codex) review