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.

Sign up for GitHub

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

Jump to bottom

Add pricegun sound #34101

Closed
wants to merge 12 commits into from
Closed

Add pricegun sound #34101

wants to merge 12 commits into from

Conversation

DylanWhittingham
Copy link
Contributor

@DylanWhittingham DylanWhittingham commented Dec 28, 2024 •
edited
Loading

About the PR

Fixes #33473
Adds a sound that plays when the Appraisal Tool successfully appraises an entity.

Why / Balance

Previously, the Appraisal Tool had no sound, this improves feedback of the tool with audio.

Technical details

added SoundPathSpecifier to "Machines/chime.ogg" to PriceGunComponent class
added PlayPredicted to SharedPriceGunSystems class in the OnAfterIntereact method.

Media

appraisal_new_sound.mp4

Requirements

  • I have read and am following the Pull Request and Changelog Guidelines
  • I have added media to this PR or it does not require an ingame showcase.

Breaking changes

None

Changelog
🆑
add: Added appraisal tool sound.

@github-actions github-actions bot added S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. size/S Denotes a PR that changes 10-99 lines. labels Dec 28, 2024
slarticodefast
slarticodefast requested changes Dec 28, 2024
View reviewed changes
Copy link
Member

@slarticodefast slarticodefast left a comment

Choose a reason for hiding this comment

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

Is there maybe a better sound than the chime? It feels a little too loud and more like a warning sound to me.

Content.Shared/Cargo/SharedPriceGunSystem.cs Outdated Show resolved Hide resolved
Content.Shared/Cargo/SharedPriceGunSystem.cs Outdated Show resolved Hide resolved
@slarticodefast slarticodefast self-assigned this Dec 28, 2024
@slarticodefast slarticodefast added the S: Awaiting Changes Status: Changes are required before another review can happen label Dec 28, 2024
@slarticodefast
Copy link
Member

Also your changelog is formatted wrong. Please have a look at the PR guidelines to see how to do it correctly, so that the bot can add it to the in-game changelog.

@lzk228 lzk228 added P3: Standard Priority: Default priority for repository items. 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. and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Dec 28, 2024
@beck-thompson
Copy link
Contributor

Fun idea! I love little things like this 👍

@github-actions github-actions bot added the Changes: Audio Changes: Might require knowledge of audio. label Dec 28, 2024
slarticodefast
slarticodefast requested changes Dec 28, 2024
View reviewed changes
Copy link
Member

@slarticodefast slarticodefast left a comment

Choose a reason for hiding this comment

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

Small suggestion that saves us the TryComp.
You will have to adjust the GetPriceOrBounty function in SharedPriceGunSystem.cs accordingly.
Also the new sound file will need to be added to the attributions.yml file in the same folder.

Content.Server/Cargo/Systems/PriceGunSystem.cs Outdated Show resolved Hide resolved
Content.Server/Cargo/Systems/PriceGunSystem.cs Outdated Show resolved Hide resolved
Content.Server/Cargo/Systems/PriceGunSystem.cs Outdated Show resolved Hide resolved
@github-actions github-actions bot added S: Needs Review Status: Requires additional reviews before being fully accepted and removed S: Awaiting Changes Status: Changes are required before another review can happen labels Dec 28, 2024
slarticodefast
slarticodefast approved these changes Dec 29, 2024
View reviewed changes
Copy link
Member

@slarticodefast slarticodefast left a comment

Choose a reason for hiding this comment

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

The code looks good and the in-game test went well.
Should be good to merge.
Thank you for your for first contribution!

@slarticodefast slarticodefast added S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. and removed S: Needs Review Status: Requires additional reviews before being fully accepted labels Dec 29, 2024
@DylanWhittingham DylanWhittingham closed this by deleting the head repository Dec 29, 2024
@DylanWhittingham DylanWhittingham mentioned this pull request Dec 29, 2024
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slarticodefast slarticodefast slarticodefast approved these changes

Assignees

@slarticodefast slarticodefast

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. P3: Standard Priority: Default priority for repository items. S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. size/S Denotes a PR that changes 10-99 lines. T: New Feature Type: New feature or content, or extending existing content
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Appraisal tool needs sound
4 participants
@DylanWhittingham @slarticodefast @beck-thompson @lzk228