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

Launch Alarm V3 #6341

Conversation

hislittlecuzingames
Copy link
Contributor

About the pull request

Updates the Dropship Launch Alarms, and UI.

Explain why it's good for the game

Adds a new "Cycle" alarm. It has a similar, but distinctly different sound from the main alarm. This is for Pilots to say "I'm leaving soon, but not evacing" whilst the FOB is under siege. Plays only once, and requires re-pressing to play again so your ears don't get assaulted. Not allowing "bwoop" scenario from v0.

Buttons are also put into the section properly using the stack like the destinations.

Retained top right button in Launch Alarms for muscle memory. Plus it doesn't take up more space having it there.

Testing Photographs and Procedure

Screenshots & Videos
unknown_2024.05.27-06.32_clip_1.mp4

Changelog

🆑
add: Added Cycle Launch Alarm
ui: changed Dropship UI to accommodate two alarm buttons
refactor: Refactored some of the code to stop all alarms easily
soundadd: Added Cycle Alarm Sounds
/:cl:

@github-actions github-actions bot added Sound Blast 5 minutes of bass boosted music to our players UI deletes nanoui/html Feature Feature coder badge Refactor Make the code harder to read labels May 27, 2024
@hislittlecuzingames
Copy link
Contributor Author

I don't know what's up with the formatting. If it's failed, I don't know what it wants.

I have not tested with format adjustments, but I imagine TSGUI being React based, and HTML tech, it should be no issue.

@vero5123
Copy link
Contributor

vero5123 commented May 27, 2024

I don't know what's up with the formatting. If it's failed, I don't know what it wants.

I have not tested with format adjustments, but I imagine TSGUI being React based, and HTML tech, it should be no issue.

prettier --write whateverfile.tsx

Screen Shot 2024-05-27 at 1 55 57 PM

@ihatethisengine
Copy link
Contributor

Alarm bloat

@hislittlecuzingames
Copy link
Contributor Author

I don't know what's up with the formatting. If it's failed, I don't know what it wants.
I have not tested with format adjustments, but I imagine TSGUI being React based, and HTML tech, it should be no issue.

prettier --write whateverfile.tsx
Screen Shot 2024-05-27 at 1 55 57 PM

image

@vero5123
Copy link
Contributor

vero5123 commented May 28, 2024

I don't know what's up with the formatting. If it's failed, I don't know what it wants.
I have not tested with format adjustments, but I imagine TSGUI being React based, and HTML tech, it should be no issue.

prettier --write whateverfile.tsx
Screen Shot 2024-05-27 at 1 55 57 PM

image

cd into the tgui folder so your end directory should be cmss13/tgui
if yarn isn't already installed globally then in the terminal type: npm install -g yarn, you can check by typing: yarn --version
Now, in the same cmss13/tgui directory type: yarn install, this will install all the dependencies
Next, type: yarn run tgui:prettier-fix

check the package.json but essentially tgui:prettier-fix runs prettier --write on all its subfiles to autoformat it.

@hislittlecuzingames
Copy link
Contributor Author

hislittlecuzingames commented May 28, 2024

I don't know what's up with the formatting. If it's failed, I don't know what it wants.
I have not tested with format adjustments, but I imagine TSGUI being React based, and HTML tech, it should be no issue.

prettier --write whateverfile.tsx
Screen Shot 2024-05-27 at 1 55 57 PM

image

cd into the tgui folder so your end directory should be cmss13/tgui if yarn isn't already installed globally then in the terminal type: npm install -g yarn, you can check by typing: yarn --version Now, in the same cmss13/tgui directory type: yarn install, this will install all the dependencies Next, type: yarn run tgui:prettier-fix

check the package.json but essentially tgui:prettier-fix runs prettier --write on all its subfiles to autoformat it.

Hey, sorry, I tried installing yarn & googled several things to get it working.

But I cannot update npm to run yarn, because my npm is out of date.
or maybe install. I'm not sure.
I don't understand how I'm supposed to update, if I need the update to install the update.

image

@vero5123
Copy link
Contributor

vero5123 commented May 28, 2024

Hey, sorry, I tried installing yarn & googled several things to get it working.

But I cannot update npm to run yarn, because my npm is out of date. or maybe install. I'm not sure. I don't understand how I'm supposed to update, if I need the update to install the update.

image

npm comes natively with node, although they technically can be packaged separately. Your issue seems to be that your node version is out of date, v16.15.1. What you need to do is to uninstall node.js, this should be simple on windows. Next, install the latest LTS version, visit https://nodejs.org/en/download/prebuilt-installer and don't install additional tools like Chocolatey when prompted.

@hislittlecuzingames
Copy link
Contributor Author

npm comes natively with node, although they technically can be packaged separately. Your issue seems to be that your node version is out of date, v16.15.1. What you need to do is to uninstall node.js, this should be simple on windows. Next, install the latest LTS version, visit https://nodejs.org/en/download/prebuilt-installer and don't install additional tools like Chocolatey when prompted.

Thanks! I forgot about the GUI installer. I wanted to see what'd happen if I didn't uninstall, & at least on my computer, the installer detected my previous installation, and was able to run properly. Then with no further configuration, it worked perfectly.

Linux is for the command line, and windows is not.

@vero5123
Copy link
Contributor

vero5123 commented May 29, 2024

check your paths and make sure there are no conflicts.

@hislittlecuzingames
Copy link
Contributor Author

check your paths and make sure there are no conflicts.

Doesn't seem to be any in "Environment Variables." Looked through a few menus & sub menus.

Guess this PR is ready to go and I am up to date with Node!
Thanks!

Copy link
Contributor

@vero5123 vero5123 left a comment

Choose a reason for hiding this comment

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

If the cycle alarm only plays the sound once then maybe it would be better to just gray out the button so that the user can't select it for the duration of the sound effect? Having a cancel button like with the other alarm just feels awkward because you need to click cancel again to play the sound even though it's not looping anymore. It's also not really "accurate" because the sound is no longer playing and playing_cycle_launch_announcement_alarm is still evaluating as true. Also, maybe it would be better if you can only select one alarm sound at a time? So if you select the main alarm you shouldn't be able to play the cycle alarm in parallel, either on accident or on purpose.

You can maybe use a cooldown instead and set it to the duration of the cycle alarm, then you could pass that in to tgui and if there is still some cooldown time left the cycle alarm message will be grayed out and disabled.

code/modules/shuttle/computers/dropship_computer.dm Outdated Show resolved Hide resolved
code/modules/shuttle/computers/dropship_computer.dm Outdated Show resolved Hide resolved
@hislittlecuzingames
Copy link
Contributor Author

hislittlecuzingames commented May 30, 2024

If the cycle alarm only plays the sound once

I was going to program it that way, but I figured I'd reuse the looping code, and then it didn't loop, so I was like "it does what I wanted anyway..."

Also, maybe it would be better if you can only select one alarm sound at a time? So if you select the main alarm you shouldn't be able to play the cycle alarm in parallel, either on accident or on purpose.

That's a neat idea.

EDIT:
I think having them separate is necessary with the main launch alarm needing to be pressed fast for evac. So I think I might not implement selecting alarms.

@vero5123
Copy link
Contributor

vero5123 commented May 30, 2024

If you choose to keep things similar and need a reference for how to set up the cooldown, see #6333 , check forest's most recent commit.

@hislittlecuzingames
Copy link
Contributor Author

If you choose to keep things similar and need a reference for how to set up the cooldown, see #6333 , check forest's most recent commit.

Is this the change I need to make to get this PR merged?

@vero5123
Copy link
Contributor

vero5123 commented Jun 4, 2024

Is this the change I need to make to get this PR merged?

I cannot approve or reject your pr; i'm a contributor like yourself. It was just my suggestion.

@hislittlecuzingames
Copy link
Contributor Author

Is this the change I need to make to get this PR merged?

I cannot approve or reject your pr; i'm a contributor like yourself. It was just my suggestion.

ahh okay. I saw you in the reviewers' section, so I thought that you were a reviewer.

@Drulikar
Copy link
Contributor

Drulikar commented Jun 9, 2024

As mentioned on the discord, I don't think any of these changes are necessary. Its more command's responsibility to communicate whether the marines are evaccing or not; its not necessarily clear to marines what sound means what; and this can just open up more miscommunication what's going on in a chaotic situation like fob evac.

@Drulikar Drulikar added the Do Not Merge If you merge this PR, I will annihilate you label Jun 9, 2024
@hislittlecuzingames
Copy link
Contributor Author

Going to convert this into a refactor.
I'll close this pr in a week or two when I am able to use the code properly.

@Drulikar Drulikar marked this pull request as draft June 9, 2024 23:22
Copy link
Contributor

This PR has been inactive for long enough to be automatically marked as stale. This means it is at risk of being auto closed in ~ 7 days, please address any outstanding review items and ensure your PR is finished, if these are all true and you are auto-staled anyway, you need to actively ask maintainers if your PR will be merged. Once you have done any of the previous actions then you should request a maintainer remove the stale label on your PR, to reset the stale timer. If you feel no maintainer will respond in that time, you may wish to close this PR youself, while you seek maintainer comment, as you will then be able to reopen the PR yourself

@github-actions github-actions bot added the Stale beg a maintainer to review your PR label Jun 17, 2024
@github-actions github-actions bot closed this Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Do Not Merge If you merge this PR, I will annihilate you Feature Feature coder badge Refactor Make the code harder to read Sound Blast 5 minutes of bass boosted music to our players Stale beg a maintainer to review your PR UI deletes nanoui/html
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants