Built-in voice chat#1284
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements a complete voice chat system: Opus-backed capture/encode/decode, per-player buffers and mixing (including proximity attenuation and panning), recording hooks in audio backends, network packet types and handlers for frames and mute updates, persistent per-address voice settings, UI panels and keybinds (mute/deafen/push-to-talk), Lua bindings/constants, HUD/nametag icons, textures, docs, and CI/Makefile changes to include Opus. Voice chat is force-disabled for public CoopNet lobbies. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Cool. But how does the voice chat work? Is it all just sending packets over coopnet? Wouldn't that be problematic? |
|
yeah it sends packets over coopnet, how is that problematic? |
I assumed it used a lot more bandwidth. But I saw you responded to AgentX about it in discord so nvm |
kermeow
left a comment
kermeow
left a comment
There was a problem hiding this comment.
I probably missed a few things. Otherwise, looks good so far! Excited to see if this goes anywhere.
| #include <stdbool.h> | ||
|
|
||
| #define PACKET_LENGTH 3000 | ||
| #define PACKET_LENGTH 4096 |
There was a problem hiding this comment.
Is there a reason this needed to be changed?
There was a problem hiding this comment.
I increased it when I was sending raw PCM data over the network and forgot to change it back
| #include <opus/opus.h> | ||
|
|
||
| #define INTERNAL_SAMPLE_RATE 48000 | ||
| #define SAMPLE_RATE 32000 |
There was a problem hiding this comment.
Could the output sample rate be increased for quality sake?
There was a problem hiding this comment.
This is actually the sample rate that's used in src/pc/audio/audio_sdl.c, I assume that changing this would break the game's audio system
| u8 frame_data[frame_size]; | ||
| packet_read(p, frame_data, frame_size); | ||
|
|
||
| proxchat_decode_audio(p->localIndex, frame_data, frame_size); |
There was a problem hiding this comment.
If I'm not mistaken, this only works on coopnet.
On the socket network system, clients ask the server to broadcast their packets for them, making this always the host's local index unless you are the host yourself. This works fine if there are only 2 clients, but doesn't behave correctly otherwise.
There was a problem hiding this comment.
Oh, what's the proper way of getting the index then?
There was a problem hiding this comment.
There isn't one afaik, you'd have to do it yourself as part of the packet data or disable outside of coopnet.
|
|
||
| // skip over player 0 because thats the client | ||
| for (s32 i = 1; i < MAX_PLAYERS; i++) { | ||
| if (!proxchat_is_ingame(i)) continue; |
There was a problem hiding this comment.
Skipping players in this way leaves their buffer non-empty if they leave or go to a different level. When you see them again, or a new player takes their index, this creates artifacts.
There was a problem hiding this comment.
Alright, will drain the buffer then
| float vol_left = 1 - pan_mono; | ||
|
|
||
| s16 player_pcm[num_samples]; | ||
| u32 n = buffer_read(&players[i].audio, sizeof(player_pcm), player_pcm) / sizeof(s16); |
There was a problem hiding this comment.
For incoming audio, it might be a good idea to avoid draining the buffer until at least however many frames have been decoded. Otherwise this is ample opportunity for stuttering.
It might also be a good idea to decode opus packets at this point rather than when received so that PLC and/or FEC can be implemented correctly later. A jitter buffer similar to what is used in other VOIP applications could be useful here.
| sum += abs(samples[i]); | ||
| } | ||
| avg = sum / num_samples; | ||
| proxchat_mic_level = 1 - powf(1 - avg / 32767.f, 10); |
There was a problem hiding this comment.
The difference is minimal, but why not use RMS here?
There was a problem hiding this comment.
I wasn't aware of it until now
| else proxchat_error[i] = PROXCHAT_ERR_NONE; | ||
|
|
||
| players[i].audio.capacity = FRAME_SIZE * sizeof(s16) * 4; | ||
| players[i].audio.dynamic = true; |
There was a problem hiding this comment.
I do not recommend keeping this enabled. In the case of lag, this will cause voices to become extremely delayed as the buffer will just grow to account for late packets.
Instead, a different way of buffering should probably be considered. See other comment about jitter buffers.
There was a problem hiding this comment.
Ah, I did intend on keeping this disabled but forgot to change it
| u32 size = proxchat_encode_audio(frame, sizeof(frame)); | ||
| if (size == 0) return; | ||
|
|
||
| packet_init(&p, PACKET_PROXCHAT_FRAME, false, PLMT_NONE); |
There was a problem hiding this comment.
Since the packets are not reliable or ordered, they need to be timestamped as there is no guarantee they will be received in order. This also ties in with the PLC and jitter buffer mentioned in other comments.
There was a problem hiding this comment.
Do I just switch the bool to true?
There was a problem hiding this comment.
I wouldn't recommend making them reliable, you should timestamp them yourself.
|
i'm hoping we get a proper lua api for this |
| if (cursorX < x) *value = min; | ||
| else if (cursorX > x + w) *value = max; | ||
| else *value = ((cursorX - x) / w) * (max - min) + min; |
There was a problem hiding this comment.
The previous code was prone to floating point errors, which could cause the slider to be stuck at min+1 or max-1. This new code ensures this doesn't happen.
…proximity-chat
|
Right I forgot, @coderabbitai you can review. |
|
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (4)
autogen/convert_functions.py (1)
85-85: ⚡ Quick winPrefer an explicit allowlist for
src/pc/voice_chat.hexports.Right now this header is guarded by a denylist only. New prototypes can become Lua-exposed unintentionally if they miss the disallow patterns. Consider adding
override_allowed_functions["src/pc/voice_chat.h"]and keeping the surface explicit.Also applies to: 150-150
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@autogen/convert_functions.py` at line 85, Add an explicit allowlist for the header "src/pc/voice_chat.h" by populating the override_allowed_functions mapping for that key instead of relying only on the denylist; update the convert_functions.py entry override_allowed_functions["src/pc/voice_chat.h"] with the exact set of Lua-exposed prototypes you want (keep the existing override_disallowed_functions/denylist as a safeguard), and mirror the same change at the other occurrence referenced (around the second mention at line ~150) so new prototypes do not become exposed implicitly.src/pc/voice_chat.c (1)
259-259: 💤 Low valueRedundant
sizeof(s16)in array declaration.
pcmis declared ass16 pcm[FRAME_SIZE * sizeof(s16)]which creates an array of 1920 elements (3840 bytes), butopus_decodeonly outputs up toFRAME_SIZE(960) samples. This wastes stack space.♻️ Suggested fix
- s16 pcm[FRAME_SIZE * sizeof(s16)]; + s16 pcm[FRAME_SIZE];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pc/voice_chat.c` at line 259, The pcm buffer is over-allocated due to using sizeof(s16) in the array length (s16 pcm[FRAME_SIZE * sizeof(s16)]), wasting stack space; change the declaration to allocate FRAME_SIZE samples (e.g., s16 pcm[FRAME_SIZE]) so opus_decode (which outputs up to FRAME_SIZE samples) writes into a correctly-sized buffer and avoid oversized stack allocation—update the pcm declaration near the opus_decode usage to remove the sizeof(s16) multiplier.src/pc/djui/djui_button.c (1)
4-38: ⚖️ Poor tradeoffType-safety concern: Accessing button->text unconditionally for both text and image buttons.
Lines 15, 22, 29, and 36 all access
button->text->baseto adjust child position. However, for image buttons created withdjui_image_button_create, thebutton->iconfield is set instead. Becausetextandiconare in a union, they share the same memory, so this happens to work ifDjuiTextandDjuiImagehavebaseat the same offset—but the code is semantically unclear and fragile. Consider checking the button type or refactoring to make the child access explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pc/djui/djui_button.c` around lines 4 - 38, The update function djui_button_update_style unsafely accesses button->text->base for positioning even when the button is an image button (created by djui_image_button_create) which sets button->icon; change djui_button_update_style to detect which child is present (e.g., check button->style or a boolean/type field used when creating image vs text buttons, or check if button->icon != NULL) and call djui_base_set_location on the correct child (button->text->base or button->icon->base) instead of always using button->text; update all four places (the branches that currently reference button->text->base) to use this conditional selection so the union usage is explicit and safe.src/pc/djui/djui_panel_voice_chat.c (1)
73-73: ConfirmsMicButtonscapacity is sufficient for currentMAX_PLAYERS(no overflow).
MAX_PLAYERSis defined as16(include/types.h:602).djui_panel_voice_chat_add_playersiteratesfor (i = 1; i < MAX_PLAYERS; i++)and, per connected player, writes two entries (sMicButtons[button_counter++]) (djui_panel_voice_chat_add_playersaround lines 121 and 150-151).- Worst case:
MAX_PLAYERS - 1 = 15players ⇒2 * 15 = 30entries, which fits insMicButtons[32](line 73).Optional: add a compile-time/static assertion tying
sMicButtonssizing to2 * (MAX_PLAYERS - 1)to prevent future overflow ifMAX_PLAYERSchanges.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pc/djui/djui_panel_voice_chat.c` at line 73, sMicButtons currently has 32 entries and is used by djui_panel_voice_chat_add_players which writes up to 2*(MAX_PLAYERS-1) entries; add a compile-time/static assertion to bind the sMicButtons array size to that expression to prevent overflow if MAX_PLAYERS changes. Locate the sMicButtons declaration (symbol: sMicButtons) and add a static assert (or `#if/`#error) that sizeof(sMicButtons)/sizeof(sMicButtons[0]) >= 2*(MAX_PLAYERS-1) so the build fails if the capacity is insufficient, referencing MAX_PLAYERS and djui_panel_voice_chat_add_players in the comment for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/lua/globals.md`:
- Line 166: The link `[VoicePlayer](structs.md#gVoicePlayers)` in
docs/lua/globals.md points to a nonexistent anchor; update the target to the
actual VoicePlayer section anchor (replace the `#gVoicePlayers` fragment with
the correct anchor, e.g. `#voiceplayer`) so the `VoicePlayer`/`gVoicePlayers`
reference correctly links to the VoicePlayer section in structs.md.
In `@Makefile`:
- Line 739: The macOS .app packaging step doesn't handle the newly always-linked
libopus, so update the packaging block that currently copies/rewrites SDL2 and
GLEW to also locate the linked libopus binary (referencing BACKEND_LDFLAGS and
libopus), copy it into the .app/Contents/Frameworks (or appropriate bundle lib
dir), and run install_name_tool to rewrite its install name and any dependent
rpaths to point to the in-bundle copy; mirror the same copy + install_name_tool
logic used for SDL2/GLEW so the app can launch on systems without a matching
local Opus install.
In `@src/pc/audio/audio_sdl.c`:
- Around line 72-82: The record functions audio_sdl_record_start and
audio_sdl_record_stop call SDL_PauseAudioDevice with capture_dev without
verifying it was opened; add a guard to check capture_dev != 0 (or an
appropriate invalid sentinel) before calling SDL_PauseAudioDevice and make the
functions a no-op (or log an error) when the device is not valid to avoid
passing 0 as a device ID; also consider guarding audio_sdl_record_callback if
you want to prevent setting callbacks when capture_dev is invalid, referencing
capture_dev, audio_sdl_record_start, audio_sdl_record_stop, and
audio_sdl_record_callback to locate the changes.
- Around line 47-51: audio_sdl_init currently treats SDL_OpenAudioDevice capture
failure (capture_dev == 0) as fatal and returns false, disabling all audio;
instead, treat capture failure as non-fatal: when capture_dev == 0 set
gVoicePlayer->error = VOICECHAT_ERR_NO_MICROPHONE and log the SDL_GetError() but
do NOT return false—mark capture as unavailable (e.g., leave capture_dev set to
0 or set a boolean like capture_enabled = false) and allow the rest of
audio_sdl_init to complete so playback device initialization still occurs;
update any places that assume capture is present to check the capture flag
before using it.
In `@src/pc/configfile.c`:
- Around line 614-619: voice_read currently dereferences tokens[1]..tokens[4]
without checking numTokens; add a guard at the start of voice_read to verify
tokens != NULL and numTokens >= 5 and return early (or log an error) if the
check fails, so you avoid out-of-bounds access when parsing malformed "voice:"
lines; keep the remainder of the function (calls to voice_list_get_or_create and
assignments to data->volume, data->is_muted, data->is_globally_muted) unchanged.
- Line 135: defaultConfigKeyMuteMic currently uses the same scancode (0x0032) as
defaultConfigKeyY causing both actions to trigger; update the initializer for
defaultConfigKeyMuteMic to a different scancode (e.g., the scancode for the M
key or any other unused key) and ensure the new value does not collide with
defaultConfigKeyY or any other defaultConfigKey* entries; change only the value
in the defaultConfigKeyMuteMic array and run a quick scan of other
defaultConfigKey* definitions to confirm uniqueness.
In `@src/pc/djui/djui_panel_host_settings.c`:
- Around line 83-87: The code currently overwrites the persisted configVoiceChat
when opening host settings for public CoopNet lobbies; instead, create a local
copy (e.g., tempVoiceChat) initialized from configVoiceChat, pass &tempVoiceChat
into djui_selectionbox_create so the UI can show/modify the transient value, and
use djui_base_set_enabled(&selVoiceChat->base, !is_public_coopnet_lobby()) to
disable the control for public lobbies; only write back to configVoiceChat when
the user explicitly saves/applies changes (not on panel creation).
In `@src/pc/nametags.c`:
- Around line 183-190: The call to dynos_texture_get(&texture) may fail and
leave TextureInfo uninitialized before passing it to
djui_hud_render_texture_interpolated; update the block that uses mic to check
the boolean return value of dynos_texture_get (calling DynOS_Tex_Get) and only
call djui_hud_render_texture_interpolated when dynos_texture_get returns true
(otherwise skip rendering or use a safe fallback), referencing the local
variable texture and the existing mic/dynos_texture_get usage to locate the code
to change.
In `@src/pc/network/packets/packet_voicechat.c`:
- Around line 25-36: network_receive_voicechat_frame currently reads an
untrusted frame_size from packet and allocates a VLA (u8 frame_data[frame_size])
causing a stack overflow risk; validate that frame_size is <= 4096 (the same max
used by network_send_voicechat_frame) and > 0 before allocating, return/abort
the packet on invalid sizes, then read into a fixed-size stack buffer (u8
frame_data[4096]) or a heap buffer sized to frame_size and pass the real size to
voicechat_decode_audio; keep using packet_read, network_local_index_from_global,
and voicechat_decode_audio but ensure you handle partial reads/errors from
packet_read and do not read/write out of bounds.
In `@src/pc/voice_chat.c`:
- Around line 428-438: Both functions validate only channel and then index
sVoiceChannels[channel].canHear[other_channel], allowing out-of-bounds access
when other_channel is invalid; fix by validating other_channel the same way as
channel before accessing: in voicechat_hear check other_channel >= 0 &&
other_channel < sNumVoiceChannels (return early if not) and in
voicechat_can_hear do the same (return false if not); keep the existing
voicechat_channel_grow(channel) call and only access
sVoiceChannels[channel].canHear[other_channel] after both indices are confirmed
valid.
- Around line 68-76: The buffer reorganization when reallocating wrapped data is
using the updated capacity and wrong lengths; capture the old capacity into a
local (e.g., old_capacity = buffer->capacity) before changing it, allocate
new_bytes from the new capacity, then memcpy the first chunk using length =
old_capacity - buffer->tail (bytes from tail to end of old allocation) and
memcpy the second chunk using length = buffer->head (bytes from start to head),
free buffer->bytes, set buffer->bytes = new_bytes, and finally update
buffer->tail = 0 and buffer->head = buffer->size; reference buffer->bytes,
buffer->capacity, buffer->tail, buffer->head, buffer->size and new_bytes when
making the fix.
---
Nitpick comments:
In `@autogen/convert_functions.py`:
- Line 85: Add an explicit allowlist for the header "src/pc/voice_chat.h" by
populating the override_allowed_functions mapping for that key instead of
relying only on the denylist; update the convert_functions.py entry
override_allowed_functions["src/pc/voice_chat.h"] with the exact set of
Lua-exposed prototypes you want (keep the existing
override_disallowed_functions/denylist as a safeguard), and mirror the same
change at the other occurrence referenced (around the second mention at line
~150) so new prototypes do not become exposed implicitly.
In `@src/pc/djui/djui_button.c`:
- Around line 4-38: The update function djui_button_update_style unsafely
accesses button->text->base for positioning even when the button is an image
button (created by djui_image_button_create) which sets button->icon; change
djui_button_update_style to detect which child is present (e.g., check
button->style or a boolean/type field used when creating image vs text buttons,
or check if button->icon != NULL) and call djui_base_set_location on the correct
child (button->text->base or button->icon->base) instead of always using
button->text; update all four places (the branches that currently reference
button->text->base) to use this conditional selection so the union usage is
explicit and safe.
In `@src/pc/djui/djui_panel_voice_chat.c`:
- Line 73: sMicButtons currently has 32 entries and is used by
djui_panel_voice_chat_add_players which writes up to 2*(MAX_PLAYERS-1) entries;
add a compile-time/static assertion to bind the sMicButtons array size to that
expression to prevent overflow if MAX_PLAYERS changes. Locate the sMicButtons
declaration (symbol: sMicButtons) and add a static assert (or `#if/`#error) that
sizeof(sMicButtons)/sizeof(sMicButtons[0]) >= 2*(MAX_PLAYERS-1) so the build
fails if the capacity is insufficient, referencing MAX_PLAYERS and
djui_panel_voice_chat_add_players in the comment for clarity.
In `@src/pc/voice_chat.c`:
- Line 259: The pcm buffer is over-allocated due to using sizeof(s16) in the
array length (s16 pcm[FRAME_SIZE * sizeof(s16)]), wasting stack space; change
the declaration to allocate FRAME_SIZE samples (e.g., s16 pcm[FRAME_SIZE]) so
opus_decode (which outputs up to FRAME_SIZE samples) writes into a
correctly-sized buffer and avoid oversized stack allocation—update the pcm
declaration near the opus_decode usage to remove the sizeof(s16) multiplier.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ba932249-302d-40e0-9d2e-62224f98938f
⛔ Files ignored due to path filters (12)
textures/segment2/custom_headphones.rgba16.pngis excluded by!**/*.pngtextures/segment2/custom_microphone.rgba16.pngis excluded by!**/*.pngtextures/segment2/custom_microphone_black_icon.rgba16.pngis excluded by!**/*.pngtextures/segment2/custom_microphone_black_icon_muted.rgba16.pngis excluded by!**/*.pngtextures/segment2/custom_microphone_icon.rgba16.pngis excluded by!**/*.pngtextures/segment2/custom_microphone_icon_muted.rgba16.pngis excluded by!**/*.pngtextures/segment2/custom_microphone_muted.rgba16.pngis excluded by!**/*.pngtextures/segment2/custom_microphone_red.rgba16.pngis excluded by!**/*.pngtextures/segment2/custom_microphone_red_icon.rgba16.pngis excluded by!**/*.pngtextures/segment2/custom_microphone_red_icon_muted.rgba16.pngis excluded by!**/*.pngtextures/segment2/custom_microphone_red_muted.rgba16.pngis excluded by!**/*.pngtextures/segment2/custom_microphone_warning.rgba16.pngis excluded by!**/*.png
📒 Files selected for processing (56)
Makefileautogen/convert_constants.pyautogen/convert_functions.pyautogen/convert_structs.pyautogen/lua_definitions/constants.luaautogen/lua_definitions/functions.luaautogen/lua_definitions/manual.luaautogen/lua_definitions/structs.luabin/custom_textures.cdata/dynos_mgr_builtin_externs.hdata/dynos_mgr_builtin_tex.cppdocs/lua/constants.mddocs/lua/functions-7.mddocs/lua/functions.mddocs/lua/globals.mddocs/lua/structs.mdlang/Czech.inilang/English.inisrc/game/hud.csrc/game/level_update.hsrc/pc/audio/audio_api.hsrc/pc/audio/audio_null.csrc/pc/audio/audio_sdl.csrc/pc/configfile.csrc/pc/configfile.hsrc/pc/djui/djui_button.csrc/pc/djui/djui_button.hsrc/pc/djui/djui_interactable.csrc/pc/djui/djui_panel_controls_extra.csrc/pc/djui/djui_panel_host_settings.csrc/pc/djui/djui_panel_options.csrc/pc/djui/djui_panel_pause.csrc/pc/djui/djui_panel_sound.csrc/pc/djui/djui_panel_voice_chat.csrc/pc/djui/djui_panel_voice_chat.hsrc/pc/djui/djui_selectionbox.csrc/pc/djui/djui_slider.csrc/pc/lua/smlua_cobject.csrc/pc/lua/smlua_cobject_autogen.csrc/pc/lua/smlua_cobject_autogen.hsrc/pc/lua/smlua_constants_autogen.csrc/pc/lua/smlua_functions_autogen.csrc/pc/nametags.csrc/pc/network/network.csrc/pc/network/network.hsrc/pc/network/network_player.csrc/pc/network/packets/packet.csrc/pc/network/packets/packet.hsrc/pc/network/packets/packet_join.csrc/pc/network/packets/packet_network_players.csrc/pc/network/packets/packet_voicechat.csrc/pc/network/voice_list.csrc/pc/network/voice_list.hsrc/pc/pc_main.csrc/pc/voice_chat.csrc/pc/voice_chat.h
| if (mic) { | ||
| struct TextureInfo texture; | ||
| dynos_texture_get(mic, &texture); | ||
| djui_hud_render_texture_interpolated(&texture, | ||
| e->prevPos[0] - prevHalfWidth - 36 * e->prevScale, prevNametagPosY, e->prevScale * 2, e->prevScale * 2, | ||
| nametag->pos[0] - currHalfWidth - 36 * nametag->scale, currNametagPosY, nametag->scale * 2, nametag->scale * 2 | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what dynos_texture_get returns and how it handles missing textures
ast-grep --pattern $'dynos_texture_get($$$) {
$$$
}'
# Search for the function signature and return behavior
rg -n -A 10 'bool dynos_texture_get|s32 dynos_texture_get|int dynos_texture_get'Repository: coop-deluxe/sm64coopdx
Length of output: 1852
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show dynos_texture_get implementation context
rg -n "bool dynos_texture_get\\(" -S .
rg -n -C 3 "DynOS_Tex_Get\\(" -S .
rg -n "struct TextureInfo" -S .
rg -n "djui_hud_render_texture_interpolated\\(" -S src data . | head -n 50
# Print the specific implementation areas if found
# (Use sed/awk to keep output small)
files=$(rg -l "DynOS_Tex_Get\\(" -S . || true)
for f in $files; do
echo "---- $f (first match context) ----"
rg -n -A 20 -B 5 "DynOS_Tex_Get\\(" "$f" || true
done
tfiles=$(rg -l "struct TextureInfo" -S . || true)
for f in $tfiles; do
echo "---- $f (TextureInfo definition) ----"
rg -n -A 30 -B 5 "struct TextureInfo" "$f" || true
done
nfiles=$(rg -l "djui_hud_render_texture_interpolated\\(" -S src data . || true)
for f in $nfiles; do
echo "---- $f (call sites near function) ----"
rg -n -C 3 "djui_hud_render_texture_interpolated\\(" "$f" || true
doneRepository: coop-deluxe/sm64coopdx
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) TextureInfo layout
rg -n "struct TextureInfo" include/types.h
# Print a small region around it
start=$(rg -n "struct TextureInfo" include/types.h | head -n1 | sed 's/:.*//')
echo "TextureInfo starts at $start"
sed -n "$((start-5)),$((start+80))p" include/types.h
# 2) Full DynOS_Tex_Get (focus on how failures handle aOutTexInfo)
rg -n "bool DynOS_Tex_Get\\(" data/dynos_mgr_tex.cpp
func_line=$(rg -n "bool DynOS_Tex_Get\\(" data/dynos_mgr_tex.cpp | head -n1 | sed 's/:.*//')
echo "DynOS_Tex_Get starts at $func_line"
sed -n "$((func_line-10)),$((func_line+140))p" data/dynos_mgr_tex.cpp
# 3) djui_hud_render_texture_interpolated uses of TextureInfo fields
rg -n "void djui_hud_render_texture_interpolated\\(" src/pc/djui/djui_hud_utils.c
hline=$(rg -n "void djui_hud_render_texture_interpolated\\(" src/pc/djui/djui_hud_utils.c | head -n1 | sed 's/:.*//')
echo "djui_hud_render_texture_interpolated starts at $hline"
sed -n "$((hline-10)),$((hline+120))p" src/pc/djui/djui_hud_utils.cRepository: coop-deluxe/sm64coopdx
Length of output: 12031
Unchecked return value from dynos_texture_get may pass uninitialized TextureInfo to rendering.
dynos_texture_get (DynOS_Tex_Get) can return false on missing/corrupt textures without populating the provided TextureInfo. In src/pc/nametags.c this return value is ignored, and the resulting (potentially uninitialized) texture is passed to djui_hud_render_texture_interpolated, which only null-checks the pointer and then reads fields.
🛡️ Proposed fix to check return value
if (mic) {
struct TextureInfo texture;
- dynos_texture_get(mic, &texture);
- djui_hud_render_texture_interpolated(&texture,
- e->prevPos[0] - prevHalfWidth - 36 * e->prevScale, prevNametagPosY, e->prevScale * 2, e->prevScale * 2,
- nametag->pos[0] - currHalfWidth - 36 * nametag->scale, currNametagPosY, nametag->scale * 2, nametag->scale * 2
- );
+ if (dynos_texture_get(mic, &texture)) {
+ djui_hud_render_texture_interpolated(&texture,
+ e->prevPos[0] - prevHalfWidth - 36 * e->prevScale, prevNametagPosY, e->prevScale * 2, e->prevScale * 2,
+ nametag->pos[0] - currHalfWidth - 36 * nametag->scale, currNametagPosY, nametag->scale * 2, nametag->scale * 2
+ );
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (mic) { | |
| struct TextureInfo texture; | |
| dynos_texture_get(mic, &texture); | |
| djui_hud_render_texture_interpolated(&texture, | |
| e->prevPos[0] - prevHalfWidth - 36 * e->prevScale, prevNametagPosY, e->prevScale * 2, e->prevScale * 2, | |
| nametag->pos[0] - currHalfWidth - 36 * nametag->scale, currNametagPosY, nametag->scale * 2, nametag->scale * 2 | |
| ); | |
| } | |
| if (mic) { | |
| struct TextureInfo texture; | |
| if (dynos_texture_get(mic, &texture)) { | |
| djui_hud_render_texture_interpolated(&texture, | |
| e->prevPos[0] - prevHalfWidth - 36 * e->prevScale, prevNametagPosY, e->prevScale * 2, e->prevScale * 2, | |
| nametag->pos[0] - currHalfWidth - 36 * nametag->scale, currNametagPosY, nametag->scale * 2, nametag->scale * 2 | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pc/nametags.c` around lines 183 - 190, The call to
dynos_texture_get(&texture) may fail and leave TextureInfo uninitialized before
passing it to djui_hud_render_texture_interpolated; update the block that uses
mic to check the boolean return value of dynos_texture_get (calling
DynOS_Tex_Get) and only call djui_hud_render_texture_interpolated when
dynos_texture_get returns true (otherwise skip rendering or use a safe
fallback), referencing the local variable texture and the existing
mic/dynos_texture_get usage to locate the code to change.
There was a problem hiding this comment.
Cannot happen. The strings and texture list that dynos_texture_get pulls from are both hardcoded.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Resolve the CodeRabbit review comment on the PR
gh pr view 1284 --repo coop-deluxe/sm64coopdx --json reviews,comments
# List comments by coderabbitai on this PR to find the comment ID to resolve
gh api graphql -f query='
{
repository(owner: "coop-deluxe", name: "sm64coopdx") {
pullRequest(number: 1284) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 5) {
nodes {
id
author { login }
body
url
}
}
}
}
}
}
}'Length of output: 130862
🏁 Script executed:
#!/bin/bash
# Resolve the review thread PRRT_kwDOKmv7Z86HOboB
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOKmv7Z86HOboB"}) {
thread {
id
isResolved
}
}
}'Length of output: 727
@Dominicentek, understood and noted! I wasn't able to auto-resolve the thread due to permission restrictions — you can dismiss it manually. I've recorded a learning so this won't be flagged again in future reviews for hardcoded builtin texture lookups.
✏️ Learnings added
Learnt from: Dominicentek
Repo: coop-deluxe/sm64coopdx PR: 1284
File: src/pc/nametags.c:183-190
Timestamp: 2026-06-05T18:13:44.467Z
Learning: In coop-deluxe/sm64coopdx, when `dynos_texture_get` is called with hardcoded string literals corresponding to textures registered via `define_builtin_tex` in `data/dynos_mgr_builtin_tex.cpp` (builtin textures), the return value check is unnecessary because lookup failure cannot occur for hardcoded builtin names. Do not flag unchecked return values of `dynos_texture_get` for hardcoded builtin texture name strings.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: Isaac0-dev
Repo: coop-deluxe/sm64coopdx PR: 1230
File: src/pc/lua/smlua_cobject.c:192-195
Timestamp: 2026-05-22T02:01:57.670Z
Learning: In the coop-deluxe/sm64coopdx codebase, follow the established convention that OOM/allocation failure will not occur. During code review, generally do not flag missing NULL/failure checks on allocation return values (e.g., results from malloc, growing_array_init, hmap_create, etc.). Only treat allocation-failure-guard omissions as issues when they contradict an explicit local safety requirement or existing error-handling pattern.
Learnt from: Isaac0-dev
Repo: coop-deluxe/sm64coopdx PR: 1240
File: src/game/rendering_graph_node.c:313-315
Timestamp: 2026-05-13T03:24:05.891Z
Learning: In `sm64coopdx` (coop-deluxe/sm64coopdx), `gBehaviorValues.InfiniteRenderDistance` is intentionally scoped only to object-level distance checks. The rendering pipeline in `src/game/rendering_graph_node.c` correctly uses `configDrawDistance == 6` directly (not `draw_distance_scalar_is_infinite()`) to control the far plane and depth culling, because those are rendering-level concerns that should not be gated by the behavior value. Do not flag this as inconsistency.
| if (buffer->bytes) { | ||
| u8* new_bytes = malloc(buffer->capacity); | ||
| memcpy(new_bytes, buffer->bytes + buffer->tail, (buffer->size - buffer->tail)); | ||
| memcpy(new_bytes + (buffer->size - buffer->tail), buffer->bytes, buffer->tail); | ||
| free(buffer->bytes); | ||
| buffer->bytes = new_bytes; | ||
| } | ||
| buffer->tail = 0; | ||
| buffer->head = buffer->size; |
There was a problem hiding this comment.
Buffer reorganization logic is incorrect for wrapped data.
When the buffer wraps around and needs to be reallocated, the memcpy calculations are wrong:
- Line 70 uses
buffer->size - buffer->tail, but should usebuffer->capacity - buffer->tail(bytes from tail to end of old allocation) - Line 71 uses
buffer->tail, but should usebuffer->headorbuffer->size - (buffer->capacity - buffer->tail)(bytes from start to head)
🐛 Proposed fix
if (buffer->bytes) {
u8* new_bytes = malloc(buffer->capacity);
- memcpy(new_bytes, buffer->bytes + buffer->tail, (buffer->size - buffer->tail));
- memcpy(new_bytes + (buffer->size - buffer->tail), buffer->bytes, buffer->tail);
+ u32 first_chunk = buffer->capacity - buffer->tail > buffer->size ? buffer->size : buffer->capacity - buffer->tail;
+ memcpy(new_bytes, buffer->bytes + buffer->tail, first_chunk);
+ if (buffer->size > first_chunk) {
+ memcpy(new_bytes + first_chunk, buffer->bytes, buffer->size - first_chunk);
+ }
free(buffer->bytes);
buffer->bytes = new_bytes;
}Wait, let me reconsider - we need the old capacity before it was modified on line 66. The current code modifies capacity before using it:
static void buffer_grow(struct VoiceBuffer* buffer, u32 bytes) {
if (buffer->dynamic && buffer->size + bytes > buffer->capacity) {
- buffer->capacity = buffer->size + bytes;
- if (buffer->capacity % 1024) buffer->capacity += 1024 - (buffer->capacity % 1024);
+ u32 old_capacity = buffer->capacity;
+ u32 new_capacity = buffer->size + bytes;
+ if (new_capacity % 1024) new_capacity += 1024 - (new_capacity % 1024);
+ buffer->capacity = new_capacity;
if (buffer->bytes) {
u8* new_bytes = malloc(buffer->capacity);
- memcpy(new_bytes, buffer->bytes + buffer->tail, (buffer->size - buffer->tail));
- memcpy(new_bytes + (buffer->size - buffer->tail), buffer->bytes, buffer->tail);
+ u32 first_chunk = old_capacity - buffer->tail;
+ if (first_chunk > buffer->size) first_chunk = buffer->size;
+ memcpy(new_bytes, buffer->bytes + buffer->tail, first_chunk);
+ if (buffer->size > first_chunk) {
+ memcpy(new_bytes + first_chunk, buffer->bytes, buffer->size - first_chunk);
+ }
free(buffer->bytes);
buffer->bytes = new_bytes;
}🧰 Tools
🪛 Cppcheck (2.20.0)
[warning] 70-70: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[error] 71-71: If memory allocation fails
(nullPointerArithmeticOutOfMemory)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pc/voice_chat.c` around lines 68 - 76, The buffer reorganization when
reallocating wrapped data is using the updated capacity and wrong lengths;
capture the old capacity into a local (e.g., old_capacity = buffer->capacity)
before changing it, allocate new_bytes from the new capacity, then memcpy the
first chunk using length = old_capacity - buffer->tail (bytes from tail to end
of old allocation) and memcpy the second chunk using length = buffer->head
(bytes from start to head), free buffer->bytes, set buffer->bytes = new_bytes,
and finally update buffer->tail = 0 and buffer->head = buffer->size; reference
buffer->bytes, buffer->capacity, buffer->tail, buffer->head, buffer->size and
new_bytes when making the fix.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pc/network/packets/packet_voicechat.c (2)
63-76:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard the global-mute path against unknown recipients.
globalIndexcomes from packet data, sonetwork_player_from_global_index(globalIndex)can returnNULLafter disconnects or on malformed packets. Line 75 then dereferencesreceiver->localIndex, which turns this into a remote crash path.Suggested fix
if (!sender) { if (p->orderedFromGlobalId == 0 && mask & VOICECHAT_MUTE_GLOBAL) voicechat_push_pending_global_mute(globalIndex); return; } + + if ((mask & VOICECHAT_MUTE_GLOBAL) && receiver == NULL) { + return; + } if (!sender->moderator && sender->globalIndex != 0) mask &= ~VOICECHAT_MUTE_GLOBAL;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pc/network/packets/packet_voicechat.c` around lines 63 - 76, The code reads receiver->localIndex without checking that network_player_from_global_index(globalIndex) returned non-NULL; update the global-mute branch in packet_voicechat.c (the logic around network_player_from_global_index, receiver, VOICECHAT_MUTE_GLOBAL, muted and gVoicePlayers) to early-return or skip applying clientMutedState when receiver is NULL to avoid dereferencing receiver; ensure the same existence check used for sender (i.e., test receiver != NULL) is applied before accessing receiver->localIndex and gVoicePlayers[receiver->localIndex].
51-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve the target player from
globalIndexbefore callingnetwork_send_to.
globalIndexis written onto the wire as a global player id, but this branch indexesgNetworkPlayerswith it directly. When global ids and local slots diverge, the mute packet goes to the wrong client.Suggested fix
- if (mask == VOICECHAT_MUTE_LOCAL) network_send_to(gNetworkPlayers[globalIndex].localIndex, &p); - else network_send(&p); + if (mask == VOICECHAT_MUTE_LOCAL) { + struct NetworkPlayer* receiver = network_player_from_global_index(globalIndex); + if (receiver == NULL) return; + network_send_to(receiver->localIndex, &p); + } else { + network_send(&p); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pc/network/packets/packet_voicechat.c` around lines 51 - 52, The mute branch must resolve the recipient's local slot from the global player id before calling network_send_to: instead of indexing gNetworkPlayers with the raw globalIndex, look up the player entry whose .globalIndex equals the provided globalIndex and use that entry's .localIndex (or call the existing helper that maps global->local if one exists) when calling network_send_to in the VOICECHAT_MUTE_LOCAL branch; ensure you handle the "not found" case (fall back to network_send or abort gracefully) and validate the localIndex is within bounds before calling network_send_to.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-coop.yaml:
- Line 21: The macOS jobs (build-macos-arm and build-macos-intel) do not install
Opus while the Makefile unconditionally links with -lopus, causing link or
bundle copy failures; add a step to install Opus in both macOS job setup
sections (e.g., run "brew install opus" or the equivalent Homebrew package) so
libopus.dylib is present before building and bundling.
In `@Makefile`:
- Around line 1568-1569: The install_name_tool commands use backticks around
`@executable_path/libopus.dylib` which causes shell command substitution and
failures; update the two lines that invoke install_name_tool (the one changing
the dependency in sm64coopdx and the one setting the id on libopus.dylib) to use
a literal string for the path instead of backticks, e.g. replace
`@executable_path/libopus.dylib` with '`@executable_path/libopus.dylib`' (or
without any quotes) so the path is passed literally to install_name_tool.
---
Outside diff comments:
In `@src/pc/network/packets/packet_voicechat.c`:
- Around line 63-76: The code reads receiver->localIndex without checking that
network_player_from_global_index(globalIndex) returned non-NULL; update the
global-mute branch in packet_voicechat.c (the logic around
network_player_from_global_index, receiver, VOICECHAT_MUTE_GLOBAL, muted and
gVoicePlayers) to early-return or skip applying clientMutedState when receiver
is NULL to avoid dereferencing receiver; ensure the same existence check used
for sender (i.e., test receiver != NULL) is applied before accessing
receiver->localIndex and gVoicePlayers[receiver->localIndex].
- Around line 51-52: The mute branch must resolve the recipient's local slot
from the global player id before calling network_send_to: instead of indexing
gNetworkPlayers with the raw globalIndex, look up the player entry whose
.globalIndex equals the provided globalIndex and use that entry's .localIndex
(or call the existing helper that maps global->local if one exists) when calling
network_send_to in the VOICECHAT_MUTE_LOCAL branch; ensure you handle the "not
found" case (fall back to network_send or abort gracefully) and validate the
localIndex is within bounds before calling network_send_to.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 29e617eb-b96e-43d5-866c-0644bf34d990
📒 Files selected for processing (11)
.github/workflows/build-coop.yamlMakefiledocs/lua/globals.mdsrc/pc/audio/audio_sdl.csrc/pc/configfile.csrc/pc/djui/djui_panel_host_settings.csrc/pc/djui/djui_panel_options.csrc/pc/djui/djui_panel_voice_chat.csrc/pc/network/packets/packet_voicechat.csrc/pc/voice_chat.csrc/pc/voice_chat.h
💤 Files with no reviewable changes (1)
- src/pc/voice_chat.h
✅ Files skipped from review due to trivial changes (2)
- src/pc/djui/djui_panel_options.c
- docs/lua/globals.md
🚧 Files skipped from review as they are similar to previous changes (4)
- src/pc/djui/djui_panel_host_settings.c
- src/pc/djui/djui_panel_voice_chat.c
- src/pc/configfile.c
- src/pc/voice_chat.c
|
could you make it so we can choose a microphone in the settings?, it doesn't detect the right one for me |
Yeah I have plans to do that |
|
Didn't realize that |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pc/audio/audio_sdl.c`:
- Around line 124-137: audio_sdl_record_start()/audio_sdl_record_stop()
currently return early when sMicrophoneDev == 0 and therefore do not update
sCurrentlyCapturing, causing audio_sdl_reopen_microphone() to mis-handle
hot-swapped mics; change both functions so they always set sCurrentlyCapturing
(true in audio_sdl_record_start, false in audio_sdl_record_stop) before
returning, and only call SDL_PauseAudioDevice or bail on sMicrophoneDev after
updating the flag (leave sCaptureCallback unchanged).
- Around line 63-89: The reopen helpers currently close the active device before
opening the replacement (see audio_sdl_reopen_speaker and
audio_sdl_reopen_microphone), which loses the working device if opening the new
one fails; change audio_sdl_reopen_microphone to the same swap-after-success
pattern used in audio_sdl_reopen_speaker: do not call
SDL_CloseAudioDevice(sMicrophoneDev) until after SDL_OpenAudioDevice succeeds,
store the newly opened handle in a temporary variable, update sActiveMicrophone
via audio_sdl_update_name, then close the old sMicrophoneDev and assign the temp
to sMicrophoneDev (and only then call SDL_PauseAudioDevice if
sCurrentlyCapturing) so the current device remains active on open failure.
- Around line 140-149: The device-listing function audio_sdl_list_devices can
call strcmp with a NULL sActiveSpeaker/sActiveMicrophone when audio_sdl_init
left the active name unset; guard the comparison by checking the appropriate
global (sActiveSpeaker or sActiveMicrophone) is non-NULL before calling strcmp
and only set *current when both strings[i] and the active name are non-NULL and
equal; update the comparison in audio_sdl_list_devices to perform a NULL check
on the chosen active name first (use the type to pick sActiveSpeaker vs
sActiveMicrophone).
In `@src/pc/configfile.c`:
- Around line 821-830: The code advances ptr by strlen(tokens[i]) even when
strncpy() truncated the copy, which lets ptr exceed option->maxStringLength and
makes the subsequent spaces calculation negative and memset() write out of
bounds; fix by computing the actual copied length (e.g., using
strnlen(tokens[i], option->maxStringLength - ptr - 1) or capturing the strncpy
size limit) and increment ptr by that copied length, clamp/ensure spaces is
non-negative and no larger than option->maxStringLength - ptr - 1 before calling
memset(), and use option->maxStringLength and tokens pointers to bound all
copies and pointer arithmetic.
In `@src/pc/djui/djui_panel_sound.c`:
- Around line 22-25: djui_panel_sound_device_change currently writes the
selected device into configAudioOutputDevice unconditionally; change it to call
gAudioApi->reopen_speaker(sAudioDevices[sCurrAudioDevice]) and check its return
value, and only when that call indicates success copy
sAudioDevices[sCurrAudioDevice] into configAudioOutputDevice (using the existing
MAX_AUDIO_DEVICE_LENGTH limit and strncpy), otherwise leave
configAudioOutputDevice unchanged and handle the failure path (e.g., early
return or error log) so a failed reopen does not persist the bad device.
In `@src/pc/network/packets/packet_voicechat.c`:
- Around line 64-79: The packet trusts the wire value sender_index for
permissions; instead resolve the actor from the transport via p->localIndex and
use that NetworkPlayer for authorization: call
network_player_from_global_index(p->localIndex) (or similar) to get the
authenticated sender, reject or overwrite the payload sender_index if it doesn't
match the transport identity, and use that resolved sender when checking
sender->moderator and VOICECHAT_MUTE_GLOBAL (while still using the payload
receiver_index and mask for the rest); also ensure
voicechat_push_pending_global_mute(receiver_index) is only reachable when the
authenticated transport is allowed to perform global mutes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1ff26ce2-eb21-44c0-b3c9-14fb052b9886
📒 Files selected for processing (15)
.github/workflows/build-coop.yamlMakefilelang/Czech.inilang/English.inisrc/pc/audio/audio_api.hsrc/pc/audio/audio_null.csrc/pc/audio/audio_sdl.csrc/pc/configfile.csrc/pc/configfile.hsrc/pc/djui/djui_panel_client_server_settings.csrc/pc/djui/djui_panel_sound.csrc/pc/djui/djui_panel_voice_chat.csrc/pc/network/packets/packet_voicechat.csrc/pc/pc_main.csrc/pc/voice_chat.c
✅ Files skipped from review due to trivial changes (1)
- lang/English.ini
🚧 Files skipped from review as they are similar to previous changes (7)
- Makefile
- src/pc/configfile.h
- src/pc/audio/audio_api.h
- lang/Czech.ini
- src/pc/pc_main.c
- src/pc/djui/djui_panel_voice_chat.c
- src/pc/voice_chat.c
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/pc/audio/audio_sdl.c (1)
136-145:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInitialize
*currentbefore scanning the device list.When the active choice is the system default,
audio_sdl_reopen_*()stores the localized"System default"label, which will never equalSDL_GetAudioDeviceName(). In that path this function leaves*currentuntouched, so callers get a stale index and can preselect the wrong device.🛠️ Minimal fix
static char** audio_sdl_list_devices(int* count, unsigned int* current, int type) { int num = SDL_GetNumAudioDevices(type); if (count) *count = num; + if (current) *current = 0; - char* curr_name = type == AudioDevType_Microphone ? sActiveMicrophone : sActiveSpeaker; + char* curr_name = type == AudioDevType_Microphone ? sActiveMicrophone : sActiveSpeaker; char** strings = malloc(num * sizeof(const char*)); for (int i = 0; i < num; i++) { strings[i] = strdup(SDL_GetAudioDeviceName(i, type)); if (curr_name && strcmp(strings[i], curr_name) == 0) if (current) *current = i;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pc/audio/audio_sdl.c` around lines 136 - 145, In audio_sdl_list_devices, initialize the output index before scanning so callers don't get a stale value when the active name is the localized "System default"; specifically, after setting curr_name and before the for loop set if (current) *current = (unsigned int)-1 (or another sentinel like UINT_MAX) so that no match leaves current in a known "not found" state; keep the rest of the loop logic (strcmp against SDL_GetAudioDeviceName and assigning current) unchanged and reference the function audio_sdl_list_devices, variables curr_name/sActiveMicrophone/sActiveSpeaker, and the call to SDL_GetAudioDeviceName.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pc/audio/audio_sdl.c`:
- Around line 81-82: The code wrongly stores the reopened microphone name into
sActiveSpeaker, clobbering the speaker label and leaving
audio_sdl_active_microphone() returning NULL; change the assignment so you free
the old sActiveMicrophone (if non-NULL) and strdup the new name (name ?:
DLANG(SOUND, SYSTEM_DEFAULT)) into sActiveMicrophone instead of sActiveSpeaker,
leaving sActiveSpeaker untouched.
---
Duplicate comments:
In `@src/pc/audio/audio_sdl.c`:
- Around line 136-145: In audio_sdl_list_devices, initialize the output index
before scanning so callers don't get a stale value when the active name is the
localized "System default"; specifically, after setting curr_name and before the
for loop set if (current) *current = (unsigned int)-1 (or another sentinel like
UINT_MAX) so that no match leaves current in a known "not found" state; keep the
rest of the loop logic (strcmp against SDL_GetAudioDeviceName and assigning
current) unchanged and reference the function audio_sdl_list_devices, variables
curr_name/sActiveMicrophone/sActiveSpeaker, and the call to
SDL_GetAudioDeviceName.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c261fa7f-2dc0-45a0-ac9a-c30f31256329
📒 Files selected for processing (11)
.github/workflows/build-coop.yamllang/Czech.inilang/English.inisrc/pc/audio/audio_sdl.csrc/pc/configfile.csrc/pc/djui/djui_panel_client_server_settings.csrc/pc/djui/djui_panel_host_settings.csrc/pc/djui/djui_panel_pause.csrc/pc/djui/djui_panel_sound.csrc/pc/djui/djui_panel_voice_chat.csrc/pc/network/packets/packet_voicechat.c
✅ Files skipped from review due to trivial changes (1)
- lang/English.ini
🚧 Files skipped from review as they are similar to previous changes (8)
- src/pc/djui/djui_panel_client_server_settings.c
- src/pc/djui/djui_panel_pause.c
- src/pc/djui/djui_panel_sound.c
- src/pc/djui/djui_panel_host_settings.c
- lang/Czech.ini
- src/pc/network/packets/packet_voicechat.c
- src/pc/configfile.c
- src/pc/djui/djui_panel_voice_chat.c
|
I saw this was no longer a draft but I do believe a few concerns from before have gone unaddressed. Has this been tested yet with more than 2 clients or even outside of the same computer? |
Yes, it has been tested with 5 people in a private lobby and it works well |
xLuigiGamerx
left a comment
xLuigiGamerx
left a comment
There was a problem hiding this comment.
Merge conflicts resolved
happens when i try to host with proximity chat, happened like twice and never again, don't know if its related (windows, opengl) |
|
would be nice if we could choose the talkign distance for proximity cvhat in server settings or something |
|
it would also be cool to have it so rooms muffle your voice and caps have effects on your voice, similarly to that old proximity chat mod, unless mods can apply modifiers to the voice |
thats outside of the scope of this patch. implementing that needs a bunch of other code and machinery, so it makes sense for it to be a separate patch. i aim to implement the basic functionality in this patch. |
5 participants
This PR adds a built-in voice and proximity chat system so that users don't have to rely on Mumble.
Disabled for public CoopNet lobbies.