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

Add the bionic syrinx implant #339

Merged
merged 14 commits into from
Nov 13, 2023
Merged

Conversation

luringens
Copy link
Contributor

@luringens luringens commented Oct 27, 2023

About the PR

Adds the bionic syrinx implant, a new syndicate uplink purchase for harpies that acts as an implantanble voice changer.

Why / Balance

From the design of @evilexecutive, this is a harpy-themed version of the voice mask that let them play into the species trait of mimicry. It costs the same as a voice mask, trading the chameleon feature for being implanted. It breaks when implanted to a non-harpy.

Technical details

Reuses most of the same code as voice masks, copying some bits to avoid conflicting. Implementation differences:

  • The action is given by implantAction in yaml instead of from an equipment system
  • The VoiceMaskComponent is added and removed to the player through a tiny new SubdermalBionicSyrinxImplantSystem instead of VoiceMaskSystem.Equip
  • VoiceMaskComponent is now mirrored in SyrinxVoiceMaskComponent to deconflict

Behaviour is identical to the voice mask with only one difference. When equipped, the voice mask immediately starts masking your name as "Unknown". This is a bit weird for a voice implant, so instead I default it to the canonical name of the entity.

Media

  • I have added screenshots/videos to this PR showcasing its changes ingame, or this PR does not require an ingame showcase
syrinx.mp4

Breaking changes

N/A

Changelog

🆑

  • add: Syndicate harpies can now purchase bionic syrinx implants to further enchance their mimicry skills. It's like a voice mask for your throat!

@luringens
Copy link
Contributor Author

luringens commented Oct 27, 2023

Remaining work:

  • Add a proper action icon
  • Feedback from @evilexecutive, especially regarding names and descriptions
  • Limit functionality to harpies
  • Fix breakage when a voicemask is equipped while implanted

@DebugOk
Copy link
Contributor

DebugOk commented Oct 27, 2023

Is there anything stopping a non harpy from using this implant?

@luringens
Copy link
Contributor Author

Not currently, purchase is limited to harpies, but usage is not. I can add a species check before the component is applied if we want that.

@FluffiestFloof
Copy link
Contributor

FluffiestFloof commented Oct 27, 2023

It probably be best if the idea is to enhance/modify the Harpy's ability to mimic voice and not give that ability to any random specie.

@DebugOk
Copy link
Contributor

DebugOk commented Oct 27, 2023

It probably be best if the idea is to enhance/modify the Harpy's ability to mimic voice and not give that ability to any random specie.

Which is what this implant should be doing

@VMSolidus
Copy link
Contributor

Should probably add a check that makes it not work for anyone not a harpy. I'd have suggested preventing implanting in a non harpy, and while thats better, it would involve touching wizden code

@VMSolidus
Copy link
Contributor

VMSolidus commented Oct 28, 2023

To resolve the issues with it breaking if you also wear a voicemask, you're probably going to straight up need to clone the voicemask code, strip out all the gas mask functionality, change its BoundUserInterface, and also add a UserInterface entry to Species/harpy.yml to support the new UI command. Iirc this is what I ran into as well when I first tried making these, but didn't know I needed a UserInterface entry.

That will not prevent a harpy from using both the implant and the voicemask, just add a redundant action. Honestly that's a skill issue for players if they buy both, because they don't need both.
image

@VMSolidus
Copy link
Contributor

Alright as far as the writing side of things goes, we should add a little text that pops up if a non-harpy tries to implant, and where it fails to give them the component. Maybe something like, "You don't have a syrinx to augment". I think your implant description is fine, and I've also pinged the creator of the harpysinger icon to see if she'd like to take a shot at the bionic syrinx icon as well.

Also holy shit did Wizden massively reduce the cost of the voicemask? When I originally tried to make this implant, the voicemask cost 6tc!

@luringens
Copy link
Contributor Author

@evilexecutive Yeah, something about making sneaky gadgets cheaper to encourage syndies to not always go loud.

Update:

  • I copied over and adapted the minimum amount of code to make it deconflict with the mask. If you have both you now still get two actions, but they open the same UI window and effectively share the same state. They act like one, and putting on or taking off one or the other doesn't change anything.
  • I also threw in an argument in upstream code to not log an error for VoiceMask component not resolving, since the code handles it gracefully and it's now expected behaviour that either VoiceMask or Syrinx is there but not the other.
  • There is a minor bug where if you implant two syrinxes and then remove one, the remaining one remains nonfunctional. Seems unlikely to happen in practice...?
  • If you try to implant it as non-harpy it just breaks like the mindshield does on headrevs:
    image
  • Haven't figured out implant action icon yet, it just inherits the corner scream like the mask has:
    image

@VMSolidus
Copy link
Contributor

  • type: entity
    id: ActionChangeVoiceMask
    name: Set name
    description: Change the name others hear to something else.
    noSpawn: true
    components:
    • type: InstantAction
      icon: Interface/Actions/scream.png # somebody else can figure out a better icon for this
      event: !type:VoiceMaskSetNameEvent

You should be able to put this somewhere in any yml file, just change the names and description to match the bionic syrinx stuff. This will also let you define what the icon is.

@VMSolidus
Copy link
Contributor

Oh yea, the most rational place to put the Voicemask UI code is to add it to the end of Species/harpy.yml, since that's also coincidentally where we put the UI code for HarpySinger. Although realistically it could be placed arbitrarily anywhere, I feel this makes the most sense since it's a component only Harpies use, and there's no other dedicated place for icon definitions like that. Usually icon definitions get placed in the same code as the items/entities that use them anyway.

@github-actions github-actions bot added the S: Merge Conflict Fix your PR! label Nov 1, 2023
Copy link
Contributor

github-actions bot commented Nov 1, 2023

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

1 similar comment
Copy link
Contributor

github-actions bot commented Nov 1, 2023

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

@github-actions github-actions bot added Changes: C# Changes any cs files Changes: Localization Changes any ftl files Changes: YML Changes any yml files and removed S: Merge Conflict Fix your PR! labels Nov 3, 2023
@luringens
Copy link
Contributor Author

That worked well, thanks for the pointer! I edited the existing harpy sing icon as a starting point. I've also removed the species check in favour of the new implant whitelist that got merged recently, which is so much simpler.

image

@luringens
Copy link
Contributor Author

Should I add an action to reset the current name back to the original canonical name? With mask you can just take it off and shove it in your backpack if you need to (literally) drop the mask quickly, but with an implant you've currently got to type it back out. An extra action would be easy, though it'd be more consistent to get that as a button in the name change popup. But that requires editing or duplicating upstream code, soo...

@VMSolidus
Copy link
Contributor

Yea, I'd say skip out for now on having the reset button, especially since thats more an upstream thing to do. For now, this PR looks to be complete!

@luringens luringens marked this pull request as ready for review November 3, 2023 18:30
@luringens luringens requested a review from Colin-Tel as a code owner November 3, 2023 18:30
@luringens luringens requested a review from DebugOk November 3, 2023 19:34
@luringens
Copy link
Contributor Author

I'm not sure why Github put a Merge Conflict tag on this, that was resolved quite a while ago.

Copy link
Contributor

@Colin-Tel Colin-Tel left a comment

Choose a reason for hiding this comment

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

image

Using the implanter on someone after its been consumed gives a message that is missing locale. It also throws an error on the server side.

Apparently this is not exclusive to your implant, and actually triggers with the storage implanter as well.

Copy link
Contributor

@Colin-Tel Colin-Tel left a comment

Choose a reason for hiding this comment

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

If a player sets a valid name in the "Voice Mask Name Change" menu, upon hitting "Set name", the confirmation message is hidden behind the Voice Mask Name Change window.

You should either have the window close upon a valid name being set, or have the confirmation message above the player, like how it is when you attempt to set an invalid name.
image

@luringens
Copy link
Contributor Author

luringens commented Nov 13, 2023

Using the implanter on someone after its been consumed gives a message that is missing locale. It also throws an error on the server side.

Yeah, that's an upstream issue with the implanter item.

If a player sets a valid name in the "Voice Mask Name Change" menu, upon hitting "Set name", the confirmation message is hidden behind the Voice Mask Name Change window.

As above, the issue is in upstream voice mask code oh wait i duplicated this, right. Still going to PR it upstream for consistency with the voice mask.

I can fix these two issues with a PR to upstream if that keeps conflicts easier to manage for us?

@luringens luringens requested a review from Colin-Tel November 13, 2023 20:12
@luringens
Copy link
Contributor Author

Copy link
Contributor

@Colin-Tel Colin-Tel 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!

@Colin-Tel Colin-Tel merged commit 7b0c061 into DeltaV-Station:master Nov 13, 2023
11 checks passed
DeltaV-Bot pushed a commit that referenced this pull request Nov 13, 2023
DebugOk pushed a commit to DebugOk/Delta-v that referenced this pull request Jan 20, 2024
* Add the bionic syrinx implant

* Make syrinx implant nonfunctional for non-harpies

* Deconflict syrinx with voice mask

* Don't allow non-harpies to inject a bionic syrinx

* Use the new implant whitelist for syrinx instead

* Add an action icon to the syrinx voicemask

* Remove now-obsolete syrinx implant error messages

* Move syrinx popups to player and to clientside

(cherry picked from commit 7b0c061)
DebugOk pushed a commit to DebugOk/Delta-v that referenced this pull request Jan 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes: C# Changes any cs files Changes: Localization Changes any ftl files Changes: YML Changes any yml files S: Needs Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants