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

Added launch alarm for PR #4976 #5105

Merged
merged 9 commits into from
Dec 7, 2023

Conversation

fordmichigan
Copy link
Contributor

@fordmichigan fordmichigan commented Dec 2, 2023

About the pull request

Adds a looping alarm sound when the lifeboats are launched. This also uses the function described in PR #4976 to trigger the sound. This is NOT for adding those features, so wait until that is merged to merge this PR.

Explain why it's good for the game

Denies the need to make an announcement warning marines that the lifeboats are leaving.

Testing Photographs and Procedure

Screenshots & Videos

https://youtu.be/L8ia2OKhUUA

Changelog

🆑alexguinea atticus-rezzer
add: Adds alarm sound for lifeboats when launching
/:cl:

@github-actions github-actions bot added the Feature Feature coder badge label Dec 2, 2023
@zzzmike
Copy link
Contributor

zzzmike commented Dec 3, 2023

Hi, 4976 author here. I think the automated ship announcement is good in addition to the sound so that you'll know whether the port or starboard boat is the one launching - I assume the sound will go far enough that it isn't obvious which boat it's coming from at first. Also, thank you!! Ever since the dropship alarm got merged, I was thinking of stealing it for my PR and never got around to it, heh. ❤️

(Also adding link to #4976 on comment rather than just on commits for my own sanity)

@Birdtalon Birdtalon added the Do Not Merge If you merge this PR, I will annihilate you label Dec 3, 2023
@Birdtalon
Copy link
Contributor

Birdtalon commented Dec 3, 2023

Are you adding code on top of #4976 here? It took me a while to clock that I'd seen this code in another PR and I didn't realise that's what you meant from the title. I'm adding the DNM tag until #4976 is merged or closed.

In future please document this in your PR description as it looks as though you're also changing access & launch options here which isn't documented in your PR, when it's actually #4976

Edit; just read the comment @zzzmike thank you for the information here. @fordmichigan could you please just add a line in your PR description that references this so anyone else that looks at this (who might not have seen #4976 ) can see easily what's going on.

@fordmichigan
Copy link
Contributor Author

Is it good now?

@Birdtalon
Copy link
Contributor

Is it good now?

👍 thanks. Will wait for #4976 then I will remove DNM

@cm13-github cm13-github added the Merge Conflict PR can't be merged because it touched too much code label Dec 6, 2023
@cm13-github
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@Birdtalon Birdtalon removed the Do Not Merge If you merge this PR, I will annihilate you label Dec 6, 2023
@cm13-github cm13-github removed the Merge Conflict PR can't be merged because it touched too much code label Dec 7, 2023
@cm13-github
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@Drulikar Drulikar added this pull request to the merge queue Dec 7, 2023
Merged via the queue into cmss13-devs:master with commit 1498307 Dec 7, 2023
26 checks passed
cm13-github added a commit that referenced this pull request Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Feature coder badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants