Conversation
…scribe to Dispose
…, null-guard stream source
…, simplify spawn resolver API
…tReactionsUiParticlePool
… renderer optimizations
…ual event channel
…ls and prefabs fixes
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Explorer/Assets/DCL/Chat/_Refactor/ChatMessages/MessageReactionsView.cs
Outdated
Show resolved
Hide resolved
|
Claude finished @biotech77's task in 3m 38s —— View job PR Review: feat/chat-reactionsTasks
Summary4 blocking issues found — the PR should not merge until these are resolved. Issues1. Security — wallet address spoofable (Critical)File:
2. Data corruption — reaction file unreadable after first write (Critical)File:
3. Debug code runs in production (Bug)File:
4. GPU regression —
|
Explorer/Assets/DCL/Chat/_Refactor/ChatReactions/Networking/MultiplayerReactionMessageBus.cs
Show resolved
Hide resolved
Explorer/Assets/DCL/Chat/_Refactor/ChatMessages/MessageReactionsView.cs
Outdated
Show resolved
Hide resolved
Explorer/Assets/DCL/Chat/_Refactor/ChatReactions/Rendering/ChatReactionsParticleRenderer.cs
Outdated
Show resolved
Hide resolved
|
PR #8018, run #23943128828 Builds: Windows change, Windows baseline, macOS change, macOS baseline Intel Core i3
|
|
PR #8018, run #23944583101 Builds: Windows change, Windows baseline, macOS change, macOS baseline Intel Core i3
|
|
PR #8018, run #23950517683 Builds: Windows change, Windows baseline, macOS change, macOS baseline Intel Core i3
|
|
PR #8018, run #23956101373 Builds: Windows change, Windows baseline, macOS change, macOS baseline Intel Core i3
|
|
PR #8018, run #23966339094 Builds: Windows change, Windows baseline, macOS change, macOS baseline Intel Core i3
|
…hat-reactions # Conflicts: # Explorer/Assets/DCL/Chat/_Refactor/ChatConfig/Resources_moved/ChatReactions/ChatReactionsMessageConfig.asset
mikhail-dcl
left a comment
mikhail-dcl
left a comment
There was a problem hiding this comment.
I reviewed the first part only, will continue later
| reactionsPresenter, | ||
| messageReactionService, | ||
| tooltipPresenter, | ||
| msgId => currentChannelService.CurrentChannel?.GetReactions(msgId)); |
There was a problem hiding this comment.
Can you pass currentChannelService directly instead of wrapping it in a delegate?
There was a problem hiding this comment.
Do you really need this interface?
| reactionsPresenter.ShowForMessage( | ||
| anchor, | ||
| chatEntryView.IsSentByOwnUser, | ||
| atlasIndex => messageReactionService.ToggleReaction(messageId, atlasIndex), |
There was a problem hiding this comment.
These delegates could be wired only once instead of passing and allocate them on every time. The only thing you need a capture for is messageId that you could save in the reactionsPresenter
| { | ||
| if (pendingReactionMessageId == null) return; | ||
|
|
||
| pendingReactionChatEntry?.messageBubbleElement.reactionButtonHoverView?.SetClicked(false); |
There was a problem hiding this comment.
It feels erroneous to intervene into the internals of the view element and, especially, control it from multiple places:
I would recommend splitting responsibilities somehow so from the code perspective it will be clearly visible which view/presenter is responsible for which part of the view elements and they do not overlap.
I am also concerned that now it's not part of any state, that makes things more obscure
| scrollToBottomPresenter.RequestScrollAction += OnRequestScrollAction; | ||
| chatHistory.MessageAdded += OnMessageAddedToChatHistory; | ||
|
|
||
| if (currentChannelService.CurrentChannel != null) |
There was a problem hiding this comment.
It can't be null according to the nullable notation
|
|
||
| private ChatReactionsAtlasConfig? atlasConfig; | ||
| private string? ownWalletAddress; | ||
| private ChatReactionsMessageConfig? messageConfig; |
| public class ChatReactionsAtlasConfig : ScriptableObject | ||
| { | ||
| [field: Note("The emoji sprite sheet texture. Must have Read/Write enabled in import settings.")] | ||
| [field: SerializeField] public Texture2D Atlas { get; private set; } |
There was a problem hiding this comment.
How this atlas is generated? Manually?
| return result; | ||
| } | ||
|
|
||
| public Rect GetUVRect(int tileIndex) |
There was a problem hiding this comment.
Is it necessary to handle UV stuff manually? Isn't it provided from anything like TMP_Sprite Asset?
| /// Views are reparented under a hidden root when returned and | ||
| /// reparented under a chat entry when acquired. | ||
| /// </summary> | ||
| public class MessageReactionsViewPool |
| private int emojiIndex; | ||
| private bool hiding; | ||
|
|
||
| public event Action<int>? Clicked; |
There was a problem hiding this comment.
Maybe it's better to use ViewEventBus than popping events through several layers?
4 participants
Pull Request Description
Summary
Adds a full emoji reaction system to the chat, supporting two modes:
The system uses GPU-instanced particle rendering, spring-force physics for world-space particles, network debouncing over LiveKit, and an MVP presenter layer for the UI.
Architecture
ChatReactions/
├── Configs/ ScriptableObject configs (atlas, UI/world lane tuning, debounce timing, tooltip positioning)
├── Core/ Orchestration layer
│ ├── ChatReactionsFactory Composition root — wires all dependencies
│ ├── SituationalReactionFacade Main API: trigger → broadcast + spawn
│ ├── SituationalSimulationLoop Per-frame tick: physics, network, rendering
│ ├── SituationalReactionDebouncer Batches outgoing reactions to reduce network traffic
│ ├── RemoteReactionReceiver Staggered dequeue for visual cascade of incoming reactions
│ ├── StreamReactionsChatCommand /streamreactions debug command — continuous test stream
│ ├── StreamReactionsEmitter Drives the stream emission loop with rate limiting
│ ├── FakeReactionsChatCommand /fakereaction debug command — single test reaction
│ ├── TokenBucketRateLimiter Rate-limits outgoing stream reactions
│ ├── ChatReactionRecentsService Tracks recently used emoji for selector bar
│ └── EmojiCodepointHelper Emoji ↔ index resolution utilities
├── Networking/ Message bus + routing
│ ├── IReactionMessageBus / MultiplayerReactionMessageBus LiveKit integration
│ ├── ReactionRouter Dispatches by type (situational vs message)
│ ├── ReactionNetworkBroadcaster Debounced network flush
│ ├── ReactionWireEncoding Packs emoji index + removal flag into single int
│ ├── ReactionChannelRouting Channel selection logic for reaction payloads
│ └── ChatMessageReactionService Toggle/persist reactions on chat messages
├── Presenters/ MVP presenters
│ ├── ChatReactionsPresenter Top-level: button + selector bars + emoji panel coordination
│ ├── ChatReactionsSelectorPresenter Emoji shortcuts (recents + defaults)
│ ├── ChatReactionButtonPresenter Individual reaction button logic
│ ├── SituationalReactionPresenter Bridges situational trigger to UI
│ ├── EmojiPanelReactionBridge Bridges full emoji panel for selection
│ └── ReactionPanelPositioner Positions selector relative to message anchors
├── Simulation/ Particle physics + storage
│ ├── DenseParticleStore Dense array with lazy compaction (allocation-free ticks)
│ ├── ParticleIntegrator Velocity integration, drag, gravity, expiration
│ ├── UI/ Screen-space: flight paths, zigzag steering, spawn resolver, stream emitter
│ └── World/ World-space: avatar anchor tracking, spring forces, frustum culling
├── Rendering/ GPU-instanced particle rendering
│ ├── ChatReactionsParticleRenderer Batches ≤1023 particles/draw, screen↔world conversion
│ ├── ChatReactionsAtlasHelper Per-emoji UV lookup from atlas config
│ └── ChatReactionMaterialFactory Creates/caches instanced materials for particle draws
├── Views/ Thin view components (button, selector bar, item, hover effect, situational view)
├── Debug/ Debug panel, event bus, mock message bus, stats window
├── Editor/ Custom inspectors, atlas preview, debug prefs
└── Tests/ 15 test files, 118 test cases
Data Flow
Local reaction:
Button click → ChatReactionsPresenter → SituationalReactionFacade
├→ ChatReactionUISimulation.SpawnBurst() (screen-space particles)
├→ LocalPlayerWorldReactor.TriggerBurst() (world-space above local avatar)
└→ ReactionNetworkBroadcaster.Enqueue() (debounced LiveKit send)
Remote reaction:
LiveKit message → ReactionRouter
├→ [Situational] RemoteReactionReceiver → stagger → world spawn above sender's avatar
└→ [Message] ChatMessageReactionService → toggle reaction in chat history
Shape
Design
Frame debugger
Debug config
What does this PR change?
Adds chat reactions feature
Test Instructions
Test Steps
Additional Testing Notes
Quality Checklist
Code Review Reference
Please review our Code Review Standards before submitting.