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

feat: now when research is unlocked in console, approver of reasearch is radio-ed too #31170

Merged

Conversation

Fildrance
Copy link
Contributor

@Fildrance Fildrance commented Aug 18, 2024

About the PR

Research console now is announcing who approved tech unlock.

Why / Balance

It should be same as cargo requests. Also resolves internal conflicts when someone researches only what he wants for points that all team is bringing up (had some rounds where RD was not liking it and had to remove consoles from all scientists instead of just firing one dude).

Technical details

Simple changes, ALMOST one-liner.

Media

emag_cargo.webm
emag_research.webm
borg_is_logged.webm
borg_name_in_consoles.webm

Requirements

  • [x ] I have read and I am following the Pull Request Guidelines. I understand that not doing so may get my pr closed at maintainer’s discretion
  • [x ] I have added screenshots/videos to this PR showcasing its changes ingame, or this PR does not require an ingame showcase

Breaking changes

Changelog

🆑

  • add: When research is unlocked in console the approver of the research is named.
  • add: Borgs door access is getting logged now (and is accessible in Log Probe Cartridge)
  • add: e-magged research and cargo consoles are not radio-ing any messages on research/buy confimation (for anyone)

@Fildrance Fildrance marked this pull request as ready for review August 18, 2024 18:54
@github-actions github-actions bot added the S: Needs Review Status: Requires additional reviews before being fully accepted label Aug 18, 2024
@deathride58 deathride58 added the S: Undergoing Maintainer Discussion Status: Currently going through an extended discussion amongst maintainers, as per procedure. label Aug 19, 2024
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.

Currently borgs show up with an empty name when they approve research because they have no ID. Do the same thing as in #30107 to fix that.
Also make the formatting for the name and job the same as there for consistency reasons.

@slarticodefast slarticodefast 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 Aug 21, 2024
@deltanedas
Copy link
Contributor

should really have an event that access logs, comms, cargo, research etc all raise and borg handles it once

@slarticodefast
Copy link
Member

slarticodefast commented Aug 21, 2024

should really have an event that access logs, comms, cargo, research etc all raise and borg handles it once

Yeah, that would be even better. Avoids hardcoding the borgchassis into other systems.

pa.pecherskij added 6 commits August 21, 2024 21:25
…r logged use TryGetIdentityShortInfoEvent which is subscibed by id-card system and borg system (to work for both carbon and synthetic life-forms)
…red, fixed cargo cent-com-originated orders
@Fildrance
Copy link
Contributor Author

should really have an event that access logs, comms, cargo, research etc all raise and borg handles it once

fixed. code related to access logs is wierd and required checkin property on id card... but oh well.

@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 Aug 21, 2024
@Everturning
Copy link

I dislike this bc it makes it harder to hide that you're antag. What is the benefit of emagging a protolathe if you cant research the cool sec tech in secret.
Right now if you research sec items then people will notice but there will still be an air of suspicion and suspense, and they'll have to figure out who and why it would be researched. This removes that rp possibility.

@Fildrance
Copy link
Contributor Author

I dislike this bc it makes it harder to hide that you're antag. What is the benefit of emagging a protolathe if you cant research the cool sec tech in secret. Right now if you research sec items then people will notice but there will still be an air of suspicion and suspense, and they'll have to figure out who and why it would be researched. This removes that rp possibility.

e-mag console and remove your id, then research- it will work and leave 'approver' blanc.

@Everturning
Copy link

I dislike this bc it makes it harder to hide that you're antag. What is the benefit of emagging a protolathe if you cant research the cool sec tech in secret. Right now if you research sec items then people will notice but there will still be an air of suspicion and suspense, and they'll have to figure out who and why it would be researched. This removes that rp possibility.

e-mag console and remove your id, then research- it will work and leave 'approver' blanc.

removing id will prevent you from researching. It's access locked.

@Fildrance
Copy link
Contributor Author

I dislike this bc it makes it harder to hide that you're antag. What is the benefit of emagging a protolathe if you cant research the cool sec tech in secret. Right now if you research sec items then people will notice but there will still be an air of suspicion and suspense, and they'll have to figure out who and why it would be researched. This removes that rp possibility.

e-mag console and remove your id, then research- it will work and leave 'approver' blanc.

removing id will prevent you from researching. It's access locked.

Not if u emag console, then any1 can do research

@SlamBamActionman
Copy link
Member

You can also use a Radio Jammer, as the radio message originates from the console (if I recall) and turning the jammer on will prevent it from sending out a message.

At least it does for Cargo. I've ordered contraband stuff right under QM's nose this way.

@Fildrance
Copy link
Contributor Author

You can also use a Radio Jammer, as the radio message originates from the console (if I recall) and turning the jammer on will prevent it from sending out a message.

At least it does for Cargo. I've ordered contraband stuff right under QM's nose this way.

If this is not working with research console - file issue and i'll fix it separately : >

@IProduceWidgets
Copy link
Contributor

IProduceWidgets commented Aug 30, 2024

I think it needs some player feedback for if you cant hear the science radio. Like a popup over the console or something.

@Fildrance
Copy link
Contributor Author

I think it needs some player feedback for if you cant hear the science radio. Like a popup over the console or something.

that sounds reasonable, but again, separate issue - as it is required for all consoles, and is not connected to original problem not getting researcher in radio message. Fila an issue, link it, POSSIBLY will fix it... lets not feature creep this thing, lets merge it without possibly breaking massive changes <_<

Copy link
Contributor

github-actions bot commented Sep 1, 2024

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

@github-actions github-actions bot added the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Sep 1, 2024
@SlamBamActionman
Copy link
Member

Hello! This PR was put up to a maintainer vote with the result: Merge.

Ensure that it works after the merge conflict is resolves. I'm also curious of how the message is handled when the console is emagged and used by someone without an ID. Ensure it doesn't just go "researched by .", it would look a bit weird.

@github-actions github-actions bot removed the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Sep 2, 2024
@Fildrance
Copy link
Contributor Author

Changed emag consoles (cargo and research) to not output messages. I tested it (see media)

@UbaserB UbaserB merged commit 6380a9a into space-wizards:master Sep 3, 2024
11 checks passed
@UbaserB UbaserB removed the S: Undergoing Maintainer Discussion Status: Currently going through an extended discussion amongst maintainers, as per procedure. label Sep 3, 2024
@Fildrance Fildrance deleted the feature/radio-researched-by-whom branch September 3, 2024 14:23
Erisfiregamer1 pushed a commit to The-Arcadis-Team/arc-station-14 that referenced this pull request Jan 9, 2025
… is radio-ed too (space-wizards#31170)

* feat: now when research is unlocked in console, approver of reasearch is radio-ed too

* refactor: now most of events that require actor name to be radio-ed or logged use TryGetIdentityShortInfoEvent which is subscibed by id-card system and borg system (to work for both carbon and synthetic life-forms)

* refactor: moved TryGetIdentityShortInfoEvent on id card system to shared, fixed cargo cent-com-originated orders

* remove unused check

* refactor: decoupled systems from IdCardSystem (those that are not dependent on it anymore)

* refactor: removed unuseed usings

* feat: emagged cargo/research consoles wont radio messages on buy/research confirm anymore

---------

Co-authored-by: pa.pecherskij <pa.pecherskij@interfax.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S: Needs Review Status: Requires additional reviews before being fully accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants