Open
Conversation
Contributor
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
# Conflicts: # Explorer/Assets/DCL/AvatarRendering/AvatarShape/Assets/Animator/CharacterAnimator.controller # Explorer/Assets/DCL/Character/CharacterMotion/Settings/CharacterControllerSettings.asset # Explorer/Assets/DCL/PluginSystem/Global/CharacterMotionPlugin.cs # Explorer/Assets/DCL/PluginSystem/Global/CharacterMotionSettings.cs # Explorer/Assets/DCL/PluginSystem/Global/Global Plugins Settings.asset
Contributor
Author
|
@claude review |
Contributor
|
Claude finished @lorenzo-ranciaffi's task in 3m 54s —— View job Code Review —
|
| # | Severity | File | Issue |
|---|---|---|---|
| 1 | 🔴 Bug | PointAtMarkerHolder.cs:109 |
ResetState() does not cancel the ongoing fade CancellationTokenSource. When PointAtMarkerCleanUpSystem forcibly returns the marker to the pool, any in-flight AnimateColorAsync coroutine continues and will modify SpriteRenderer.color on a reused pooled instance, corrupting another player's marker. Fix: call fadeCts?.Cancel() inside ResetState(). |
| 2 | 🔴 Bug | HandPointAtSystem.cs:167 |
result.HitPoint == Vector3.zero uses exact floating-point equality as a sentinel. If the scene has geometry at the world origin, a valid hit there triggers the far-plane fallback incorrectly. Fix: introduce a bool hitSomething flag instead of a sentinel value. |
| 3 | 🟠 Performance | HandPointAtSystem.cs:145 |
GetComponent<RemoteAvatarCollider>() is called on every raycast hit in Update(). This allocates and violates the allocation-free Update() rule. The collider→component mapping should be precomputed (e.g., a static dictionary keyed by collider, populated at instantiation). |
| 4 | 🟡 Standards | TorsoIKSystem.cs:39 |
ApplyPointAtIK query is missing [None(typeof(DeleteEntityIntention))]. All queries must exclude entities being deleted per project standards. |
| 5 | 🟡 Standards | RemoteHandPointAtSystem.cs:38 |
ApplyRemotePointAtIK query is missing [None(typeof(DeleteEntityIntention))]. Same issue as above. |
| 6 | 🟡 Standards | HandPointAtSystem.cs:180 |
Debug.DrawLine left in production code. Wrap in `#if UNITY_EDITOR |
REVIEW_RESULT: FAIL ❌
Explorer/Assets/DCL/Character/CharacterMotion/Systems/HandPointAtSystem.cs
Show resolved
Hide resolved
Explorer/Assets/DCL/Character/CharacterMotion/Systems/HandPointAtSystem.cs
Outdated
Show resolved
Hide resolved
Explorer/Assets/DCL/Character/CharacterMotion/Systems/HandPointAtSystem.cs
Outdated
Show resolved
Hide resolved
Explorer/Assets/DCL/AvatarRendering/AvatarShape/Assets/PointAtMarkerHolder.cs
Show resolved
Hide resolved
Explorer/Assets/DCL/Character/CharacterMotion/Systems/RemoteHandPointAtSystem.cs
Show resolved
Hide resolved
Contributor
Author
|
@claude re-review |
Contributor
|
Claude finished @lorenzo-ranciaffi's task in 54s —— View job Re-Review —
|
| # | Severity | File | Status |
|---|---|---|---|
| 1 | 🔴 Bug | PointAtMarkerHolder.cs:109 |
Fixed — ResetState() now calls fadeCts?.SafeCancelAndDispose() |
| 2 | 🔴 Bug | HandPointAtSystem.cs |
Fixed — sentinel == Vector3.zero replaced with needRaycastFallback bool |
| 3 | 🟠 Performance | HandPointAtSystem.cs |
Fixed — GetComponent<RemoteAvatarCollider>() removed; layer check uses hit.collider.gameObject.layer (allocation-free) |
| 4 | 🟡 Standards | TorsoIKSystem.cs:41 |
Fixed — [None(typeof(DeleteEntityIntention))] added |
| 5 | 🟡 Standards | RemoteHandPointAtSystem.cs:40 |
Fixed — [None(typeof(DeleteEntityIntention))] added |
| 6 | 🟡 Standards | HandPointAtSystem.cs:183 |
Fixed — Debug.DrawLine wrapped in #if UNITY_EDITOR |
REVIEW_RESULT: PASS ✅
# Conflicts: # Explorer/Assets/DCL/Prefs/DCLPrefKeys.cs # Explorer/Assets/DCL/Settings/Configuration/DropdownModuleBinding.cs
Contributor
Author
|
@claude re-review |
Contributor
|
Claude finished @lorenzo-ranciaffi's task in 3m 53s —— View job Re-Review —
|
| # | Severity | File | Status |
|---|---|---|---|
| 1 | 🔴 Bug | PointAtMarkerHolder.cs:109 |
Fixed — ResetState() calls fadeCts?.SafeCancelAndDispose() |
| 2 | 🔴 Bug | HandPointAtSystem.cs |
Fixed — sentinel replaced with needRaycastFallback bool |
| 3 | 🟠 Performance | HandPointAtSystem.cs |
Fixed — GetComponent removed; layer check uses hit.collider.gameObject.layer |
| 4 | 🟡 Standards | TorsoIKSystem.cs:41 |
Fixed — [None(typeof(DeleteEntityIntention))] added |
| 5 | 🟡 Standards | RemoteHandPointAtSystem.cs:40 |
Fixed — [None(typeof(DeleteEntityIntention))] added |
| 6 | 🟡 Standards | HandPointAtSystem.cs:182 |
Fixed — Debug.DrawLine wrapped in #if UNITY_EDITOR |
New Issue Found
| # | Severity | File | Issue |
|---|---|---|---|
| 1 | 🟡 Standards | PointAtMarkerHolder.cs:74 |
await UniTask.Yield(ct) uses exception-based cancellation. If the token fires during the yield (between the IsCancellationRequested check and the await), an OperationCanceledException escapes the UniTaskVoid, is passed to UniTask's global handler (logged as an error), and the post-loop pool?.Release(this) is bypassed. Project standard: "Handle cancellation with ct.IsCancellationRequested, never ThrowIfCancellationRequested()". Fix: use await UniTask.Yield() without the token — the top-of-loop check already handles cancellation correctly. Fix this → |
REVIEW_RESULT: FAIL ❌
Explorer/Assets/DCL/AvatarRendering/AvatarShape/Assets/PointAtMarkerHolder.cs
Outdated
Show resolved
Hide resolved
Contributor
Author
|
@claude re-review |
Contributor
|
Claude finished @lorenzo-ranciaffi's task in 43s —— View job Re-Review —
|
| # | Severity | File | Status |
|---|---|---|---|
| 1 | 🟡 Standards | PointAtMarkerHolder.cs:74 |
Fixed — await UniTask.Yield(ct) changed to await UniTask.Yield(), cancellation handled by top-of-loop IsCancellationRequested check |
All Prior Issues — Still Resolved ✅
| # | File | Status |
|---|---|---|
| Bug | PointAtMarkerHolder.cs:109 |
ResetState() calls fadeCts?.SafeCancelAndDispose() |
| Bug | HandPointAtSystem.cs |
needRaycastFallback bool replaces sentinel |
| Perf | HandPointAtSystem.cs |
GetComponent removed; layer check uses hit.collider.gameObject.layer |
| Standards | TorsoIKSystem.cs:41 |
[None(typeof(DeleteEntityIntention))] present |
| Standards | RemoteHandPointAtSystem.cs:40 |
[None(typeof(DeleteEntityIntention))] present |
| Standards | HandPointAtSystem.cs:182 |
Debug.DrawLine wrapped in #if UNITY_EDITOR |
REVIEW_RESULT: PASS ✅
# Conflicts: # Explorer/Assets/DCL/AvatarRendering/AvatarShape/Assets/AvatarBase.prefab # Explorer/Assets/DCL/AvatarRendering/AvatarShape/Systems/AvatarInstantiatorSystem.cs # Explorer/Assets/DCL/AvatarRendering/AvatarShape/UnityInterface/AvatarBase.cs # Explorer/Assets/DCL/FeatureFlags/FeatureFlagsStrings.cs # Explorer/Assets/DCL/FeatureFlags/FeaturesRegistry.cs
# Conflicts: # Explorer/Assets/DCL/Infrastructure/Global/Tests/PlayMode/IntegrationTestsSuite.cs # Explorer/Assets/DCL/PluginSystem/Global/AvatarPlugin.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Description
Fixes #7301
Protocol PR
Notion Page
What does this PR change?
This PR introduces the point-at feature: users are now able to point at things in world using the middle click mouse button.
When pointing at something, if the hit is near enough, a marker will appear at the point (only for friends). Avatars never generate a hit marker.
By pointing at something, the avatar reacts with a pointing pose that is solved with IK.
The whole action/marker is shared between clients.
The marker uses the SpriteData of the Profile attached to the ECS entity. There was a bug where sprites were loaded incorrectly due to a hash clash caused by the exclusion of
AvatarTextureUserIdin the hash. Now every Profile carries the right sprite data (local player included).Test Instructions
Prerequisites
--debug --point-atTest Steps
Quality Checklist
Code Review Reference
Please review our Code Review Standards before submitting.