Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Port] TTS #121

Merged
merged 6 commits into from
Dec 3, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Content.Client/Audio/ContentAudioSystem.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ public sealed partial class ContentAudioSystem : SharedContentAudioSystem
public const float LobbyMultiplier = 3f;
public const float InterfaceMultiplier = 2f;
public const float AnnouncerMultiplier = 3f;
public const float TTSMultiplier = 1f; // WD EDIT

public override void Initialize()
{
Expand Down
3 changes: 3 additions & 0 deletions Content.Client/Entry/EntryPoint.cs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using Content.Client._White.TTS;
using Content.Client.Administration.Managers;
using Content.Client.Changelog;
using Content.Client.Chat.Managers;
Expand Down Expand Up @@ -74,6 +75,7 @@ public sealed class EntryPoint : GameClient
[Dependency] private readonly ILogManager _logManager = default!;
[Dependency] private readonly JoinQueueManager _joinQueue = default!;
[Dependency] private readonly DiscordAuthManager _discordAuth = default!;
[Dependency] private readonly TTSManager _ttsManager = default!; // WD EDIT

public override void Init()
{
Expand Down Expand Up @@ -169,6 +171,7 @@ public override void PostInit()
_documentParsingManager.Initialize();
_joinQueue.Initialize();
_discordAuth.Initialize();
_ttsManager.Initialize(); // WD EDIT

_baseClient.RunLevelChanged += (_, args) =>
{
Expand Down
2 changes: 2 additions & 0 deletions Content.Client/IoC/ClientContentIoC.cs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using Content.Client._White.TTS;
using Content.Client.Administration.Managers;
using Content.Client.Changelog;
using Content.Client.Chat.Managers;
Expand Down Expand Up @@ -54,6 +55,7 @@ public static void Register()
collection.Register<ISharedPlaytimeManager, JobRequirementsManager>();
IoCManager.Register<JoinQueueManager>();
IoCManager.Register<DiscordAuthManager>();
IoCManager.Register<TTSManager>(); // WD EDIT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

🛠️ Refactor suggestion

The review comment is correct and should be implemented.

The codebase analysis reveals:

  1. TTSManager is used via dependency injection in multiple places
  2. The codebase consistently uses interfaces for managers (IChatManager, IParallaxManager, etc.)
  3. There are actually two TTSManager implementations (Client and Server) that could benefit from shared interface definitions
  4. The manager contains important business logic and external service integration that would benefit from mocking in tests

The original review suggestions are valid and align with the codebase's patterns. Creating an ITTSManager interface would:

  • Match the established pattern of manager interfaces in the codebase
  • Enable proper mocking for the HTTP-dependent TTS service
  • Allow sharing interface definitions between Client and Server implementations
  • Improve maintainability and testability
🔗 Analysis chain

Consider registering TTSManager with an interface.

Most other services in this file are registered with interfaces (e.g., IParallaxManager, IChatManager). Consider creating an ITTSManager interface to:

  • Improve testability through mocking
  • Enable easier implementation swapping
  • Follow the dependency inversion principle

Example implementation:

- IoCManager.Register<TTSManager>(); // WD EDIT
+ IoCManager.Register<ITTSManager, TTSManager>(); // WD EDIT

Additionally, since the PR mentions no API is available for testing:

  1. Ensure proper error handling is implemented in TTSManager
  2. Consider adding a mock implementation for testing purposes

Let's check if TTSManager is used with dependency injection elsewhere:


Let me check the actual usage of TTSManager in those files to better understand how it's being injected.


Let me check the TTSManager class implementation to see its current structure and determine if it's already implementing any interfaces.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for TTSManager usage in dependency injection
rg -l "TTSManager.*\[Dependency\]" || rg -l "\[Dependency\].*TTSManager"

Length of output: 147


Script:

#!/bin/bash
# Check TTSManager dependency injection usage in the found files
echo "=== Content.Server/_White/TTS/TTSSystem.cs ==="
rg -A 2 -B 2 "TTSManager.*\[Dependency\]|\[Dependency\].*TTSManager" "Content.Server/_White/TTS/TTSSystem.cs"

echo -e "\n=== Content.Client/Entry/EntryPoint.cs ==="
rg -A 2 -B 2 "TTSManager.*\[Dependency\]|\[Dependency\].*TTSManager" "Content.Client/Entry/EntryPoint.cs"

# Also check if TTSManager is already implementing any interface
ast-grep --pattern 'class TTSManager : $_'

Length of output: 1113


Script:

#!/bin/bash
# Find and show the TTSManager class implementation
echo "=== TTSManager Implementation ==="
fd -e cs TTSManager.cs -x cat {}

# Also check for any existing IManager interfaces in the codebase for consistency
echo -e "\n=== Existing IManager interfaces ==="
rg -l "interface I.*Manager"

Length of output: 8064

}
}
}
9 changes: 9 additions & 0 deletions Content.Client/Lobby/UI/HumanoidProfileEditor.xaml
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,15 @@
<Control HorizontalExpand="True"/>
<OptionButton Name="SexButton" HorizontalAlignment="Right" />
</BoxContainer>
<!--WD EDIT START-->
<!--TTS-->
<BoxContainer HorizontalExpand="True">
<Label Text="{Loc 'humanoid-profile-editor-voice-label'}" />
<Control HorizontalExpand="True"/>
<OptionButton Name="VoiceButton" HorizontalAlignment="Right" />
<Button Name="VoicePlayButton" Text="▶" MaxWidth="80" />
</BoxContainer>
<!--WD EDIT END-->
Comment on lines +83 to +91
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Based on the gathered information, I can now provide the final response:

Voice playback implementation needs error handling improvements

The current implementation lacks proper error handling for several critical scenarios:

  • No error handling for failed TTS API requests
  • No user feedback when voice playback fails
  • Missing validation for audio stream availability
  • Content.Client/_White/TTS/TTSManager.cs: Add error handling for failed network requests
  • Content.Client/_White/TTS/HumanoidProfileEditor.TTS.cs: Implement user feedback mechanism for playback failures
  • Content.Client/_White/TTS/TTSSystem.cs: Add validation for audio stream availability before playback
🔗 Analysis chain

Verify voice playback functionality.

Since the PR description mentions that testing wasn't possible due to API unavailability, we should ensure proper error handling is implemented.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for error handling in voice playback implementation

# Look for error handling in the voice playback implementation
rg -A 5 "VoicePlayButton" --type cs

# Check for TTSManager integration
rg -A 5 "TTSManager" --type cs

# Look for any exception handling related to TTS
ast-grep --pattern 'try {
  $$$
  tts$$$
  $$$
} catch ($$$) {
  $$$
}'

Length of output: 5134


Script:

#!/bin/bash
# Let's check the PlayTTS implementation and error handling

# Check the PlayTTS method implementation
rg -A 10 "PlayTTS\(\)" --type cs

# Look for error handling in TTS-related files
rg -A 5 "catch|error|Exception" Content.Client/_White/TTS/

# Check TTSSystem implementation for error handling
rg -A 10 "TTSSystem" --type cs --type-add cs:*.cs

Length of output: 6831


Script:

#!/bin/bash
# Let's check the TTSManager implementation for error handling during requests

# Check TTSManager's RequestTTS implementation
rg -A 15 "RequestTTS" --type cs

# Look for network-related error handling
rg -A 10 "NetManager|NetworkManager" Content.Client/_White/TTS/

# Check for any audio-related error handling
rg -A 10 "AudioComponent|IAudioManager" Content.Client/_White/TTS/

Length of output: 7512

<!-- Pronouns -->
<BoxContainer HorizontalExpand="True">
<Label Text="{Loc 'humanoid-profile-editor-pronouns-label'}" />
Expand Down
20 changes: 19 additions & 1 deletion Content.Client/Lobby/UI/HumanoidProfileEditor.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,14 @@ public HumanoidProfileEditor(

#endregion Sex

// WD EDIT START
#region Voice

InitializeVoice();

#endregion
// WD EDIT END

Comment on lines +152 to +159
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling for voice initialization.

The voice initialization block should include try-catch error handling to gracefully handle any initialization failures.

// WD EDIT START
#region Voice

+ try {
    InitializeVoice();
+ } catch (Exception ex) {
+    Logger.Error($"Failed to initialize TTS voices: {ex}");
+ }

#endregion
// WD EDIT END
📝 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.

Suggested change
// WD EDIT START
#region Voice
InitializeVoice();
#endregion
// WD EDIT END
// WD EDIT START
#region Voice
try {
InitializeVoice();
} catch (Exception ex) {
Logger.Error($"Failed to initialize TTS voices: {ex}");
}
#endregion
// WD EDIT END

#region Age

AgeEdit.OnTextChanged += args =>
Expand Down Expand Up @@ -636,6 +644,7 @@ public void SetProfile(HumanoidCharacterProfile? profile, int? slot)

UpdateNameEdit();
UpdateSexControls();
UpdateTTSVoicesControls(); // WD EDIT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Implementation of UpdateTTSVoicesControls() has proper error handling but needs improvement

The implementation shows that the method:

  1. Has null check for Profile
  2. Filters voices based on character's sex using CanHaveVoice
  3. Handles fallback to first valid voice if current selection is invalid

However, there are a few issues:

  • The code appears to have formatting issues with missing braces for event handler and loop
  • The method contains duplicate code for voice list population
  • There's no error handling for potential exceptions from prototype manager
🔗 Analysis chain

Verify the implementation of UpdateTTSVoicesControls().

The method is called after sex changes and during profile updates, suggesting that voice options may be sex-dependent. However:

  1. The implementation of this method is not visible in the changes
  2. Consider adding error handling in case the voice update fails

Also applies to: 1134-1134

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for the UpdateTTSVoicesControls implementation
ast-grep --pattern 'private void UpdateTTSVoicesControls() {
  $$$
}'

Length of output: 71


Script:

#!/bin/bash
# Let's try a broader search pattern to find the method implementation
rg -l "UpdateTTSVoicesControls" 

# Then get the content of files containing this method
rg "UpdateTTSVoicesControls" -A 10

Length of output: 2924


Script:

#!/bin/bash
# Let's see the complete implementation of UpdateTTSVoicesControls
rg -A 20 "private void UpdateTTSVoicesControls\(\)" Content.Client/_White/TTS/HumanoidProfileEditor.TTS.cs

# Also check for VoiceButton declaration and _voiceList
rg "VoiceButton|_voiceList" Content.Client/_White/TTS/HumanoidProfileEditor.TTS.cs

Length of output: 1451

UpdateGenderControls();
UpdateSkinColor();
UpdateSpawnPriorityControls();
Expand Down Expand Up @@ -1122,10 +1131,19 @@ private void SetSex(Sex newSex)
}
UpdateGenderControls();
Markings.SetSex(newSex);
UpdateTTSVoicesControls(); // WD EDIT
ReloadProfilePreview();
SetDirty();
}

// WD EDIT START
private void SetVoice(string newVoice)
{
Profile = Profile?.WithVoice(newVoice);
IsDirty = true;
}
// WD EDIT END

Comment on lines +1146 to +1153
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add validation for voice parameter.

The SetVoice method accepts any string without validation. Consider adding checks to ensure:

  1. The voice parameter is not null or empty
  2. The voice value is a valid option from the available TTS voices
private void SetVoice(string newVoice)
{
+   if (string.IsNullOrEmpty(newVoice))
+       return;
+   
+   // Add validation to check if newVoice is in the list of available voices
+   if (!_availableVoices.Contains(newVoice))
+       return;
+       
    Profile = Profile?.WithVoice(newVoice);
    IsDirty = true;
}

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +1146 to +1153
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add input validation and documentation for SetVoice method.

The SetVoice method lacks parameter validation and XML documentation comments.

+ /// <summary>
+ /// Updates the profile's voice setting.
+ /// </summary>
+ /// <param name="newVoice">The new voice identifier to set. Must not be null or empty.</param>
private void SetVoice(string newVoice)
{
+    if (string.IsNullOrEmpty(newVoice))
+        throw new ArgumentException("Voice identifier cannot be null or empty", nameof(newVoice));
+
    Profile = Profile?.WithVoice(newVoice);
    IsDirty = true;
}
📝 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.

Suggested change
// WD EDIT START
private void SetVoice(string newVoice)
{
Profile = Profile?.WithVoice(newVoice);
IsDirty = true;
}
// WD EDIT END
// WD EDIT START
/// <summary>
/// Updates the profile's voice setting.
/// </summary>
/// <param name="newVoice">The new voice identifier to set. Must not be null or empty.</param>
private void SetVoice(string newVoice)
{
if (string.IsNullOrEmpty(newVoice))
throw new ArgumentException("Voice identifier cannot be null or empty", nameof(newVoice));
Profile = Profile?.WithVoice(newVoice);
IsDirty = true;
}
// WD EDIT END

private void SetGender(Gender newGender)
{
Profile = Profile?.WithGender(newGender);
Expand Down Expand Up @@ -1437,7 +1455,7 @@ private void UpdateWeight()
var avg = (Profile.Width + Profile.Height) / 2;
var weight = MathF.Round(MathF.PI * MathF.Pow(radius * avg, 2) * density);
WeightLabel.Text = Loc.GetString("humanoid-profile-editor-weight-label", ("weight", (int) weight));
}
}
else // Whelp, the fixture doesn't exist, guesstimate it instead
WeightLabel.Text = Loc.GetString("humanoid-profile-editor-weight-label", ("weight", (int) 71));

Expand Down
15 changes: 15 additions & 0 deletions Content.Client/Options/UI/Tabs/AudioTab.xaml
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,21 @@
<Label Name="AmbientMusicVolumeLabel" MinSize="48 0" Align="Right" />
<Control MinSize="4 0"/>
</BoxContainer>
<!--WD EDIT START-->
<BoxContainer Orientation="Horizontal" Margin="5 0 0 0">
<Label Text="{Loc 'ui-options-tts-volume'}" HorizontalExpand="True" />
<Control MinSize="8 0" />
<Slider Name="TtsVolumeSlider"
MinValue="0"
MaxValue="200"
HorizontalExpand="True"
Comment on lines +56 to +58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Normalize the volume slider maximum value.

The TTS volume slider has a maximum value of 200, while all other volume controls use 100. This inconsistency might confuse users and complicate volume management.

-                            MaxValue="200"
+                            MaxValue="100"
📝 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.

Suggested change
MinValue="0"
MaxValue="200"
HorizontalExpand="True"
MinValue="0"
MaxValue="100"
HorizontalExpand="True"

MinSize="80 0"
Rounded="True" />
<Control MinSize="8 0" />
<Label Name="TtsVolumeLabel" MinSize="48 0" Align="Right" />
<Control MinSize="4 0" />
</BoxContainer>
<!--WD EDIT END-->
Comment on lines +51 to +65
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

🛠️ Refactor suggestion

Volume slider range should be 100, not 200

The codebase evidence shows that the TTS volume slider value is divided by 100 in the code and uses a multiplier of 1.0, consistent with other volume controls:

_cfg.SetCVar(WhiteCVars.TtsVolume, TtsVolumeSlider.Value / 100f * ContentAudioSystem.TTSMultiplier);

The MaxValue should be changed to 100 to match:

  • The actual scaling used in the code (divided by 100)
  • The pattern used by all other volume sliders
  • The UI consistency across the audio settings
-                            MaxValue="200"
+                            MaxValue="100"
🔗 Analysis chain

Consider standardizing the volume slider range.

The TTS volume slider has a maximum value of 200, while all other volume sliders in the file use a maximum value of 100. This inconsistency might:

  • Create confusion for users expecting consistent scaling across all volume controls
  • Make it harder to maintain relative volume levels
  • Lead to unexpected behavior if the code assumes a standard range

Let's check if there's a specific reason for this range difference:

Consider one of these approaches:

  1. Standardize to match other sliders:
-                            MaxValue="200"
+                            MaxValue="100"
  1. If higher volume range is needed, add a comment explaining the requirement:
                             MaxValue="200"
+                            ToolTip="{Loc 'ui-options-tts-volume-extended-range-tooltip'}"
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for TTSMultiplier or volume-related constants
rg -A 2 "TTSMultiplier|VolumeMultiplier" 

Length of output: 3749

Comment on lines +51 to +65
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider aligning TTS volume range with other sliders.

While the implementation follows the UI pattern well, the TTS volume slider uses a range of 0-200 while all other volume sliders use 0-100. This inconsistency might confuse users.

Consider this change:

     <Slider Name="TtsVolumeSlider"
             MinValue="0"
-            MaxValue="200"
+            MaxValue="100"
             HorizontalExpand="True"
             MinSize="80 0"
             Rounded="True" />

If higher volume range is necessary for TTS, consider applying the amplification in the audio system rather than exposing it in the UI.

📝 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.

Suggested change
<!--WD EDIT START-->
<BoxContainer Orientation="Horizontal" Margin="5 0 0 0">
<Label Text="{Loc 'ui-options-tts-volume'}" HorizontalExpand="True" />
<Control MinSize="8 0" />
<Slider Name="TtsVolumeSlider"
MinValue="0"
MaxValue="200"
HorizontalExpand="True"
MinSize="80 0"
Rounded="True" />
<Control MinSize="8 0" />
<Label Name="TtsVolumeLabel" MinSize="48 0" Align="Right" />
<Control MinSize="4 0" />
</BoxContainer>
<!--WD EDIT END-->
<!--WD EDIT START-->
<BoxContainer Orientation="Horizontal" Margin="5 0 0 0">
<Label Text="{Loc 'ui-options-tts-volume'}" HorizontalExpand="True" />
<Control MinSize="8 0" />
<Slider Name="TtsVolumeSlider"
MinValue="0"
MaxValue="100"
HorizontalExpand="True"
MinSize="80 0"
Rounded="True" />
<Control MinSize="8 0" />
<Label Name="TtsVolumeLabel" MinSize="48 0" Align="Right" />
<Control MinSize="4 0" />
</BoxContainer>
<!--WD EDIT END-->

<BoxContainer Orientation="Horizontal" Margin="5 0 0 0">
<Label Text="{Loc 'ui-options-ambience-volume'}" HorizontalExpand="True" />
<Control MinSize="8 0" />
Expand Down
4 changes: 4 additions & 0 deletions Content.Client/Options/UI/Tabs/AudioTab.xaml.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Content.Client.Audio;
using Content.Shared._White;
using Content.Shared.CCVar;
using Robust.Client.Audio;
using Robust.Client.AutoGenerated;
Expand Down Expand Up @@ -40,6 +41,7 @@ public AudioTab()
LobbyVolumeSlider,
InterfaceVolumeSlider,
AnnouncerVolumeSlider,
TtsVolumeSlider, // WD EDIT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add TtsVolumeSlider to DetachUpdateChangesHandler

The TtsVolumeSlider is registered in AttachUpdateChangesHandler but missing from DetachUpdateChangesHandler. This inconsistency could lead to memory leaks or event handler issues.

Add TtsVolumeSlider to the DetachUpdateChangesHandler parameters:

 DetachUpdateChangesHandler(
     MasterVolumeSlider,
     MidiVolumeSlider,
     AmbientMusicVolumeSlider,
     AmbienceVolumeSlider,
     AmbienceSoundsSlider,
     LobbyVolumeSlider,
     InterfaceVolumeSlider,
     AnnouncerVolumeSlider,
+    TtsVolumeSlider,
     LobbyMusicCheckBox,
     RestartSoundsCheckBox,
     EventMusicCheckBox,
     AnnouncerDisableMultipleSoundsCheckBox,
     AdminSoundsCheckBox
 );

Also applies to: 82-95

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add TtsVolumeSlider to DetachUpdateChangesHandler.

The TtsVolumeSlider is added to AttachUpdateChangesHandler but missing from DetachUpdateChangesHandler. This asymmetry could lead to memory leaks as event handlers aren't properly cleaned up on disposal.

Add TtsVolumeSlider to DetachUpdateChangesHandler parameters:

             InterfaceVolumeSlider,
             AnnouncerVolumeSlider,
+            TtsVolumeSlider,

Committable suggestion skipped: line range outside the PR's diff.


LobbyMusicCheckBox,
RestartSoundsCheckBox,
Expand Down Expand Up @@ -125,6 +127,7 @@ private void OnApplyButtonPressed(BaseButton.ButtonEventArgs args)
_cfg.SetCVar(CCVars.LobbyMusicVolume, LobbyVolumeSlider.Value / 100f * ContentAudioSystem.LobbyMultiplier);
_cfg.SetCVar(CCVars.InterfaceVolume, InterfaceVolumeSlider.Value / 100f * ContentAudioSystem.InterfaceMultiplier);
_cfg.SetCVar(CCVars.AnnouncerVolume, AnnouncerVolumeSlider.Value / 100f * ContentAudioSystem.AnnouncerMultiplier);
_cfg.SetCVar(WhiteCVars.TtsVolume, TtsVolumeSlider.Value / 100f * ContentAudioSystem.TTSMultiplier); // WD EDIT

_cfg.SetCVar(CCVars.MaxAmbientSources, (int)AmbienceSoundsSlider.Value);

Expand All @@ -151,6 +154,7 @@ private void Reset()
LobbyVolumeSlider.Value = _cfg.GetCVar(CCVars.LobbyMusicVolume) * 100f / ContentAudioSystem.LobbyMultiplier;
InterfaceVolumeSlider.Value = _cfg.GetCVar(CCVars.InterfaceVolume) * 100f / ContentAudioSystem.InterfaceMultiplier;
AnnouncerVolumeSlider.Value = _cfg.GetCVar(CCVars.AnnouncerVolume) * 100f / ContentAudioSystem.AnnouncerMultiplier;
TtsVolumeSlider.Value = _cfg.GetCVar(WhiteCVars.TtsVolume) * 100f / ContentAudioSystem.TTSMultiplier; // WD EDIT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add TTS volume comparison to UpdateChanges

The TTS volume reset is implemented correctly, but the UpdateChanges method is missing the TTS volume comparison in the isEverythingSame check. This could lead to incorrect UI state updates where the Apply/Reset buttons don't reflect TTS volume changes.

Add the following code:

 var isAnnouncerVolumeSame =
     Math.Abs(AnnouncerVolumeSlider.Value - _cfg.GetCVar(CCVars.AnnouncerVolume) * 100f / ContentAudioSystem.AnnouncerMultiplier) < 0.01f;
+var isTtsVolumeSame =
+    Math.Abs(TtsVolumeSlider.Value - _cfg.GetCVar(WhiteCVars.TtsVolume) * 100f / ContentAudioSystem.TTSMultiplier) < 0.01f;

 var isAmbientSoundsSame = (int)AmbienceSoundsSlider.Value == _cfg.GetCVar(CCVars.MaxAmbientSources);
 // ... other comparisons ...
 var isEverythingSame = isMasterVolumeSame && isMidiVolumeSame && isAmbientVolumeSame
     && isAmbientMusicVolumeSame && isAmbientSoundsSame && isLobbySame && isRestartSoundsSame && isEventSame
     && isAnnouncerDisableMultipleSoundsSame && isAdminSoundsSame && isLobbyVolumeSame
-    && isInterfaceVolumeSame && isAnnouncerVolumeSame;
+    && isInterfaceVolumeSame && isAnnouncerVolumeSame && isTtsVolumeSame;

Also applies to: 189-195


AmbienceSoundsSlider.Value = _cfg.GetCVar(CCVars.MaxAmbientSources);

Expand Down
77 changes: 77 additions & 0 deletions Content.Client/_White/TTS/HumanoidProfileEditor.TTS.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
using System.Linq;
using Content.Client._White.TTS;
using Content.Shared.Preferences;
using Content.Shared._White.TTS;
using Robust.Shared.Random;

// ReSharper disable InconsistentNaming
// ReSharper disable once CheckNamespace
namespace Content.Client.Lobby.UI;

public sealed partial class HumanoidProfileEditor
{
private TTSSystem _ttsSystem = default!;
private TTSManager _ttsManager = default!;
private IRobustRandom _random = default!;

private List<TTSVoicePrototype> _voiceList = default!;

private readonly string[] _sampleText =
[
"Помогите, клоун насилует в технических тоннелях!",
"ХоС, ваши сотрудники украли у меня собаку и засунули ее в стиральную машину!",
"Агент синдиката украл пиво из бара и взорвался!",
"Врача! Позовите врача!"
];
Comment on lines +19 to +25
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve internationalization support for sample text.

The sample text is hardcoded in Russian, which limits internationalization. Consider:

  1. Moving these strings to localization resources
  2. Including samples in multiple languages
-    private readonly string[] _sampleText =
-    [
-        "Помогите, клоун насилует в технических тоннелях!",
-        "ХоС, ваши сотрудники украли у меня собаку и засунули ее в стиральную машину!",
-        "Агент синдиката украл пиво из бара и взорвался!",
-        "Врача! Позовите врача!"
-    ];
+    private string[] _sampleText => new[]
+    {
+        Loc.GetString("tts-sample-text-1"),
+        Loc.GetString("tts-sample-text-2"),
+        Loc.GetString("tts-sample-text-3"),
+        Loc.GetString("tts-sample-text-4")
+    };

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +19 to +25
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider improving sample text handling.

The current implementation has several concerns:

  1. Sample text is hardcoded in Russian, making it unsuitable for international users
  2. Some sample texts contain potentially sensitive content about violence

Consider:

  1. Moving sample texts to localization files
  2. Using more neutral content for samples
-    private readonly string[] _sampleText =
-    [
-        "Помогите, клоун насилует в технических тоннелях!",
-        "ХоС, ваши сотрудники украли у меня собаку и засунули ее в стиральную машину!",
-        "Агент синдиката украл пиво из бара и взорвался!",
-        "Врача! Позовите врача!"
-    ];
+    private readonly string[] _sampleTextKeys =
+    [
+        "tts-sample-text-1",
+        "tts-sample-text-2",
+        "tts-sample-text-3",
+        "tts-sample-text-4"
+    ];

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +19 to +25
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider revising sample text for internationalization and professionalism.

The current implementation has several concerns:

  1. Hardcoded Russian text limits internationalization
  2. The sample messages contain potentially inappropriate or distressing content
  3. Test phrases should be more neutral and professional

Consider implementing this alternative approach:

-    private readonly string[] _sampleText =
-    [
-        "Помогите, клоун насилует в технических тоннелях!",
-        "ХоС, ваши сотрудники украли у меня собаку и засунули ее в стиральную машину!",
-        "Агент синдиката украл пиво из бара и взорвался!",
-        "Врача! Позовите врача!"
-    ];
+    private string[] GetSampleText() => new[]
+    {
+        Loc.GetString("tts-sample-greeting"),
+        Loc.GetString("tts-sample-weather"),
+        Loc.GetString("tts-sample-announcement"),
+        Loc.GetString("tts-sample-emergency")
+    };

Committable suggestion skipped: line range outside the PR's diff.


private void InitializeVoice()
{
_random = IoCManager.Resolve<IRobustRandom>();
_ttsManager = IoCManager.Resolve<TTSManager>();
_ttsSystem = IoCManager.Resolve<IEntityManager>().System<TTSSystem>();
_voiceList = _prototypeManager.EnumeratePrototypes<TTSVoicePrototype>().Where(o => o.RoundStart).ToList();

VoiceButton.OnItemSelected += args =>
{
VoiceButton.SelectId(args.Id);
SetVoice(_voiceList[args.Id].ID);
};

VoicePlayButton.OnPressed += _ => { PlayTTS(); };
}

private void UpdateTTSVoicesControls()
{
if (Profile is null)
return;

VoiceButton.Clear();

var firstVoiceChoiceId = 1;
for (var i = 0; i < _voiceList.Count; i++)
{
var voice = _voiceList[i];
if (!HumanoidCharacterProfile.CanHaveVoice(voice, Profile.Sex))
continue;

var name = Loc.GetString(voice.Name);
VoiceButton.AddItem(name, i);

if (firstVoiceChoiceId == 1)
firstVoiceChoiceId = i;
}

var voiceChoiceId = _voiceList.FindIndex(x => x.ID == Profile.Voice);
if (!VoiceButton.TrySelectId(voiceChoiceId) && VoiceButton.TrySelectId(firstVoiceChoiceId))
SetVoice(_voiceList[firstVoiceChoiceId].ID);
}
Comment on lines +43 to +67
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix potential issues in voice selection logic.

There are several concerns in the voice control update logic:

  1. firstVoiceChoiceId is initialized to 1 but used with 0-based indices
  2. No handling for empty voice list
  3. No validation for null/empty voice IDs

Apply these fixes:

-        var firstVoiceChoiceId = 1;
+        var firstVoiceChoiceId = -1;  // Use -1 as sentinel value
         for (var i = 0; i < _voiceList.Count; i++)
         {
             var voice = _voiceList[i];
             if (!HumanoidCharacterProfile.CanHaveVoice(voice, Profile.Sex))
                 continue;

             var name = Loc.GetString(voice.Name);
             VoiceButton.AddItem(name, i);

-            if (firstVoiceChoiceId == 1)
+            if (firstVoiceChoiceId == -1)
                 firstVoiceChoiceId = i;
         }

+        if (firstVoiceChoiceId == -1)
+        {
+            Logger.Warning("No compatible voices found for profile");
+            return;
+        }
📝 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.

Suggested change
private void UpdateTTSVoicesControls()
{
if (Profile is null)
return;
VoiceButton.Clear();
var firstVoiceChoiceId = 1;
for (var i = 0; i < _voiceList.Count; i++)
{
var voice = _voiceList[i];
if (!HumanoidCharacterProfile.CanHaveVoice(voice, Profile.Sex))
continue;
var name = Loc.GetString(voice.Name);
VoiceButton.AddItem(name, i);
if (firstVoiceChoiceId == 1)
firstVoiceChoiceId = i;
}
var voiceChoiceId = _voiceList.FindIndex(x => x.ID == Profile.Voice);
if (!VoiceButton.TrySelectId(voiceChoiceId) && VoiceButton.TrySelectId(firstVoiceChoiceId))
SetVoice(_voiceList[firstVoiceChoiceId].ID);
}
private void UpdateTTSVoicesControls()
{
if (Profile is null)
return;
VoiceButton.Clear();
var firstVoiceChoiceId = -1; // Use -1 as sentinel value
for (var i = 0; i < _voiceList.Count; i++)
{
var voice = _voiceList[i];
if (!HumanoidCharacterProfile.CanHaveVoice(voice, Profile.Sex))
continue;
var name = Loc.GetString(voice.Name);
VoiceButton.AddItem(name, i);
if (firstVoiceChoiceId == -1)
firstVoiceChoiceId = i;
}
if (firstVoiceChoiceId == -1)
{
Logger.Warning("No compatible voices found for profile");
return;
}
var voiceChoiceId = _voiceList.FindIndex(x => x.ID == Profile.Voice);
if (!VoiceButton.TrySelectId(voiceChoiceId) && VoiceButton.TrySelectId(firstVoiceChoiceId))
SetVoice(_voiceList[firstVoiceChoiceId].ID);
}

Comment on lines +43 to +67
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix potential issue with voice selection logic.

The firstVoiceChoiceId initialization and usage could be improved:

  1. It's initialized to 1 but used as an array index
  2. The logic for finding the first valid voice is unclear

Consider this improved approach:

-        var firstVoiceChoiceId = 1;
+        var firstValidVoiceId = -1;
         for (var i = 0; i < _voiceList.Count; i++)
         {
             var voice = _voiceList[i];
             if (!HumanoidCharacterProfile.CanHaveVoice(voice, Profile.Sex))
                 continue;
 
             var name = Loc.GetString(voice.Name);
             VoiceButton.AddItem(name, i);
 
-            if (firstVoiceChoiceId == 1)
-                firstVoiceChoiceId = i;
+            if (firstValidVoiceId == -1)
+                firstValidVoiceId = i;
         }
 
         var voiceChoiceId = _voiceList.FindIndex(x => x.ID == Profile.Voice);
-        if (!VoiceButton.TrySelectId(voiceChoiceId) && VoiceButton.TrySelectId(firstVoiceChoiceId))
-            SetVoice(_voiceList[firstVoiceChoiceId].ID);
+        if (!VoiceButton.TrySelectId(voiceChoiceId) && firstValidVoiceId != -1 && VoiceButton.TrySelectId(firstValidVoiceId))
+            SetVoice(_voiceList[firstValidVoiceId].ID);
📝 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.

Suggested change
private void UpdateTTSVoicesControls()
{
if (Profile is null)
return;
VoiceButton.Clear();
var firstVoiceChoiceId = 1;
for (var i = 0; i < _voiceList.Count; i++)
{
var voice = _voiceList[i];
if (!HumanoidCharacterProfile.CanHaveVoice(voice, Profile.Sex))
continue;
var name = Loc.GetString(voice.Name);
VoiceButton.AddItem(name, i);
if (firstVoiceChoiceId == 1)
firstVoiceChoiceId = i;
}
var voiceChoiceId = _voiceList.FindIndex(x => x.ID == Profile.Voice);
if (!VoiceButton.TrySelectId(voiceChoiceId) && VoiceButton.TrySelectId(firstVoiceChoiceId))
SetVoice(_voiceList[firstVoiceChoiceId].ID);
}
private void UpdateTTSVoicesControls()
{
if (Profile is null)
return;
VoiceButton.Clear();
var firstValidVoiceId = -1;
for (var i = 0; i < _voiceList.Count; i++)
{
var voice = _voiceList[i];
if (!HumanoidCharacterProfile.CanHaveVoice(voice, Profile.Sex))
continue;
var name = Loc.GetString(voice.Name);
VoiceButton.AddItem(name, i);
if (firstValidVoiceId == -1)
firstValidVoiceId = i;
}
var voiceChoiceId = _voiceList.FindIndex(x => x.ID == Profile.Voice);
if (!VoiceButton.TrySelectId(voiceChoiceId) && firstValidVoiceId != -1 && VoiceButton.TrySelectId(firstValidVoiceId))
SetVoice(_voiceList[firstValidVoiceId].ID);
}


private void PlayTTS()
{
if (Profile is null)
return;

_ttsSystem.StopCurrentTTS(PreviewDummy);
_ttsManager.RequestTTS(PreviewDummy, _random.Pick(_sampleText), Profile.Voice);
}
}
24 changes: 24 additions & 0 deletions Content.Client/_White/TTS/TTSManager.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
using Content.Shared._White.TTS;
using Robust.Shared.Network;

namespace Content.Client._White.TTS;

// ReSharper disable once InconsistentNaming
public sealed class TTSManager
{
[Dependency] private readonly IClientNetManager _netMgr = default!;
[Dependency] private readonly EntityManager _entityManager = default!;

public void Initialize()
{
_netMgr.RegisterNetMessage<MsgRequestTTS>();
}

// ReSharper disable once InconsistentNaming
public void RequestTTS(EntityUid uid, string text, string voiceId)
{
var netEntity = _entityManager.GetNetEntity(uid);
var msg = new MsgRequestTTS { Text = text, Uid = netEntity, VoiceId = voiceId };
_netMgr.ClientSendMessage(msg);
}
Spatison marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +18 to +23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add input validation and error handling.

The RequestTTS method lacks several important validations and error handling:

  1. Parameter validation
  2. Text length limits
  3. Network error handling
  4. Voice ID validation

Consider implementing these safeguards:

-    public void RequestTTS(EntityUid uid, string text, string voiceId)
+    public void RequestTTS(EntityUid uid, string text, string voiceId)
+    {
+        if (string.IsNullOrEmpty(text))
+            throw new ArgumentException("Text cannot be null or empty", nameof(text));
+
+        if (string.IsNullOrEmpty(voiceId))
+            throw new ArgumentException("Voice ID cannot be null or empty", nameof(voiceId));
+
+        if (text.Length > 1000) // Add appropriate limit
+            throw new ArgumentException("Text exceeds maximum length", nameof(text));
+
+        try
+        {
+            var netEntity = _entityManager.GetNetEntity(uid);
+            var msg = new MsgRequestTTS { Text = text, Uid = netEntity, VoiceId = voiceId };
+            _netMgr.ClientSendMessage(msg);
+        }
+        catch (Exception ex)
+        {
+            // Consider logging the error
+            throw new InvalidOperationException("Failed to send TTS request", ex);
+        }
+    }
📝 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.

Suggested change
public void RequestTTS(EntityUid uid, string text, string voiceId)
{
var netEntity = _entityManager.GetNetEntity(uid);
var msg = new MsgRequestTTS { Text = text, Uid = netEntity, VoiceId = voiceId };
_netMgr.ClientSendMessage(msg);
}
public void RequestTTS(EntityUid uid, string text, string voiceId)
{
if (string.IsNullOrEmpty(text))
throw new ArgumentException("Text cannot be null or empty", nameof(text));
if (string.IsNullOrEmpty(voiceId))
throw new ArgumentException("Voice ID cannot be null or empty", nameof(voiceId));
if (text.Length > 1000) // Add appropriate limit
throw new ArgumentException("Text exceeds maximum length", nameof(text));
try
{
var netEntity = _entityManager.GetNetEntity(uid);
var msg = new MsgRequestTTS { Text = text, Uid = netEntity, VoiceId = voiceId };
_netMgr.ClientSendMessage(msg);
}
catch (Exception ex)
{
// Consider logging the error
throw new InvalidOperationException("Failed to send TTS request", ex);
}
}

Comment on lines +17 to +23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add input validation and error handling.

The RequestTTS method needs several improvements for robustness:

  1. Input validation for text and voiceId
  2. Error handling for network operations
  3. Size limits for text to prevent network issues
  4. Documentation about the expected voiceId format
+    /// <summary>
+    /// Requests a TTS message to be played for the specified entity.
+    /// </summary>
+    /// <param name="uid">The entity to play the TTS for</param>
+    /// <param name="text">The text to convert to speech (max 1000 characters)</param>
+    /// <param name="voiceId">The voice identifier (format: language-voice, e.g., "en-US-1")</param>
+    /// <exception cref="ArgumentException">Thrown when inputs are invalid</exception>
     public void RequestTTS(EntityUid uid, string text, string voiceId)
     {
+        if (string.IsNullOrEmpty(text))
+            throw new ArgumentException("Text cannot be empty", nameof(text));
+        if (string.IsNullOrEmpty(voiceId))
+            throw new ArgumentException("VoiceId cannot be empty", nameof(voiceId));
+        if (text.Length > 1000)
+            throw new ArgumentException("Text exceeds maximum length of 1000 characters", nameof(text));
+
         var netEntity = _entityManager.GetNetEntity(uid);
-        var msg = new MsgRequestTTS { Text = text, Uid = netEntity, VoiceId = voiceId };
-        _netMgr.ClientSendMessage(msg);
+        try {
+            var msg = new MsgRequestTTS { Text = text.Trim(), Uid = netEntity, VoiceId = voiceId };
+            _netMgr.ClientSendMessage(msg);
+        } catch (Exception e) {
+            Logger.ErrorS("tts", e, "Failed to send TTS request");
+            throw;
+        }
     }
📝 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.

Suggested change
// ReSharper disable once InconsistentNaming
public void RequestTTS(EntityUid uid, string text, string voiceId)
{
var netEntity = _entityManager.GetNetEntity(uid);
var msg = new MsgRequestTTS { Text = text, Uid = netEntity, VoiceId = voiceId };
_netMgr.ClientSendMessage(msg);
}
/// <summary>
/// Requests a TTS message to be played for the specified entity.
/// </summary>
/// <param name="uid">The entity to play the TTS for</param>
/// <param name="text">The text to convert to speech (max 1000 characters)</param>
/// <param name="voiceId">The voice identifier (format: language-voice, e.g., "en-US-1")</param>
/// <exception cref="ArgumentException">Thrown when inputs are invalid</exception>
public void RequestTTS(EntityUid uid, string text, string voiceId)
{
if (string.IsNullOrEmpty(text))
throw new ArgumentException("Text cannot be empty", nameof(text));
if (string.IsNullOrEmpty(voiceId))
throw new ArgumentException("VoiceId cannot be empty", nameof(voiceId));
if (text.Length > 1000)
throw new ArgumentException("Text exceeds maximum length of 1000 characters", nameof(text));
var netEntity = _entityManager.GetNetEntity(uid);
try {
var msg = new MsgRequestTTS { Text = text.Trim(), Uid = netEntity, VoiceId = voiceId };
_netMgr.ClientSendMessage(msg);
} catch (Exception e) {
Logger.ErrorS("tts", e, "Failed to send TTS request");
throw;
}
}

}
130 changes: 130 additions & 0 deletions Content.Client/_White/TTS/TTSSystem.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
using System.IO;
using Content.Shared._White;
using Content.Shared._White.TTS.Events;
using Robust.Client.Audio;
using Robust.Shared.Audio;
using Robust.Shared.Audio.Components;
using Robust.Shared.Configuration;

namespace Content.Client._White.TTS;

// ReSharper disable InconsistentNaming
public sealed class TTSSystem : EntitySystem
{
[Dependency] private readonly IAudioManager _audioManager = default!;
[Dependency] private readonly IConfigurationManager _cfg = default!;
[Dependency] private readonly AudioSystem _audioSystem = default!;

private float _volume;
private readonly Dictionary<EntityUid, AudioComponent> _currentlyPlaying = new();

private readonly Dictionary<EntityUid, Queue<AudioStreamWithParams>> _enquedStreams = new();

// Same as Server.ChatSystem.VoiceRange
private const float VoiceRange = 10;

public override void Initialize()
{
_cfg.OnValueChanged(WhiteCVars.TtsVolume, OnTtsVolumeChanged, true);

SubscribeNetworkEvent<PlayTTSEvent>(OnPlayTTS);
}

public override void Shutdown()
{
base.Shutdown();
_cfg.UnsubValueChanged(WhiteCVars.TtsVolume, OnTtsVolumeChanged);
ClearQueues();
}

public override void FrameUpdate(float frameTime)
{
foreach (var (uid, audioComponent) in _currentlyPlaying)
{
if (!Deleted(uid) && audioComponent is { Running: true, Playing: true }
|| !_enquedStreams.TryGetValue(uid, out var queue)
|| !queue.TryDequeue(out var toPlay))
Comment on lines +43 to +46
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Logical error in the conditional statement within 'FrameUpdate'

The use of logical OR (||) in the if condition may lead to unintended behavior, causing the loop to continue execution when it should process the audio queue. This can result in audio playback issues. Consider revising the condition to accurately reflect the intended logic using logical AND (&&) and proper parentheses for clarity.

Apply this diff to correct the conditional logic:

- if (!Deleted(uid) && audioComponent is { Running: true, Playing: true }
-     || !_enqueuedStreams.TryGetValue(uid, out var queue)
-     || !queue.TryDequeue(out var toPlay))
+ if ((!Deleted(uid) && audioComponent is { Running: true, Playing: true })
+     || (!_enqueuedStreams.TryGetValue(uid, out var queue))
+     || (!queue.TryDequeue(out var toPlay)))
    continue;

Alternatively, refactor the condition for better readability:

if ((!Deleted(uid) && audioComponent.Running && audioComponent.Playing))
    continue;

if (!_enqueuedStreams.TryGetValue(uid, out var queue))
    continue;

if (!queue.TryDequeue(out var toPlay))
    continue;

continue;

var audio = _audioSystem.PlayEntity(toPlay.Stream, uid, toPlay.Params);
if (!audio.HasValue)
continue;

_currentlyPlaying[uid] = audio.Value.Component;
}
}
Comment on lines +42 to +55
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Clean up _currentlyPlaying to remove finished audio components.

The _currentlyPlaying dictionary may retain entries for audio components that have finished playing. This could lead to unnecessary iterations and potential memory issues over time. Consider removing entries when the audio has stopped.

Modify the FrameUpdate method to remove finished audio components:

 public override void FrameUpdate(float frameTime)
 {
+    var uidsToRemove = new List<EntityUid>();

     foreach (var (uid, audioComponent) in _currentlyPlaying)
     {
-        if (!Deleted(uid) && audioComponent is { Running: true, Playing: true }
+        if (Deleted(uid) || audioComponent is not { Running: true, Playing: true })
+        {
+            uidsToRemove.Add(uid);
+            continue;
+        }

         if (!_enqueuedStreams.TryGetValue(uid, out var queue)
             || !queue.TryDequeue(out var toPlay))
             continue;

         var audio = _audioSystem.PlayEntity(toPlay.Stream, uid, toPlay.Params);
         if (!audio.HasValue)
             continue;

         _currentlyPlaying[uid] = audio.Value.Component;
     }

+    foreach (var uid in uidsToRemove)
+    {
+        _currentlyPlaying.Remove(uid);
+        _enqueuedStreams.Remove(uid);
+    }
 }

This ensures that entries for completed audio are removed, preventing potential memory leaks.

📝 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.

Suggested change
foreach (var (uid, audioComponent) in _currentlyPlaying)
{
if (!Deleted(uid) && audioComponent is { Running: true, Playing: true }
|| !_enquedStreams.TryGetValue(uid, out var queue)
|| !queue.TryDequeue(out var toPlay))
continue;
var audio = _audioSystem.PlayEntity(toPlay.Stream, uid, toPlay.Params);
if (!audio.HasValue)
continue;
_currentlyPlaying[uid] = audio.Value.Component;
}
}
var uidsToRemove = new List<EntityUid>();
foreach (var (uid, audioComponent) in _currentlyPlaying)
{
if (Deleted(uid) || audioComponent is not { Running: true, Playing: true })
{
uidsToRemove.Add(uid);
continue;
}
if (!_enquedStreams.TryGetValue(uid, out var queue)
|| !queue.TryDequeue(out var toPlay))
continue;
var audio = _audioSystem.PlayEntity(toPlay.Stream, uid, toPlay.Params);
if (!audio.HasValue)
continue;
_currentlyPlaying[uid] = audio.Value.Component;
}
foreach (var uid in uidsToRemove)
{
_currentlyPlaying.Remove(uid);
_enqueuedStreams.Remove(uid);
}
}


private void OnTtsVolumeChanged(float volume)
{
_volume = volume;
}

private void OnPlayTTS(PlayTTSEvent ev)
{
PlayTTS(GetEntity(ev.Uid), ev.Data, ev.BoostVolume ? _volume + 5 : _volume);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Incorrect parameter passed to PlayTTS method

The PlayTTS method expects an EntityUid as its first parameter, but GetEntity(ev.Uid) returns an Entity. Passing an Entity instead of EntityUid may lead to a compile-time error or unintended behavior.

Apply this diff to fix the issue:

-    PlayTTS(GetEntity(ev.Uid), ev.Data, ev.BoostVolume ? _volume + 5 : _volume);
+    PlayTTS(ev.Uid, ev.Data, ev.BoostVolume ? _volume + 5 : _volume);
📝 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.

Suggested change
PlayTTS(GetEntity(ev.Uid), ev.Data, ev.BoostVolume ? _volume + 5 : _volume);
PlayTTS(ev.Uid, ev.Data, ev.BoostVolume ? _volume + 5 : _volume);

}

public void PlayTTS(EntityUid uid, byte[] data, float volume)
{
if (_volume <= -20f)
return;
Comment on lines +67 to +70
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Inconsistent volume check using _volume instead of volume

In the PlayTTS method, the parameter volume is passed in, but the code checks the member variable _volume instead. This might lead to incorrect behavior, especially when volume differs from _volume.

Apply this diff to correct the volume check:

-public void PlayTTS(EntityUid uid, byte[] data, float volume)
-{
-    if (_volume <= -20f)
+public void PlayTTS(EntityUid uid, byte[] data, float volume)
+{
+    if (volume <= -20f)
        return;

This ensures the method behaves correctly based on the actual volume being used.

📝 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.

Suggested change
public void PlayTTS(EntityUid uid, byte[] data, float volume)
{
if (_volume <= -20f)
return;
public void PlayTTS(EntityUid uid, byte[] data, float volume)
{
if (volume <= -20f)
return;

Comment on lines +68 to +70
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Confusion between '_volume' and 'volume' parameters in 'PlayTTS' method

The method PlayTTS uses both _volume (the class field) and volume (the parameter), which can be confusing. Additionally, the volume check uses _volume instead of the passed volume parameter. This might lead to inconsistent behavior if volume differs from _volume.

Consider updating the volume check to use the volume parameter:

- if (_volume <= -20f)
+ if (volume <= -20f)
    return;

This ensures that the method behaves correctly based on the volume specified when calling PlayTTS.

📝 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.

Suggested change
{
if (_volume <= -20f)
return;
{
if (volume <= -20f)
return;

💡 Codebase verification

Based on the search results, I can now provide a final response:

Volume threshold of -20f is too restrictive for TTS audio

The TTS volume is configured through a client-side CVar with a default value of 0f, and the UI allows users to adjust it through a slider that's scaled appropriately with a TTS multiplier. The -20f threshold appears to be an arbitrary cutoff that could prevent TTS playback even when users have set a low but intentional volume.

  • The volume setting is properly managed through WhiteCVars.TtsVolume
  • Default value is 0f, not -20f
  • Volume is adjustable through the audio options UI tab
  • The UI scales the value appropriately (0-100 range converted to appropriate audio system range)
🔗 Analysis chain

Verify the volume threshold logic in 'PlayTTS'

The condition if (_volume <= -20f) might inadvertently prevent audio from playing if the volume is set to a low level. Verify that the volume threshold aligns with the application's volume settings and user expectations.

Run the following script to check where _volume is set and how it interacts with the volume settings:

This will help ensure that the volume logic is consistent and that users can adjust the volume as expected.


This will help us understand the TTS volume configuration and verify if -20f is a well-defined threshold.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of '_volume' in relation to volume settings.

# Search for all assignments to '_volume' and references in the codebase
rg '_volume' -A 3 -B 3

# Check the default and minimum volume settings in configuration files
rg 'TtsVolume' **/*.cs

Length of output: 12770


Script:

#!/bin/bash
# Search for TTS volume related settings and configurations
rg -A 3 -B 3 'TtsVolume|tts.*volume' --type cs

# Search for volume-related constants or thresholds in audio systems
ast-grep --pattern 'const float $_ = -20'

Length of output: 4250


Comment on lines +69 to +71
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure volume levels remain within acceptable limits.

When boosting the volume, there's a risk of exceeding the maximum volume threshold, which could cause audio distortion. Implement a check to cap the volume.

Adjust the PlayTTS method to cap the volume:

 public void PlayTTS(EntityUid uid, byte[] data, float volume)
 {
     if (_volume <= -20f)
         return;

+    // Cap the volume to a maximum of 0dB
+    if (volume > 0f)
+        volume = 0f;

     var stream = CreateAudioStream(data);

     var audioParams = new AudioParams
     {
         Volume = volume,
         MaxDistance = VoiceRange
     };

This prevents the volume from exceeding the maximum allowable level.

Also applies to: 76-78

var stream = CreateAudioStream(data);

var audioParams = new AudioParams
{
Volume = volume,
MaxDistance = VoiceRange
};

var audioStream = new AudioStreamWithParams(stream, audioParams);
EnqueueAudio(uid, audioStream);
}

public void StopCurrentTTS(EntityUid uid)
{
if (!_currentlyPlaying.TryGetValue(uid, out var audio))
return;

_audioSystem.Stop(audio.Owner);
Comment on lines +86 to +89
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Check for null before stopping audio to prevent exceptions

In StopCurrentTTS, the code assumes audio.Owner is valid. If audio is null or audio.Owner is invalid, this could lead to a NullReferenceException.

Consider adding a null check:

if (!_currentlyPlaying.TryGetValue(uid, out var audio) || audio?.Owner == null)
    return;

_audioSystem.Stop(audio.Owner);

This ensures the method safely handles cases where the audio component may not be valid.

📝 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.

Suggested change
if (!_currentlyPlaying.TryGetValue(uid, out var audio))
return;
_audioSystem.Stop(audio.Owner);
if (!_currentlyPlaying.TryGetValue(uid, out var audio) || audio?.Owner == null)
return;
_audioSystem.Stop(audio.Owner);

}

private void EnqueueAudio(EntityUid uid, AudioStreamWithParams audioStream)
{
if (!_currentlyPlaying.ContainsKey(uid))
{
var audio = _audioSystem.PlayEntity(audioStream.Stream, uid, audioStream.Params);
if (!audio.HasValue)
return;

_currentlyPlaying[uid] = audio.Value.Component;
return;
}

if (_enquedStreams.TryGetValue(uid, out var queue))
{
queue.Enqueue(audioStream);
return;
}

queue = new Queue<AudioStreamWithParams>();
queue.Enqueue(audioStream);
_enquedStreams[uid] = queue;
}

private void ClearQueues()
{
foreach (var (_, queue) in _enquedStreams)
{
queue.Clear();
}
}

private AudioStream CreateAudioStream(byte[] data)
{
var dataStream = new MemoryStream(data) { Position = 0 };
return _audioManager.LoadAudioOggVorbis(dataStream);
Comment on lines +125 to +126
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure MemoryStream is properly disposed to prevent resource leaks

The MemoryStream created in CreateAudioStream is not being disposed, which could lead to memory leaks. Ensure the stream is properly disposed after use.

Modify the method to dispose of the stream:

-private AudioStream CreateAudioStream(byte[] data)
-{
-    var dataStream = new MemoryStream(data) { Position = 0 };
-    return _audioManager.LoadAudioOggVorbis(dataStream);
-}
+private AudioStream CreateAudioStream(byte[] data)
+{
+    using var dataStream = new MemoryStream(data) { Position = 0 };
+    return _audioManager.LoadAudioOggVorbis(dataStream);
+}

Alternatively, confirm if LoadAudioOggVorbis internally handles the disposal of the stream. If it does, consider adding a comment to clarify this behavior.

Committable suggestion skipped: line range outside the PR's diff.

}
Comment on lines +123 to +127
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add exception handling for audio stream creation.

The CreateAudioStream method may throw an exception if the audio data is invalid. To enhance robustness, include exception handling to manage such scenarios gracefully.

Modify the method to catch exceptions:

 private AudioStream CreateAudioStream(byte[] data)
 {
     var dataStream = new MemoryStream(data) { Position = 0 };
-    return _audioManager.LoadAudioOggVorbis(dataStream);
+    try
+    {
+        return _audioManager.LoadAudioOggVorbis(dataStream);
+    }
+    catch (Exception ex)
+    {
+        Logger.ErrorS("tts", $"Failed to load audio stream: {ex.Message}");
+        return null;
+    }
 }

Ensure calling methods handle the possibility of a null audio stream.

📝 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.

Suggested change
private AudioStream CreateAudioStream(byte[] data)
{
var dataStream = new MemoryStream(data) { Position = 0 };
return _audioManager.LoadAudioOggVorbis(dataStream);
}
private AudioStream CreateAudioStream(byte[] data)
{
var dataStream = new MemoryStream(data) { Position = 0 };
try
{
return _audioManager.LoadAudioOggVorbis(dataStream);
}
catch (Exception ex)
{
Logger.ErrorS("tts", $"Failed to load audio stream: {ex.Message}");
return null;
}
}

Comment on lines +124 to +127
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle exceptions when loading audio streams in 'CreateAudioStream'

The CreateAudioStream method does not handle potential exceptions that may occur if the audio data is invalid or corrupted. This could lead to crashes or unhandled exceptions.

Consider wrapping the audio loading code in a try-catch block to handle exceptions:

private AudioStream CreateAudioStream(byte[] data)
{
    var dataStream = new MemoryStream(data) { Position = 0 };
+   try
+   {
        return _audioManager.LoadAudioOggVorbis(dataStream);
+   }
+   catch (Exception ex)
+   {
+       Logger.ErrorS("tts", $"Failed to load audio stream: {ex.Message}");
+       // Handle the exception as appropriate, e.g., return a default stream or null
+       return null;
+   }
}

Additionally, update the calling methods to handle a potential null return value from CreateAudioStream.

Committable suggestion skipped: line range outside the PR's diff.


private record AudioStreamWithParams(AudioStream Stream, AudioParams Params);
}
Loading
Loading