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

Add notification sound to headsets #32936

Closed
wants to merge 18 commits into from

Conversation

FireNameFN
Copy link
Contributor

@FireNameFN FireNameFN commented Oct 21, 2024

About the PR

Add notification sound to headset when it receives radio message.
You can configure with RBM which channels will emit sound and which not.
On regular headsets there are all channels except common by default. On centcom and command headsets enabled only it's channels and syndicate.
Bold font on verbs is used to mark channel as channel with enabled sound.

Why / Balance

Without notification sound you can easily miss important message.
If you don't want this feature you can set all channels to off.

Technical details

Added verbs to headsets.
Added fields to HeadsetComponent.
Added VerbCategory.
Changed headset prototypes.
Added sound emitting to headset.
Added new sounds.
Added new sound collection.

Media

image
image

Requirements

Breaking changes

None

Authors

@FireNameFN (Discord: fntech) - idea and code.
@Komti23 (Discord: komti23) - sounds.

Changelog

🆑 FireNameFN, Komti23

  • add: Added headset notification sounds.

@ps3moira
Copy link
Contributor

ps3moira commented Oct 21, 2024

here's a previous PR that was trying to do a similar thing 30780

@FireNameFN
Copy link
Contributor Author

FireNameFN commented Oct 21, 2024

here's a previous PR that was trying to do a similar thing 30780

#30780 (review)

After internal discussion, we've decided that there should be a toggle per channel and the sound itself needs to be changed so that it's less jarring.

So... I made what they wanted.

@FireNameFN FireNameFN marked this pull request as ready for review October 21, 2024 14:51
Copy link
Contributor

@beck-thompson beck-thompson left a comment

Choose a reason for hiding this comment

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

Tested in game, works works great! Maybe some variety on the radio noise would be nice but that's pretty minor.

The only issue I found was that you can right click someone elses headset and see the channels that are stored on it. Security will metagame thieves/syndicates a lot with this so you should probably change it so you can only change channels if its on your head or the ground (Anytime you could inspect it basically).

@tail-call
Copy link

Very cool code, definitely needs to be merged

@github-actions github-actions bot added the S: Needs Review Status: Requires additional reviews before being fully accepted label Oct 21, 2024
Copy link
Contributor

@beck-thompson beck-thompson left a comment

Choose a reason for hiding this comment

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

Only some minor stuff, rest looks good to me!

@FireNameFN
Copy link
Contributor Author

Needs final test because I didn't compile it after last fix.

@Unbelievable-Salmon
Copy link
Contributor

You're an absolute star, thank you. This feature helps people with visual impairments infinitely.

@YotaXP
Copy link

YotaXP commented Oct 22, 2024

Bold text is an inadequate indicator of a selected option, IMO. Far too difficult to differentiate, and may be even worse in other languages. Maybe give them a checkmark icon?

@TheShuEd TheShuEd self-assigned this Oct 23, 2024
@FireNameFN FireNameFN requested a review from TheShuEd October 24, 2024 08:26
@TheShuEd
Copy link
Member

code approve

@slarticodefast
Copy link
Member

slarticodefast commented Oct 26, 2024

Are borgs or other entities with intrinsic radio able to set the notification sound as well?
I think this should be a setting in the chat window instead of only a verb for headsets.
That way the setting is also bound to the player and not to the headset, meaning you can exchange headsets without having to reconfigure them.

@FireNameFN
Copy link
Contributor Author

FireNameFN commented Oct 26, 2024

Are borgs or other entities with intrinsic radio able to set the notification sound as well?

Maybe later. We can add to them headset slot with not unequippable headset.

I think this should be a setting in the chat window instead of only a verb for headsets.
That way the setting is also bound to the player and not to the headset, meaning you can exchange headsets without having to reconfigure them.

But if you are passenger and you find command's headset, you want it to be configured as command headset. Also there will be a need to define default sound channels for each role. I think it's bad.

@YotaXP
Copy link

YotaXP commented Oct 26, 2024

But if you are passenger and you find command's headset, you want it to be configured as command headset. Also there will be a need to define default sound channels for each role. I think it's bad.

I believe you would just have 'Command' notifications enabled before you even get your hands on the privileged headset, along with all other department channels. This would only be annoying for Captain.

@FireNameFN
Copy link
Contributor Author

Maybe I should use CanAccess. Wait.

@SlamBamActionman SlamBamActionman added the S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. label Nov 14, 2024
@beck-thompson beck-thompson added P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. T: New Feature Type: New feature or content, or extending existing content D3: Low Difficulty: Some codebase knowledge required. A: General Interactions Area: General in-game interactions that don't relate to another area. size/L Denotes a PR that changes 1000-4999 lines. and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Nov 18, 2024
@Pumkin69
Copy link

i mean it seems good to me

@beck-thompson beck-thompson added the S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. label Dec 26, 2024
Copy link
Contributor

@beck-thompson beck-thompson left a comment

Choose a reason for hiding this comment

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

Only one minor change! Looks good besides this 👍

@github-actions github-actions bot added Changes: Audio Changes: Might require knowledge of audio. size/M Denotes a PR that changes 100-999 lines. and removed size/L Denotes a PR that changes 1000-4999 lines. labels Jan 18, 2025
Copy link
Contributor

@beck-thompson beck-thompson left a comment

Choose a reason for hiding this comment

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

Looks good! Its under maintainer discussion now 🫡

@beck-thompson beck-thompson added the S: Undergoing Maintainer Discussion Status: Currently going through an extended discussion amongst maintainers, as per procedure. label Jan 18, 2025
Copy link
Contributor

@beck-thompson beck-thompson left a comment

Choose a reason for hiding this comment

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

Hey! Sorry for the long wait on this PR. There was a lot of debate on this but it was eventually decided that it would be better to have the notification sound be a chat menu setting rather than a setting on the headset itself. This is mainly because there are many situations where you would want notifications but dont have a headset, e.g if your a borg, or a holoparasite (Forks have even more examples as well). This will probably become a even bigger issue after the incoming chat refactor is finished!

There are some PRs up for chat highlighting, and adding a menu for notification sounds should be somewhat similar (UI wise at least)!

#31442
#33249

If you need any help or have questions just ask here or the discord

@ArtisticRoomba ArtisticRoomba added S: Awaiting Changes Status: Changes are required before another review can happen and removed S: Needs Review Status: Requires additional reviews before being fully accepted labels Jan 27, 2025
@FireNameFN FireNameFN closed this Feb 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: General Interactions Area: General in-game interactions that don't relate to another area. Changes: Audio Changes: Might require knowledge of audio. D3: Low Difficulty: Some codebase knowledge required. P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. S: Awaiting Changes Status: Changes are required before another review can happen S: Undergoing Maintainer Discussion Status: Currently going through an extended discussion amongst maintainers, as per procedure. size/M Denotes a PR that changes 100-999 lines. T: New Feature Type: New feature or content, or extending existing content
Projects
None yet
Development

Successfully merging this pull request may close these issues.