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

allow syringes to set transfer amount #12173

Merged

Conversation

JustinTrotter
Copy link
Contributor

@JustinTrotter JustinTrotter commented Oct 23, 2022

About the PR

So I decided to try out playing as the sole chemist with only using syringes. Making the stuff was a breeze and was more engaging than the current pill meta (where you just scatter a massive amount of pills on the table for the public to use all willy-nilly). I was able to give out precise doses and was happy with the results.

The crew, on the other hand, were not happy. They wanted their pills and they wanted them badly, to the point of breaking into my lab making them themselves. I would ask them why they didn't want to bother with the syringes and the major pain point was the speed in administering the drugs. Crew are limited to only 15u per syringe and could only inject 5u per doafter.

Solution I've come up with is to allow syringes to have their solution transfer amount to be configurable just like any other container. It still defaults to 5u, but can be set to 10 or 15u as necessary. This in my opinion buffs syringes just enough to make them more palatable for crew to use.

Screenshots

syringe

Changelog

🆑

  • add: Syringes can now set how much solution to transfer.

@github-actions github-actions bot added the S: Needs Review Status: Requires additional reviews before being fully accepted label Oct 23, 2022
Comment on lines 73 to 74
component.TransferAmount = FixedPoint2.New(amount);
args.User.PopupMessage(Loc.GetString("comp-solution-transfer-set-amount", ("amount", amount)));
Copy link
Member

Choose a reason for hiding this comment

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

Maybe make the delay change based on the amount transferred? Something like 1 second per 5u? Still faster than multiple injections, but gives the target a bit more time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Done.

Comment on lines 53 to 59
// Custom transfer verb
AlternativeVerb custom = new();
custom.Text = Loc.GetString("comp-solution-transfer-verb-custom-amount");
custom.Category = VerbCategory.SetTransferAmount;
custom.Act = () => component.UserInterface?.Open(actor.PlayerSession);
custom.Priority = 1;
args.Verbs.Add(custom);
Copy link
Member

Choose a reason for hiding this comment

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

In SS13, it only allows 5, 10, and 15 for syringes. No custom amounts. For smaller amounts, players would use a dropper for 1-5. Should that be replicated here or should it just all be done using syringes?

Copy link
Contributor Author

@JustinTrotter JustinTrotter Oct 24, 2022

Choose a reason for hiding this comment

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

Yeah I'll strip out the custom amount.

Update: Stripped.

Comment on lines 45 to 48
private void AddSetTransferVerbs(EntityUid uid, InjectorComponent component, GetVerbsEvent<AlternativeVerb> args)
{
if (!args.CanAccess || !args.CanInteract || args.Hands == null)
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is double-indented

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Derp.

Comment on lines 75 to 78
}


private static void OnInjectionCancelled(InjectionCancelledEvent ev)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should just be one blank line here.

@@ -64,5 +81,7 @@ public InjectorToggleMode ToggleState
Dirty();
}
}

[ViewVariables] public BoundUserInterface? UserInterface => Owner.GetUIOrNull(TransferAmountUiKey.Key);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed? Doesn't look like it's used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RIght. This was used in the custom transfer amount that was stripped out, thanks.

Comment on lines 25 to 31
{

/// <summary>
/// Default transfer amounts for the set-transfer verb.
/// </summary>
public static readonly List<int> DefaultTransferAmounts = new() { 5, 10, 15};
private void InitializeInjector()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{
/// <summary>
/// Default transfer amounts for the set-transfer verb.
/// </summary>
public static readonly List<int> DefaultTransferAmounts = new() { 5, 10, 15};
private void InitializeInjector()
{
/// <summary>
/// Default transfer amounts for the set-transfer verb.
/// </summary>
public static readonly List<int> DefaultTransferAmounts = new() { 5, 10, 15};
private void InitializeInjector()

Also maybe just call this TransferAmounts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see any difference in the suggested change diff. Updated the name as requested.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just whitespace. There should be a blank line before the first method.

if (!args.CanAccess || !args.CanInteract || args.Hands == null)
return;

if (!EntityManager.TryGetComponent<ActorComponent?>(args.User, out var actor))
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a TryGetComp proxy function you could use instead. It does the same it's just shorthand.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tho it looks like actor never gets used so it should probably be HasComp.

@ShadowCommander
Copy link
Member

Looks good. Just so you know, in the future when you finish part of a review you can hit the Resolve conversation button to minimize it and let the reviewer know that you addressed that part of the review.

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.

3 participants