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

Keycard authenticator can call ERT winout admin intervention #4477

Closed
wants to merge 12 commits into from

Conversation

Diegoflores31
Copy link
Contributor

@Diegoflores31 Diegoflores31 commented Sep 23, 2023

About the pull request

Fixes #4333

the keycard authentication will now show an option to send a distress signal similar to the almayer control console that will allow to call the FIRST distress signal on the almayer winout the need for admin approval . ( only works on red alert)

Explain why it's good for the game

Gives more options to lowpop command and provides a unique reward for using the authenticator in CIC instead of callling ERT remotely from lifeboats.

Testing Photographs and Procedure

Screenshots & Videos

Put screenshots and videos here with an empty line between the screenshots and the <details> tags.

Changelog

🆑
add: Adds a new way to send a Distress signal with the Keycard authenticator.
fix: Removes non functional "Call ERT" button in Keycard authenticator.
/:cl:

@github-actions github-actions bot added Feature Feature coder badge Fix Fix one bug, make ten more labels Sep 23, 2023
@ghost
Copy link

ghost commented Sep 23, 2023

If that's the only time is_ert_blocked() is even used you might just want to shove that logic into call_ert() and simply return.

Copy link
Member

@morrowwolf morrowwolf left a comment

Choose a reason for hiding this comment

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

Auto ERTs isn't gonna fly.

It still needs admin approval.

@morrowwolf morrowwolf marked this pull request as draft September 24, 2023 08:01
@Diegoflores31
Copy link
Contributor Author

Auto ERTs isn't gonna fly.

It still needs admin approval.

but it only works for the first ERT . cannot be used more than once

Copy link
Member

@morrowwolf morrowwolf left a comment

Choose a reason for hiding this comment

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

If you keep it to post hijack and it works the same way for every way to call an ERT it can be a free one but it should give an opportunity for staff to cancel it and extra credit for a button somewhere to pre-toggle it off.

@Diegoflores31
Copy link
Contributor Author

Ok

@Diegoflores31
Copy link
Contributor Author

If you keep it to post hijack and it works the same way for every way to call an ERT it can be a free one but it should give an opportunity for staff to cancel it and extra credit for a button somewhere to pre-toggle it off.

Changes applied:
-staff will have 15 seconds to cancel the ERT request . if it goes unanswered for more than that it will automatically approve
-If distress i called before hijack it will just call a standart ERT request,
-minor change applied ( you cant swipe for red alert if the ship is already on red alert)

@Diegoflores31 Diegoflores31 marked this pull request as ready for review October 2, 2023 02:54
Copy link
Contributor

@Zonespace27 Zonespace27 left a comment

Choose a reason for hiding this comment

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

Follow cthulhu's reviews please

code/datums/emergency_calls/emergency_call.dm Outdated Show resolved Hide resolved
code/datums/emergency_calls/emergency_call.dm Show resolved Hide resolved
@Zonespace27 Zonespace27 marked this pull request as draft October 7, 2023 16:03
@Diegoflores31 Diegoflores31 marked this pull request as ready for review October 23, 2023 01:16
@realforest2001 realforest2001 marked this pull request as draft October 25, 2023 15:57
@Diegoflores31 Diegoflores31 marked this pull request as ready for review November 1, 2023 21:05
@Diegoflores31
Copy link
Contributor Author

github web moment 💀

Copy link
Member

@fira fira 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 codewise

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 Nov 18, 2023
@Diegoflores31 Diegoflores31 changed the title SLIDE YOUR ID LIUTENANT ERT removal Nov 26, 2023
@Diegoflores31 Diegoflores31 changed the title ERT removal Call ERT removal Nov 26, 2023
@harryob harryob added this pull request to the merge queue Nov 26, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 26, 2023
@Nanu308 Nanu308 added this pull request to the merge queue Nov 29, 2023
@Diegoflores31 Diegoflores31 changed the title Call ERT removal Keycard authenticator can call ERT winout admin intervention Dec 12, 2023
@fira
Copy link
Member

fira commented Dec 12, 2023

Testing take 2 to confirm this effectively works as intended

@ihatethisengine
Copy link
Contributor

Hello, diego!

I think in general this is a great feature, but it's absolutely unusable in the current state. There is no working feedback whatsoever. You never know why you fail. Is it a second lieutenant who failed to swipe the ID in time? Did you not fulfill some requirements? Did it just bugged out? Please add messages on fail describing what go wrong. I see there are messages you added but they don't work in-game. Probably you should rework the messages the way it shows visible message for everyone around the swiping thing, not to usr which is probably null.

Also, as far as I am aware, you can swipe the card for ERT only after the crash, which is extremely nonviable. Let people call ERT just after hijack, you can just forbid it from landing before crash or add a timer.

Sincerely, your good friend.

@Diegoflores31
Copy link
Contributor Author

Diegoflores31 commented Dec 19, 2023

There is currently a bug that some times your swipes wont register . I am almost 100% its because of table code . I will probably add some additional features today to make it more intuitive

Copy link
Contributor

@Drulikar Drulikar left a comment

Choose a reason for hiding this comment

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

I think the issue with this is mostly just confusion/feedback to users.

Consider adding a sound/balloon_alert whenever the keycard_auth activates/times out (I believe that occurs here: https://github.com/cmss13-devs/cmss13/blob/master/code/modules/security_levels/keycard_authentication.dm#L116-L132

Also consider adding a message if a different user tries to authorize the same keycard_auth - or allow the same machine to both initiate and confirm a request if the triggerer/confirmer is a different user.

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 Dec 27, 2023
@Drulikar Drulikar marked this pull request as draft December 31, 2023 08:51
@github-actions github-actions bot closed this Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Feature coder badge Fix Fix one bug, make ten more Stale beg a maintainer to review your PR Testmerge Candidate we'll test this while you're asleep and the server has 10 players
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ERTs called via card-swiping do not actually call an ERT.
8 participants