Skip to content

Conversation

EnnyDaiz
Copy link
Contributor

@EnnyDaiz EnnyDaiz commented Feb 13, 2025

About The Pull Request

Returns stereo sound for music and ambient, which was changed here =>
#55333
Due to the override environment (probably) absolutely ALL music was downmixed to mono, such as music via global sound, direct sound to mob and megafauna themes.
In addition to this, the sound from musical instruments at certain positioning was sent exclusively to one ear, which made it extremely uncomfortable to listen to with headphones.

Commenting of 183-189 lines was done to make proc local_sound work correctly, otherwise an error occurs.

Video from the tests is attached, old - what we have at the moment; new - what we get when commenting on the lines.
https://drive.google.com/file/d/1wSZ2I5XE74nePOTfMmaj-xqmonf3g58O/view
(too much weight, had to upload it on google-drive, I apologize for that.)

Why It's Good For The Game

Fixes the bug? Based on the PR with override environment, downmixing music in mono was not planned.
Makes working with music and listening to it more comfortable

Changelog

🆑
fix: remove downmix stereo=>mono where it is not required
/:cl:

@github-actions github-actions bot added Sound Oh god my ears, they bleed, did you normalise the volume? Fix Rewrites a bug so it appears in different circumstances labels Feb 13, 2025
@EnnyDaiz EnnyDaiz marked this pull request as ready for review February 13, 2025 06:07
Copy link
Member

@SmArtKar SmArtKar left a comment

Choose a reason for hiding this comment

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

Aren't you literally removing sound reverb and echo here?

@@ -1,5 +1,7 @@

///Default override for echo
// TFF CHANGE START - FIX STEREO-AUDIO
Copy link
Member

Choose a reason for hiding this comment

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

uuuh, did you PR this from a modular codebase?

@@ -176,11 +180,13 @@
else
var/area/A = get_area(src)
sound_to_use.environment = A.sound_environment

// TFF CHANGE START - FIX STEREO-AUDIO
Copy link
Member

Choose a reason for hiding this comment

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

same here?

@Ghommie Ghommie added the Good First PR We all have to start somewhere label Feb 13, 2025
@EnnyDaiz
Copy link
Contributor Author

Aren't you literally removing sound reverb and echo here?

Not really - without this override, the byond uses a standard environment (probably "generic") that has echo and reverb in it.
So in practice it still exists as it is, and becomes even more noticeable.
At least - that's what I heard while I was testing this out
(note 2:56 - 2:58 and 3:30 - 3:32 - in the first case the guitar sound is interrupted abruptly, while in the second a slight “tail” can be heard. )

And no, this change is not made solely for the sake of the modular codebase. It's just that I... I've seen that when modifying core-files they do that, that's why I put “TFF CHANGE”, but it seems to be only misleading ._."
(This is the first PR I'm doing without outside help, so some things have to be learned “here and now”...)

@SmArtKar
Copy link
Member

Aren't you literally removing sound reverb and echo here?

Not really - without this override, the byond uses a standard environment (probably "generic") that has echo and reverb in it. So in practice it still exists as it is, and becomes even more noticeable. At least - that's what I heard while I was testing this out (note 2:56 - 2:58 and 3:30 - 3:32 - in the first case the guitar sound is interrupted abruptly, while in the second a slight “tail” can be heard. )

And no, this change is not made solely for the sake of the modular codebase. It's just that I... I've seen that when modifying core-files they do that, that's why I put “TFF CHANGE”, but it seems to be only misleading ._." (This is the first PR I'm doing without outside help, so some things have to be learned “here and now”...)

// X CHANGE is a modular codebase thing, we don't use it - if you remove code, delete it, not comment it out

@CRITAWAKETS
Copy link
Contributor

CRITAWAKETS commented Feb 13, 2025

For some reason, this actually has been tested and works, making sounds play in stereo instead of mono.
None of the documentation says this should affect stereo/mono output, with the only documentation being on sound position causing downmixing.

Thanks BYOND.

EDIT: echo being defined for a sound causes it to downmix to mono.

@MrMelbert

This comment was marked as outdated.

@MrMelbert

This comment was marked as outdated.

@MrMelbert MrMelbert added the FUCK Only two things are infinite, the universe and human stupidity, and I'm not sure about the former label Feb 13, 2025
@MrMelbert
Copy link
Member

MrMelbert commented Feb 13, 2025

Ok my understanding of this was completely wrong

EDIT: echo being defined for a sound causes it to downmix to mono.

As it turns out modifying echo OR environment downmixes to mono in any situation. I guess that makes sense

@MrMelbert
Copy link
Member

MrMelbert commented Feb 13, 2025

However, this code does not result in downmixing (But I didn't see any positive effect from it during tests either, I suspect that it doesn't work). In that case should I keep it, or delete it?

Assuming this code is combined with the PR's existing code, this will do nothing because it is erroring, as echo is null and not a list

@SyncIt21
Copy link
Contributor

Please remove [FIX] from your PR title. The Changelog makes that apparent.

Also update your branch with master to restart icon diff bot

@EnnyDaiz EnnyDaiz changed the title [FIX] Return stereo for music/ambient Return stereo for music/ambient Feb 19, 2025
@Ghommie Ghommie merged commit 61e0b8d into tgstation:master Mar 17, 2025
24 checks passed
tgstation-ci bot added a commit that referenced this pull request Mar 17, 2025
github-actions bot added a commit that referenced this pull request Mar 17, 2025
Vallat pushed a commit to Vallat/tgstation that referenced this pull request Apr 9, 2025
## About The Pull Request
Returns stereo sound for music and ambient, which was changed here =>
tgstation#55333
Due to the override environment (probably) absolutely ALL music was
downmixed to mono, such as music via global sound, direct sound to mob
and megafauna themes.
In addition to this, the sound from musical instruments at certain
positioning was sent exclusively to one ear, which made it extremely
uncomfortable to listen to with headphones.

Commenting of 183-189 lines was done to make proc local_sound work
correctly, otherwise an error occurs.

Video from the tests is attached, old - what we have at the moment; new
- what we get when commenting on the lines.
https://drive.google.com/file/d/1wSZ2I5XE74nePOTfMmaj-xqmonf3g58O/view
(too much weight, had to upload it on google-drive, I apologize for
that.)

## Why It's Good For The Game
Fixes the bug? Based on the PR with override environment, downmixing
music in mono was not planned.
Makes working with music and listening to it more comfortable

## Changelog
:cl:
fix: remove downmix stereo=>mono where it is not required
/:cl:
Vallat pushed a commit to Vallat/tgstation that referenced this pull request Apr 9, 2025
buffyuwu pushed a commit to The-Final-Nights/The-Final-Nights that referenced this pull request Apr 17, 2025
<!-- Write **BELOW** The Headers and **ABOVE** The comments else it may
not be viewable. -->
<!-- You can view Contributing.MD for a detailed description of the pull
request process. -->

## About The Pull Request

<!-- Describe The Pull Request. Please be sure every change is
documented or this can delay review and even discourage maintainers from
merging your PR! -->

Ports the following TG station PRs:
- [x] tgstation/tgstation#59667
- [x] tgstation/tgstation#61573
- [x] tgstation/tgstation#67455
- [x] tgstation/tgstation#72805
- [x] tgstation/tgstation#74170
- [x] tgstation/tgstation#74650
- [x] tgstation/tgstation#75691
- [x] tgstation/tgstation#76453
- [x] tgstation/tgstation#76751
- [x] tgstation/tgstation#81135
- [x] tgstation/tgstation#81923
- [x] tgstation/tgstation#82152
- [x] tgstation/tgstation#85953
- [x] tgstation/tgstation#85967
- [x] tgstation/tgstation#89470
- [x] tgstation/tgstation#89777
- [x] tgstation/tgstation#90330


## Why It's Good For The Game

<!-- Argue for the merits of your changes and how they benefit the game,
especially if they are controversial and/or far reaching. If you can't
actually explain WHY what you are doing will improve the game, then it
probably isn't good for the game in the first place. -->

jukebox fixes + audio stuff is good...

## Testing Photographs and Procedure
<!-- Include any screenshots/videos/debugging steps of the modified code
functioning successfully, ideally including edge cases. -->
<!-- You can uncomment line 1 @ _maps/_basemap.dm to boot up a test map
that loads much faster. -->
<details>
<summary>Screenshots&Videos</summary>



https://github.com/user-attachments/assets/c5ef5f9c-0f8b-4b16-a9aa-e6a36c04443f



</details>

## Changelog

<!-- If your PR modifies aspects of the game that can be concretely
observed by players or admins you should add a changelog. If your change
does NOT meet this description, remove this section. Be sure to properly
mark your PRs to prevent unnecessary GBP loss. You can read up on GBP
and its effects on PRs in the tgstation guides for contributors. Please
note that maintainers freely reserve the right to remove and add tags
should they deem it appropriate. You can attempt to finagle the system
all you want, but it's best to shoot for clear communication right off
the bat. -->

:cl: XeonMations, JJRcop
add: Added a new verb that players can request audio from admins with
add: Admins can now play internet links.
config: More config changes related to roundstart and roundend music.
/:cl:

<!-- Both :cl:'s are required for the changelog to work! You can put
your name to the right of the first :cl: if you want to overwrite your
GitHub username as author ingame. -->
<!-- You can use multiple of the same prefix (they're only used for the
icon ingame) and delete the unneeded ones. Despite some of the tags,
changelogs should generally represent how a player might be affected by
the changes rather than a summary of the PR's contents. -->

---------

Co-authored-by: Jonathan Rubenstein <jrubcop@gmail.com>
Co-authored-by: san7890 <the@san7890.com>
Co-authored-by: Kylerace <kylerlumpkin1@gmail.com>
Co-authored-by: SomeRandomOwl <2568378+SomeRandomOwl@users.noreply.github.com>
Co-authored-by: Fikou <23585223+Fikou@users.noreply.github.com>
Co-authored-by: Aleksej Komarov <stylemistake@gmail.com>
Co-authored-by: Watermelon914 <37270891+Watermelon914@users.noreply.github.com>
Co-authored-by: MrMelbert <51863163+MrMelbert@users.noreply.github.com>
Co-authored-by: John Willard <53777086+JohnFulpWillard@users.noreply.github.com>
Co-authored-by: PeriodicChaos <44913068+PeriodicChaos@users.noreply.github.com>
Co-authored-by: EnnyDaiz <160051896+EnnyDaiz@users.noreply.github.com>
Co-authored-by: harry <me@harryob.live>
Co-authored-by: harryob <55142896+harryob@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Rewrites a bug so it appears in different circumstances FUCK Only two things are infinite, the universe and human stupidity, and I'm not sure about the former Good First PR We all have to start somewhere Sound Oh god my ears, they bleed, did you normalise the volume?
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants