Repository navigation
Conversation
Test this pull request
|
decentraland-bot
left a comment
There was a problem hiding this comment.
Review: PlayerVoiceState (1224) + VoiceChatModifierArea (1225)
The schema looks good. Both components are additive (no breaking change for existing consumers), CI is green and IDs 1224/1225 don't collide with any open PR (#495 holds 1223, #377 uses 1217). The naming follows existing components: area/exclude_ids match AvatarModifierArea, is_speaking matches is_guest, and the Player* prefix matches PlayerIdentityData. Flat optional fields are the right call for future modifiers, since a repeated enum like AvatarModifierType can't carry parameters.
The proto is the cross-client contract (Unity, Bevy, Godot), so the semantics need to be pinned down before clients implement them independently. Inline comments have the details.
P1: must fix before merge
- The rule for combining
isolateis undefined. The text says "most restrictive value wins" and "the effect follows the speaker", butisolatealso restricts listeners. Nothing says how isolation combines across overlapping regions, or whether an isolated listener still hears speakers who are in another, non-isolating region they also stand in. - There is no rule for out-of-range or non-finite values. Nothing covers
volume_scale> 1 (boosting a voice for every listener is a griefing vector), < 0 or NaN, ormax_distance< 0.1, negative or Inf. NaN also breaks any min/max "most restrictive" comparison, so a NaN region could cancel a mute set by an overlapping one. PlayerVoiceState's scope and source are unspecified. Combined withPlayerIdentityData.address, a scene can log who spoke when and send it out withfetch. The docs should say which players are reported (only those inside the scene's parcels?) and that the value reflects audio actually transmitted to Nearby, not raw mic detection while muted or idle on push-to-talk.
P2: suggestions
- Line 18: listeners standing outside the scene hear speakers inside a
mute/isolateregion unmodified. Say so explicitly, and note that enforcement happens on each listener's client, so this is not a privacy boundary. - Say whether
areais clipped to the scene's parcels, so a region can't affect speakers who are standing in a neighbouring scene. - Lines 23-24: say what happens when the entity does carry another area component (ignored? first one wins?).
muteoverlaps withvolume_scale = 0. Say whether it's a shorthand or explain why both exist.exclude_ids: state the expected format (lowercase address, compared case-insensitively) and a maximum length.- Line 29: the comment is very long for one line. Move the range and default rules into the message header.
- Follow-up outside this repo:
components/README.mdrequires a component test in@dcl/ecs(js-sdk-toolchain). Please link it once it's open.
Consumer impact
The change is purely additive: two new components and two import public lines. No existing component, field or ID changes, and validate-compatibility passes.
Reviewed by Jarvis 🤖 · Requested by Gabriel Díaz (<@U03MGHMAJL8>) via Slack
| // The effect follows the speaker: it applies to the voice of players standing inside the region, | ||
| // for every listener who is also standing in this scene. When a player stands inside several | ||
| // regions, the most restrictive value of each property wins. An excluded player is treated as |
There was a problem hiding this comment.
[P1] "Most restrictive value of each property wins" fits volume_scale (min), max_distance (min) and mute (OR). It doesn't cover isolate, which is also a listener rule (line 31), so it contradicts "the effect follows the speaker". Please spell out:
- If a speaker is in isolating regions A and B, must the listener be in A, B, either, or both?
- Does an isolated listener still hear a speaker standing in a different, non-isolating region?
- Do two isolating regions that don't overlap form two separate rooms?
Without this, Unity, Bevy and Godot will each implement it differently.
There was a problem hiding this comment.
Fixed in 447b080. The header now spells out the combination rules: volume_scale and max_distance take the lowest value, mute applies if any region mutes, and for isolate a speaker in isolating regions is heard only by listeners in at least one of those same regions. A listener in an isolating region only hears speakers who share one of its isolating regions, so an isolated listener does not hear someone standing in a different, non-isolating region. Two separate isolating regions are two separate rooms. This matches the Explorer implementation in decentraland/unity-explorer#10419.
| message PBVoiceChatModifierArea { | ||
| decentraland.common.Vector3 area = 1; // the 3D size of the region | ||
| repeated string exclude_ids = 2; // user IDs that can enter and remain unaffected | ||
| optional float volume_scale = 3; // voice volume in [0, 1] (default: 1) |
There was a problem hiding this comment.
[P1] Please define what clients do with values outside [0, 1] and with NaN or ±Inf. Without that, volume_scale: 10 could boost a speaker for every listener in the scene. Also, NaN makes min-based "most restrictive" comparisons return false, which can cancel a limit set by an overlapping region. Suggested wording: "Clients clamp to [0, 1]; NaN/±Inf are treated as unset."
There was a problem hiding this comment.
Fixed in 447b080: volume_scale is clamped to [0, 1], and NaN and infinite values are treated as unset. The Explorer now does the same (decentraland/unity-explorer#10419, with a test for NaN, +Inf and -Inf), so a NaN region can't cancel a mute or a limit from an overlapping one.
| decentraland.common.Vector3 area = 1; // the 3D size of the region | ||
| repeated string exclude_ids = 2; // user IDs that can enter and remain unaffected | ||
| optional float volume_scale = 3; // voice volume in [0, 1] (default: 1) | ||
| optional float max_distance = 4; // meters at which the voice fades out, e.g. 3 to whisper; at least 0.1, and values beyond the client's Nearby range have no effect (default: the client's Nearby range) |
There was a problem hiding this comment.
[P1] "At least 0.1" doesn't say whether a lower value is clamped up to 0.1 or ignored, and it doesn't cover negative values, NaN or Inf. Suggested wording: "clamped to [0.1, Nearby range]; NaN/±Inf treated as unset". [P2] This comment is also very long; consider moving the range and default rules into the message header.
There was a problem hiding this comment.
Fixed in 447b080: lower values are raised to 0.1, values beyond the client's Nearby range have no effect, and NaN or infinite values are treated as unset. I moved the range and default rules into the message header, so the field comments are short now.
| // PlayerVoiceState reports whether a player is currently speaking in Nearby voice chat. | ||
| // It is written by the engine on player entities (including `engine.PlayerEntity`) and only | ||
| // reflects Nearby voice chat: private and community calls are never reported. | ||
| message PBPlayerVoiceState { | ||
| bool is_speaking = 1; // true while the player's voice activity is detected |
There was a problem hiding this comment.
[P1] Please define the scope and source. Read alongside PlayerIdentityData.address, this lets a scene build a per-wallet speaking timeline and send it out with fetch. Suggested additions:
- which players get it: only those inside this scene's parcels, like other player components?
is_speakingis true only while audio is actually being transmitted to Nearby (mic on, not muted), never from local mic input that isn't sent. This matters forengine.PlayerEntity.- the expected update cadence or debounce, so scenes don't build on a flickering signal.
There was a problem hiding this comment.
Fixed in 447b080. It's only written on the entities of players standing on this scene's parcels, including engine.PlayerEntity while the local player is there, so a scene never sees speaking state for players elsewhere. is_speaking comes from the voice server's active-speaker detection on audio actually transmitted to Nearby, so it's never true with the mic off, muted or idle on push-to-talk. The doc also notes that it updates when the server reports a change (usually within a second) and can toggle between words, so scenes should debounce if they need a steady signal.
…speaking-state source
|
Thanks for the review. All three P1s are addressed in 447b080 (replies inline), and here's how I handled the P2s:
|
decentraland-bot
left a comment
There was a problem hiding this comment.
Re-review: 447b080
All three P1s from the previous review are resolved:
- ✅ Combining overlapping regions: each field now has an explicit rule.
volume_scale/max_distancetake the lowest value,muteapplies if any region mutes, andisolateuses shared-region membership in both directions (separate isolating regions are separate rooms). The rules are consistent with "the effect follows the speaker" and with exclusions. - ✅ Value limits:
volume_scaleis clamped to [0, 1],max_distanceis raised to at least 0.1, and NaN/±Inf are treated as unset. That closes both the volume-boost path and the NaN-cancels-mute path. - ✅
PlayerVoiceStatescope and source: it's only written for players on the scene's parcels, it comes from server-side active-speaker detection on transmitted audio (never true while muted or idle on push-to-talk), and the update cadence and debounce advice are documented.
The P2s are handled too: client-side enforcement is documented as not a privacy boundary, regions are clipped to the scene's parcels, mute vs volume_scale 0 is explained, the exclude_ids format is given, and the field comments are short. The @dcl/ecs tests are in decentraland/js-sdk-toolchain#1647. The reasoning for not capping exclude_ids is acceptable.
Remaining P2s (non-blocking)
- "Only one of them takes effect" when the entity also has another trigger-area component doesn't say which one, so clients could differ. Either name the winner, or say the result is undefined and must be avoided.
areastill has no rule for negative, zero, NaN or infinite sizes. This gap is inherited fromAvatarModifierArea, so it could be a separate follow-up across all area components.
The change is still purely additive, IDs 1224/1225 are still unique, and CI is green (3/3).
Reviewed by Jarvis 🤖 · Requested by Gabriel Díaz (<@U03MGHMAJL8>) via Slack
| // If the entity also carries another component that uses a trigger region (AvatarModifierArea, | ||
| // CameraModeArea, TriggerArea), only one of them takes effect: put each on its own entity. |
There was a problem hiding this comment.
[P2] "Only one of them takes effect" leaves the winner unspecified, so the Unity, Bevy and Godot clients could each pick a different one. Consider naming it, or saying "the result is undefined; put each on its own entity".
|
as ModifierAreas for the Voice Chat were declined by design for the better architecture for voice sound effects, we close this PR and go with the smaller approach here #501 |
Brings `optional bool talking = 6` on `PBAvatarBase` from `experimental-bevy` (#493) to `main`, so Unity can ship it too and scenes get one cross-engine signal. The engine writes it with the rest of the player's AvatarBase; it stays unset on engines that don't report voice activity, so a scene can tell "no data" from "not talking". Additive to `PBAvatarShape.talking` (scene-to-engine display flag). Same line as #472, so no conflict when that lands. Supersedes the `PlayerVoiceState` part of #499. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Lets scenes read and shape Nearby voice chat.
PlayerVoiceState(1224) — engine-written on player entities, includingengine.PlayerEntity.is_speakingis true while the player's voice is detected in Nearby voice chat. Private and community calls are never reported.VoiceChatModifierArea(1225) — a region that changes how players speaking inside it are heard:volume_scalein [0, 1],max_distancefor whispering,mute, andisolate(only listeners inside the region hear it, and they only hear speakers inside it).exclude_idskeeps chosen players unaffected. The effect follows the speaker, and when a player stands in several regions the most restrictive value of each property wins.This is voice-only and separate from the general audio effects proposed in #377 (
AudioEffectZone).How to test:
make buf-lint && make test;make list-components-idsshows 1224 and 1225 unique (1223 is claimed by #495).SDK and Explorer PRs will link here once open.
Explorer stack: decentraland/unity-explorer#10417 → #10418 → #10419 → #10420