-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Station AI ability to electricute doors #32012
base: master
Are you sure you want to change the base?
Station AI ability to electricute doors #32012
Conversation
RSI Diff Bot; head commit c1f9f60 merging into 2a6f15d Resources/Textures/Interface/Actions/actions_ai.rsi
|
Hype |
Resources/Textures/Interface/Actions/actions_ai.rsi/electrify_door.png
Outdated
Show resolved
Hide resolved
Resources/Textures/Interface/Actions/actions_ai.rsi/unelectrify_door.png
Outdated
Show resolved
Hide resolved
|
||
namespace Content.Shared.Silicons.StationAi; | ||
|
||
public abstract partial class SharedStationAiSystem | ||
{ | ||
private static readonly SoundPathSpecifier AirlockOverchargeDisabled = new("/Audio/Machines/airlock_overcharge_on.ogg"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldnt you put those in the component or would that break things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure where to put this. Maybe maint will propose good ideas. AirlockComponent is not used in this code path, ElectrifyComponent - well, fine but its still wierd... its only hear-able to AI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's only hear-able to AI, maybe put specifiers into AI Core or Held? It would allow change audio paths in prototypes...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@metalgearsloth what do you think about moving this to HeldComponent example? sounds a bit sus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be wherever stuff is electrified, also don't call it "overcharge" because it's just confusing what it's for.
"name": "unelectrify_door" | ||
}, | ||
{ | ||
"name": "door_overcharge_on" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New ones are CC0-1.0 by ScarKy0, don't really know how to add this info to license and copyright! :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inserted new files and mention side-by-side... folder fits too well...
…th door which have wire cut
Headphone warning for the Airhorn when electrifying the door. Ouch. My only complaints are the sound made when electrifying the door, why a airhorn? I also still don't like the sound for Emergency access either. Just my 2 cents. |
Its mostly due to the quality of the clips, i believe |
[Dependency] private readonly SharedElectrocutionSystem _electrify = default!; | ||
[Dependency] private readonly SharedAirlockSystem _airlocks = default!; | ||
[Dependency] private readonly SharedEyeSystem _eye = default!; | ||
[Dependency] protected readonly SharedMapSystem Maps = default!; | ||
[Dependency] private readonly SharedMindSystem _mind = default!; | ||
[Dependency] private readonly SharedMoverController _mover = default!; | ||
[Dependency] private readonly SharedTransformSystem _xforms = default!; | ||
[Dependency] private readonly SharedUserInterfaceSystem _uiSystem = default!; | ||
[Dependency] private readonly StationAiVisionSystem _vision = default!; | ||
[Dependency] private readonly SharedPopupSystem _popup = default!; | ||
[Dependency] protected readonly SharedPowerReceiverSystem PowerReceiver = default!; | ||
[Dependency] private readonly SharedAudioSystem _audio = default!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alphabetical and follow format of the existing stuff please I beg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fine, but it was not me who started it (39 line by original file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not 100% sure what u wanted but i sorted it all by field names.
var verb = new AlternativeVerb | ||
{ | ||
Text = isOpen ? Loc.GetString("ai-close") : Loc.GetString("ai-open"), | ||
Act = () => | ||
{ | ||
// no need to show menu if device is not powered. | ||
SharedApcPowerReceiverComponent? component = null; | ||
if (PowerReceiver.ResolveApc(ent.Owner, ref component) && !component.Powered) | ||
{ | ||
_popup.PopupClient(Loc.GetString("ai-device-not-responding"), user, PopupType.MediumCaution); | ||
return; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not hardcode in devices here, it's supposed to be generic. Use an event or anything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I swear PowerReceiver.IsPowered was throwing exception... but now i tested it and it works fine for all cases. Oh well...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
private void OnElectrified(EntityUid ent, ElectrifiedComponent component, StationAiElectrifiedEvent args) | ||
{ | ||
if (!TryComp<ElectrifiedComponent>(ent, out var electrified) | ||
|| !TryComp<StationAiWhitelistComponent>(ent, out var whiteList)) | ||
{ | ||
return; | ||
} | ||
|
||
SharedApcPowerReceiverComponent? apcPowerReceiverComponent = null; | ||
if ( | ||
!whiteList.Enabled | ||
|| electrified.IsWireCut | ||
|| ( | ||
PowerReceiver.ResolveApc(ent, ref apcPowerReceiverComponent) | ||
&& !apcPowerReceiverComponent.Powered | ||
) | ||
) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whitelist comp gets handled in the BUI attempt event, having to check this manually every single time is bug prone and shouldn't be done.
Just add the whitelist enabled check to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll send popup from there, ye. It checks out pretty well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
: AirlockOverchargeEnabled; | ||
_audio.PlayEntity(soundToPlay, args.User, ent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the predicted version this isn't predicted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
if (!whiteList.Enabled || !component.Powered) | ||
{ | ||
_popup.PopupClient(Loc.GetString("ai-device-not-responding"), args.User, PopupType.MediumCaution); | ||
return; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AirlockComponent.Powered is obsolete please don't use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
used PowerReceiver.IsPowered instead. we might need to mark it obsolete and remove related code btw... its really misleading...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it true for bolts component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, and used PowerReceiver system
|
||
if (!whiteList.Enabled || component.BoltWireCut || !component.Powered) | ||
{ | ||
_popup.PopupClient(Loc.GetString("ai-device-not-responding"), args.User, PopupType.MediumCaution); | ||
return; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latter 3 checks should be handled via a setbolts overload not here because other callers will forget to do this.
Enabled as mentioned should be handled in the BUI message attempt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to add TrySetBolt to send popup in case it fails
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
…ctricute-for-real
…ctricute-for-real
Updated media with cool (and more up to date) video from ScarKy0 |
…/closing when ai wire was cut
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me
|| !HasComp<StationAiHeldComponent>(args.User) | ||
|| !HasComp<StationAiWhitelistComponent>(args.Target)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this whitelist check is unneeded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, i belive, currently overlapping with OnHeldInteraction, but i'm not 100% sure it covers every possible scenario
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seemed good when i tested it - so i removed condition check duplication!
StationAiWhitelistComponent? whitelistComponent = null; | ||
if (TryComp(ev.Actor, out StationAiHeldComponent? aiComp) && | ||
(!ValidateAi((ev.Actor, aiComp)) || | ||
!HasComp<StationAiWhitelistComponent>(ev.Target))) | ||
!TryComp(ev.Target, out whitelistComponent))) | ||
{ | ||
ev.Cancel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just inline the component no reason to leave it separate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean - use one block? and always call ShowDeviceNotRespondingPopup? this doesn't sound right - popup is kinda bound to whitelistComponent is { Enabled: false } scenario - there is no reason to show it if ai is not valid or we have no StationAiHeldComponent comp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot inline declaration because then declaration (which is required in the next lines) will be optional (based on previous conditions eval) and code won't compile :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And i don't want popup spam on every click! :/ so i want popup in separate condition... otherwise it will spam popup on every click, anywhere (walls included).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It absolutely will compile, the only reason it won't is if you don't get the component out of it, just adjust the if statement for it.
private void OnElectrified(EntityUid ent, ElectrifiedComponent component, StationAiElectrifiedEvent args) | ||
{ | ||
if (!TryComp<ElectrifiedComponent>(ent, out var electrified)) | ||
{ | ||
return; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This component is doubled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!yea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
public void SetElectrifiedWireCut(Entity<ElectrifiedComponent> ent, bool value) | ||
{ | ||
ent.Comp.IsWireCut = value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should guard these values behind equals checks to avoid dirtying if the value hasn't changed because dirtying is expensive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
/// <summary> | ||
/// Sound to play when the airlock emergency access is turned on. | ||
/// </summary> | ||
[DataField, ViewVariables(VVAccess.ReadWrite)] | ||
public SoundSpecifier EmergencyOnSound = new SoundPathSpecifier("/Audio/Machines/airlock_emergencyon.ogg"); | ||
|
||
/// <summary> | ||
/// Sound to play when the airlock emergency access is turned off. | ||
/// </summary> | ||
[DataField, ViewVariables(VVAccess.ReadWrite)] | ||
public SoundSpecifier EmergencyOffSound = new SoundPathSpecifier("/Audio/Machines/airlock_emergencyoff.ogg"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VVRW is redundant with datafield.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
var sound = ent.Comp.EmergencyAccess ? ent.Comp.EmergencyOnSound : ent.Comp.EmergencyOffSound; | ||
if (predicted) | ||
Audio.PlayPredicted(sound, ent, user: user); | ||
else | ||
Audio.PlayPvs(sound, ent); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do any of the callers even need this? This method is new...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AdminVerbSystem uses non-predicted version. is that wrong?
/// <summary> | ||
/// Sets electrified value of component and marks dirty if required. | ||
/// </summary> | ||
public void SetElectrified(Entity<ElectrifiedComponent> ent, bool value) | ||
{ | ||
var oldValue = ent.Comp.Enabled; | ||
ent.Comp.Enabled = value; | ||
if (value != oldValue) | ||
{ | ||
Dirty(ent, ent.Comp); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You did it here but also this is written in a weird way, just check if the values are the same before assigning (not sure if the JIT even accounts for this?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
|
||
namespace Content.Shared.Silicons.StationAi; | ||
|
||
public abstract partial class SharedStationAiSystem | ||
{ | ||
private static readonly SoundPathSpecifier AirlockOverchargeDisabled = new("/Audio/Machines/airlock_overcharge_on.ogg"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be wherever stuff is electrified, also don't call it "overcharge" because it's just confusing what it's for.
StationAiWhitelistComponent? whitelistComponent = null; | ||
if (TryComp(ev.Actor, out StationAiHeldComponent? aiComp) && | ||
(!ValidateAi((ev.Actor, aiComp)) || | ||
!HasComp<StationAiWhitelistComponent>(ev.Target))) | ||
!TryComp(ev.Target, out whitelistComponent))) | ||
{ | ||
ev.Cancel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It absolutely will compile, the only reason it won't is if you don't get the component out of it, just adjust the if statement for it.
[Dependency] private readonly ISharedAdminManager _admin = default!; | ||
[Dependency] private readonly IGameTiming _timing = default!; | ||
[Dependency] private readonly INetManager _net = default!; | ||
[Dependency] private readonly ItemSlotsSystem _slots = default!; | ||
[Dependency] private readonly ItemToggleSystem _toggles = default!; | ||
[Dependency] private readonly ActionBlockerSystem _blocker = default!; | ||
[Dependency] private readonly MetaDataSystem _metadata = default!; | ||
[Dependency] private readonly SharedAirlockSystem _airlocks = default!; | ||
[Dependency] private readonly SharedAppearanceSystem _appearance = default!; | ||
[Dependency] private readonly SharedAudioSystem _audio = default!; | ||
[Dependency] private readonly ActionBlockerSystem _blocker = default!; | ||
[Dependency] private readonly SharedContainerSystem _containers = default!; | ||
[Dependency] private readonly SharedDoorSystem _doors = default!; | ||
[Dependency] private readonly SharedElectrocutionSystem _electrify = default!; | ||
[Dependency] private readonly SharedEyeSystem _eye = default!; | ||
[Dependency] protected readonly SharedMapSystem Maps = default!; | ||
[Dependency] private readonly SharedMindSystem _mind = default!; | ||
[Dependency] private readonly MetaDataSystem _metadata = default!; | ||
[Dependency] private readonly SharedMindSystem _mind = default!; | ||
[Dependency] private readonly SharedMoverController _mover = default!; | ||
[Dependency] private readonly SharedTransformSystem _xforms = default!; | ||
[Dependency] private readonly INetManager _net = default!; | ||
[Dependency] private readonly SharedPopupSystem _popup = default!; | ||
[Dependency] private readonly ItemSlotsSystem _slots = default!; | ||
[Dependency] private readonly IGameTiming _timing = default!; | ||
[Dependency] private readonly ItemToggleSystem _toggles = default!; | ||
[Dependency] private readonly SharedUserInterfaceSystem _uiSystem = default!; | ||
[Dependency] private readonly StationAiVisionSystem _vision = default!; | ||
[Dependency] private readonly SharedTransformSystem _xforms = default!; | ||
[Dependency] protected readonly SharedMapSystem Maps = default!; | ||
[Dependency] private readonly SharedPowerReceiverSystem PowerReceiver = default!; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a dog's breakfast please just make it alphabetical like it used to be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thats a wierd way to sort it. ok, done.
About the PR
AI now have ability to electrify airlocks.
AI now have ability to set door to EmergencyAccess (yellow-lights-door).
AI now have better feedback on actions with airlocks: new icons for radial menu, sound effects
Why / Balance
AI can now help low-pop crew by setting some non-critical doors to emergency-access, enabling people to access medical self-care without breaking law (and doors).
AI can have quite harmful laws but it doesn't have any way to implement them. Electrifying doors is first step how AI can dominate human race and show who is master of the station (in not entirely gamebreaking way).
Technical details
ElectrifiedComponent was moved to shared to indicate is it On/Off for AI actions radial menu. It is marked dirty on IsEnabled change, logic was not transferred to shared.
SharedAirlockSystem.ToggleEmergencyAccess was changed to SetEmergencyAccess so AI commands can be more predictable.
Media
device_not_responding.1.mp4
bolt_showcase.mp4
overcharge_showcase.mp4
ai_wire_showcase.mp4
Requirements
Breaking changes
none
Changelog
🆑 Fildrance, ScarKy0