Conversation
…e prefab, PlayerSpawner
B-1: Rename Assets/_SecondSpawn/Scenes/SampleScene.unity -> ZoneTest_Hub.unity
(GUID 99c9720ab356a0642a771bea13969a05 preserved, build settings updated).
B-2: Build scene hierarchy buckets per docs/setup/unity-conventions.md:
_Managers, _Cameras, _Lights, _UI, World, _DynamicObjects. Reparent
existing objects (Main Camera renamed to PlayerCamera under _Cameras;
Directional Light + Global Volume under _Lights; _NetworkBootstrap
under _Managers).
B-3: Add Player_NetworkCube.prefab (Cube + NetworkObject + NetworkPlayer)
at Assets/_SecondSpawn/Prefabs/. Attach PlayerSpawner to _NetworkBootstrap
and wire _playerPrefab + _spawnRoot SerializedObject refs. PlayerSpawner
implements INetworkRunnerCallbacks; OnPlayerJoined spawns the prefab
server-side with input authority assigned to the joining player and
reparents under _DynamicObjects. Server-only spawn gate enforces
Pillar 4 (server-authoritative) per Hard Rule #2.
Bundled (Phase A follow-through, not standalone):
- ProjectSettings.asset: runInBackground=1 (network tick continues with
editor unfocused, required for Host Mode smoke test), Fusion 2.1.1
scripting define symbols added for Standalone + Android.
- PhotonAppSettings.asset: SDK serializer auto-added empty AppIdRealtime,
AppIdQuantum, NetworkLogging, ClientLogging fields.
Bundled (dev tooling):
- Packages/manifest.json + packages-lock.json: add com.coplaydev.unity-mcp
package (HTTP localhost:8080 MCP bridge). Replaces Unity AI Assistant
relay for Claude Code agents; bypasses cap=1 entitlement contention
documented in CLAUDE.md sustainable MCP workflow. Unity AI Assistant
package retained for Codex agent.
Code review: APPROVED (one low-urgency should-fix deferred to slice
phase 2 when pets/mounts introduce multi-NetworkObject ownership).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ion 2 reparent pattern (observed not to stick)
- Wrap INetworkRunnerCallbacks.OnUserSimulationMessage with #pragma warning
disable CS0618 in PlayerSpawner.cs + NetworkInputProvider.cs. Fusion 2.1.1
marked the SimulationMessagePtr parameter type obsolete ("Not used anymore"
per Photon/Fusion/release_history.txt line 408) but the interface still
requires the implementation, so the warning cannot be eliminated by code
removal. The pragma is scoped to the single method, with a comment citing
the release_history reference.
- Refactor PlayerSpawner.OnPlayerJoined to use Fusion 2 idiomatic
runner.Spawn(prefab, pos, rot, player, onBeforeSpawned: callback) overload.
Captures _spawnRoot into a local before passing the closure so re-spawns
read the field fresh on each call. Calls Transform.SetParent inside the
onBeforeSpawned callback (after instantiation, before Spawned()) which is
the documented Fusion 2 pattern for setting a NetworkObject's parent at
spawn time.
KNOWN ISSUE: Play mode verification shows the reparent does NOT stick under
Fusion 2.1.1 - the cube still ends up at scene root rather than under
_DynamicObjects. The TODO comment in the callback flags the gap for a
Phase B follow-up investigation (likely Fusion moves the GO into a
runner-managed scene after the callback). Tracked for the next slice.
Phase B smoke-test core criteria remain met: cube spawns on PlayerJoined
with NetworkObject + NetworkPlayer + weaver-generated NetworkObjectPrefabData
attached. Reparent organization is cosmetic for the smoke test scope.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… (FieldAccessException workaround)
Add SECOND SPAWN PATCH AssemblyAttributes.cs declaring
`[assembly: System.Runtime.CompilerServices.IgnoresAccessChecksTo("Fusion.Runtime")]`
on SecondSpawn.Networking. Without this, Fusion 2.1.1's IL weaver emits
direct accesses to Fusion.NetworkBehaviour::Ptr (internal field) inside
every [Networked] property getter/setter, but Fusion.Runtime.dll's
[InternalsVisibleTo] list only grants access to Fusion.Unity.Tests/Editor,
Unity.Fusion.CodeGen, Fusion.Plugin/Json/Runtime.Tests - it does NOT
include SecondSpawn.Networking. CLR raises FieldAccessException at runtime
as soon as NetworkPlayer.Spawned() executes set_NetworkedPosition.
Phase A smoke test passed (commit 7aacfe5) because NetworkInputProvider's
INetworkInput is a plain data struct that never accesses Ptr. Phase B
(commit f109928) NetworkPlayer is the first NetworkBehaviour that does.
IgnoresAccessChecksToAttribute is not in BCL - declared the type inline
(Roslyn matches by full name). Confirmed Play mode passes the
FieldAccessException barrier after this patch in Unity 6.5 beta b7
plus Fusion 2.1.1-RC.
Long-term fix: when Photon ships a Fusion version that grants access via
AssembliesToWeave or exposes Ptr publicly, remove this file. Track
Photon/Fusion/release_history.txt; revert when confirmed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Claude (@claude) review Please review this PR with focus on:
Context: JOY is non-coder, so please call out concrete blocking issues first, then nice-to-have cleanup. |
![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 transitions the game backend to Nakama OSS (ADR 0010) and integrates Photon Fusion Simple KCC for authoritative movement. It establishes a framework for AI agents and OpenClaw-connected NPCs, including character profiles, soul definitions, and memory systems. The changes span Unity client logic, a Go LLM gateway prototype, and Nakama runtime modules. Review feedback highlights critical networking issues, specifically the need to use networked state for velocity calculations in NetworkAnimatorBridge and to network agent inputs in NetworkPlayer to avoid simulation divergence. Further recommendations include optimizing object discovery in CharacterMemorySync, adding error handling for JSON parsing in Nakama modules, and adopting more robust methods for JWT parsing and unique ID generation.
| var worldVelocity = (transform.position - _previousPosition) / deltaTime; | ||
| worldVelocity.y = 0f; | ||
| _previousPosition = transform.position; | ||
|
|
||
| var localVelocity = transform.InverseTransformDirection(worldVelocity); | ||
| var referenceMoveSpeed = Mathf.Max(0.01f, _referenceMoveSpeed); | ||
| NetSpeed = Mathf.Clamp01(worldVelocity.magnitude / referenceMoveSpeed); | ||
| NetVelocityX = Mathf.Clamp(localVelocity.x / referenceMoveSpeed, -1f, 1f); | ||
| NetVelocityZ = Mathf.Clamp(localVelocity.z / referenceMoveSpeed, -1f, 1f); |
There was a problem hiding this comment.
The velocity calculation manually computes a delta from _previousPosition, which is not a [Networked] variable. In Photon Fusion, non-networked state is not rolled back during resimulations, leading to incorrect velocity values and animation glitches on the client when mispredictions occur. Since SimpleKCC is already a dependency on this object, it is more reliable and resimulation-safe to use _kcc.RealVelocity directly for these calculations.
var worldVelocity = _kcc.RealVelocity;\n worldVelocity.y = 0f;\n\n var localVelocity = transform.InverseTransformDirection(worldVelocity);\n var referenceMoveSpeed = Mathf.Max(0.01f, _referenceMoveSpeed);\n NetSpeed = Mathf.Clamp01(worldVelocity.magnitude / referenceMoveSpeed);\n NetVelocityX = Mathf.Clamp(localVelocity.x / referenceMoveSpeed, -1f, 1f);\n NetVelocityZ = Mathf.Clamp(localVelocity.z / referenceMoveSpeed, -1f, 1f);| public void SetPrototypeAgentInput(NetworkInputData input) | ||
| { | ||
| _prototypeAgentInput = input; | ||
| _hasPrototypeAgentInput = true; | ||
| if (HasStateAuthority) | ||
| { | ||
| IsAgentControlled = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
The SetPrototypeAgentInput method sets a local _prototypeAgentInput field which is not [Networked]. In a dedicated server topology, the server will not have access to this input if it is set by a client (e.g., via the PrototypeLLMAgentDriver which reads local keyboard input), causing the simulation to diverge. Furthermore, for the 'offline' agent feature to function, the server must be the authority driving the agent; if the input is only stored locally on a client, the server cannot simulate the agent's movement when the player is disconnected.
|
|
||
| private static bool TryApplyProfileEquipment(int equipmentVisualId) | ||
| { | ||
| var players = Object.FindObjectsByType<NetworkPlayer>(FindObjectsInactive.Exclude, FindObjectsSortMode.None); |
There was a problem hiding this comment.
Object.FindObjectsByType is an expensive operation that searches the entire scene hierarchy. Calling it every frame within the ApplyProfileEquipmentWhenAvailable coroutine loop (line 114) for up to 10 seconds can cause significant performance spikes. Consider using a more efficient discovery mechanism, such as having the NetworkPlayer register itself with a manager upon spawning or using a static registry.
| var start = json.IndexOf(marker, StringComparison.Ordinal); | ||
| if (start < 0) | ||
| { | ||
| return ""; | ||
| } | ||
|
|
||
| start += marker.Length; | ||
| var end = json.IndexOf('"', start); | ||
| return end > start ? json.Substring(start, end - start) : ""; |
There was a problem hiding this comment.
Manual parsing of JWT claims using string searching (json.IndexOf) is brittle and prone to failure if the JSON formatting changes (e.g., whitespace, field order, or minification). It is safer to use a proper JSON parser after base64-decoding the JWT payload segment to ensure compatibility with different serialization outputs.
| return null; | ||
| } | ||
|
|
||
| var body = JSON.parse(response.body); |
There was a problem hiding this comment.
JSON.parse(response.body) is called without a try-catch block. If the external Supabase Auth service returns an unexpected non-JSON response (such as an HTML error page from a proxy or an empty body), this will throw a runtime exception, causing the beforeAuthenticateCustom hook to fail and preventing user login.
var body;\n try {\n body = JSON.parse(response.body);\n } catch (e) {\n logger.error("Supabase Auth returned invalid JSON: " + response.body);\n return null;\n }| memory.importance = clampNumber(memory.importance || 5, 1, 10); | ||
| if (!memory.id) { | ||
| memory.id = "mem-" + nowId() + "-" + (context.body.memory.length + 1); | ||
| } |
There was a problem hiding this comment.
Generating IDs using Date.getTime() is prone to collisions if multiple memory records are added in the same millisecond. While the array length suffix provides some mitigation, it is not robust against concurrent requests or deletions in a distributed environment. Using a UUID or a more robust unique ID generator is recommended for production-ready data.
|
Addressed Gemini review findings in
Re-verified locally:
|
|
Codex local review pass after recovered thread continuation. Verdict: approved with merge held for optional/local Claude Code review, per repo hard rule. What I checked:
Verification:
No merge-blocking issues found by Codex local review. Holding actual |
1 participant
What does this PR do?
Promotes the current prototype foundation from
devtomain: Simple KCC player control, local visual loading, animation/equipment visual handling, prototype AI agent/NPC brain, Nakama OSS backend runtime, and Cloud Run gateway schema updates.Linked issue / ADR
docs/adr/0010-nakama-oss-game-backend.mdTouched areas
Unity/)backend/gateway/)docs/design/) or ADRs (docs/adr/)Test plan
cd backend/gateway && go test ./...- passedcd backend/nakama && npm run build- passedcd backend/nakama && npm test- passedsecond-spawn-gateway-00004-67hPOST /v1/agent/decideagainst Cloud Run withequipmentinsidecontext.body- returned200 OKAssets/_SecondSpawn/Scenes/ZoneTest_Hub.unityplayers=1,loaders=1, and equipment visual assigned on the authoritative playerServer-authority check (mandatory if touching gameplay)
Reviewer pass
JOY is non-coder per
.claude/CLAUDE.mdHard Rule #7. Before merge, this PR should receive AI reviewer feedback from Claude Code Max and Gemini.