refactor: optimize LoadAsync methods and improve handle management - #23
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the LoadAsync methods of the Unity Addressables-backed object references (AddressableObjectReference<T> and SerializableAddressableObjectReference<T>) to add a synchronous cached fast-path (avoiding an async state machine when the asset is already loaded) and to consolidate handle cleanup into a private Release() helper that is invoked on any load failure. It replaces UniTask's autoReleaseWhenCanceled: true with explicit try/catch → Release() so the handle is released on cancellation and failure, and expands the test suite to cover progress-reporting and failure-release behavior.
Changes:
- Add a cached fast-path to both
LoadAsyncoverloads (returnnew ValueTask<T>(...)synchronously when the asset is already loaded/succeeded) with the async work moved into a static local function. - Introduce a
[MethodImpl(AggressiveInlining)] Release()helper (using_handle.Release()instead ofAddressables.Release(_handle)) and release the handle whenever a load throws. - Add
Missingaddress/GUID fixtures, aHasValidOperationHandlereflection helper, aThrowingProgressstub, and new parameterized tests for cached/failed/progress-throwing scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
Runtime/AddressableObjectReference.cs |
Cached fast-path, static local-function load core, and Release() helper with failure-driven cleanup. |
Runtime/SerializableAddressableObjectReference.cs |
Parallel refactor for the serializable variant using _value/_cached and Release(). |
Tests/Runtime/ObjectReferenceTests.cs |
Parameterizes cache tests over withProgress and adds cached-progress-throws and load-failure release tests plus a ThrowingProgress stub. |
Tests/Runtime/ObjectReferenceTestEnvironment.cs |
Adds Missing fixtures (capacity 4→6), and HasValidOperationHandle reflection helpers for both reference types. |
The core logic is sound: the removal of autoReleaseWhenCanceled: true is correctly compensated by the new catch → Release(), _handle.Release() is a valid public API, and the fast-path Status == Succeeded gating still falls through to reuse an in-progress handle. My only substantive feedback is the two optional notes about releasing an already-loaded asset when a progress callback throws.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
No description provided.