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

fix: AI should not have alt-menu option and alt-menu-related verb in drop-down menu as it have no options in it #34468

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Fildrance
Copy link
Contributor

@Fildrance Fildrance commented Jan 16, 2025

About the PR

Now AI core doesn't have radial menu related verb on it. It had no options ready for radial menu (so if it could work, it would display just empty radial), and radial menu display couldn't work because it requires core to be powered using APC (which it is not).

Why / Balance

Useless option raised questions - look at https://discord.com/channels/310555209753690112/770682801607278632/1329502538591895676

Technical details

After short investigation the following was found:

  1. verb was always available and is bound only to target entity having StationAiWhitelistComponent (no bueno).
  2. if we restrict verb based on amount of options GetStationAiRadialEvent returns (as there is no reason to display menu if its empty) we suddenly discover that most of consoles and devices have ElectrifiedComponent which adds electrocute option to list of available actions (using GetStationAiRadialEvent). This is really concerning and looks like a mistake, as none of those devices can electrocute user currently (and in the past). Most likely its technical debt thats left after moving power logic to ApcPowerReceiverComponent (thats just my guess).
  3. The only reason why radial menu currently is not displayed (and cannot let you electrify consoles) is that consoles are missing UI marker for radial menu.

To minimize possible side effects of changes in PR - i just added check, if required ui is missing on entity.

Media

before

image

after

image

Requirements

Breaking changes

none

Changelog

🆑 Fildrance

  • fix: removed unused 'Open actions' verb on AI Core

@Fildrance Fildrance requested a review from DrSmugleaf as a code owner January 16, 2025 19:34
@github-actions github-actions bot added size/XS Denotes a PR that changes 0-9 lines. Changes: UI Changes: Might require knowledge of UI design or code. S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. S: Needs Review Status: Requires additional reviews before being fully accepted labels Jan 16, 2025
@Fildrance
Copy link
Contributor Author

Checked that all the default stuff (door interactions, consoles) still works for AI

@ScarKy0 ScarKy0 added T: Bugfix Type: Bugs and/or bugfixes P3: Standard Priority: Default priority for repository items. DB: Beginner Friendly Difficulty: Great for beginners. Unambiguous in scope, and explains how to achieve the result. A: Silicons Area: Relates to Silicon roles, including AI. and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Jan 16, 2025
@ScarKy0
Copy link
Contributor

ScarKy0 commented Jan 16, 2025

Tested in game and works
Though, is there any solution for other entities with StationAiWhitelist? Currently every device with it has "Open actions", or at least all I checked

@ScarKy0 ScarKy0 self-assigned this Jan 16, 2025
@slarticodefast
Copy link
Member

@Errant-4
Any particular reason you added the whitelist to the AI core?
grafik

@Fildrance
Copy link
Contributor Author

Fildrance commented Jan 17, 2025

Tested in game and works Though, is there any solution for other entities with StationAiWhitelist? Currently every device with it has "Open actions", or at least all I checked

Hmm, i missed that part. I guess better fix it all at once? I think the solution would be to add some new field for Whitelist component and set it True only for doors... as they are the only ones who have interactions right now? I thought that consoles can be electrified too now that they have wires...

@ScarKy0
Copy link
Contributor

ScarKy0 commented Jan 17, 2025

Hmm, i missed that part. I guess better fix it all at once? I think the solution would be to add some new field for Whitelist component and set it True only for doors... as they are the only ones who have interactions right now? I thought that consoles can be electrified too now that they have wires...

Ideally the event for radials would say its empry and never give the verb unless theres associated menu options, but dunno how doable it is

@Fildrance
Copy link
Contributor Author

Ideally the event for radials would say its empry and never give the verb unless theres associated menu options, but dunno how doable it is

Thats also certainly doable - it'll just require sending event twice - first time for collecting verbs, second time to collect buttons. Maybe thats the right way. Gonna do it today/tomorrow

@ScarKy0
Copy link
Contributor

ScarKy0 commented Jan 17, 2025

Thats also certainly doable - it'll just require sending event twice - first time for collecting verbs, second time to collect buttons. Maybe thats the right way. Gonna do it today/tomorrow

That does sound like a better solution

@Errant-4
Copy link
Member

@slarticodefast It was already there, rider just removed a whitespace when I edited other parts of the file
image

@Fildrance
Copy link
Contributor Author

Well, funny thing - collecting actions for every console returns electrify option because it have ElectrifyComponent. I'm a bit puzzled why its there and how to approach this, to be perfectly honest. Sloth proposed to use stationaiwhitelist , maybe i'll add some flag there, we will see...

@Fildrance
Copy link
Contributor Author

Updated description with investigation details!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: Silicons Area: Relates to Silicon roles, including AI. Changes: UI Changes: Might require knowledge of UI design or code. DB: Beginner Friendly Difficulty: Great for beginners. Unambiguous in scope, and explains how to achieve the result. P3: Standard Priority: Default priority for repository items. S: Needs Review Status: Requires additional reviews before being fully accepted size/XS Denotes a PR that changes 0-9 lines. T: Bugfix Type: Bugs and/or bugfixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants