KSPAction.active check

[SirDiazo]

Looking at the code when I commented on the DOACTION issue, I did not see a check for the .active bool on a KSPAction when KSP generates it's action lists.

https://github.com/KSP-KOS/KOS/blob/71738858aba22a46f90594c914f349bf57355988/src/kOS/Suffixed/PartModuleField/PartModuleFields.cs#L314

Is where I looked.

I think this needs to be added for two reasons:

1) If KSPAction.active is false, KSP will not execute the action even if you trigger it. (This probably needs to be confirmed again, last time I tested this was before KSP 1.0)

2) Modders use .active to show/hide the action in the editor for multi-mode partModules. I can't remember exactly what mod brought this issue to my attention (probably was KSPI or NearFuture) but there was a reactor that depending on which mode it was in would show/hide some actions that were only relevent in certain modes. This isn't any sort of "standard" for modding or anything, but its the only way I am aware of to hide an action from showing in the action groups editor mode of the VAB so it is what is used to hide actions by modders.

Now, perhaps this isn't as big a deal for kOS (you aren't showing actions in a GUI like my mod did), but I can still see the fact that an .active = false action just silently fails being an issue.

The other option would be to instead throw an error message that the KSPAction.active flag is false and the action has not executed?

[AlchemistCH]

Can you give some specification on how and when this is set? Is it just set for part prefab to be used in VAB? Does it stay the same for flight? Or is it subject to dynamic changes the same way as with events (buttons in right-click menu)? (Or only some some plugins use to change it in flight?)

I think MultiModeEngine uses this to hide engine activation and shutdown actions of the engine modules, replacing those with its own actions (that send the command to the currently active module)

However, my opinion is that calling hidden actions should not result in execution-breaking errors, because in the current state actions are used as fault-proof commands for particular partModule (unlike events, that can dynamically hide or change names). Maybe just adjustments for the list of actions to show that the particular action is currently hidden (so that calling it won't do anything for this module)?

[hvacengi]

It was my understanding that the .active member is set dynamically by the module, so that it can hide an action depending on various conditions. I'll probably need to look at it more carefully though. We check to see if events are visible, but we don't check to see if actions are active. That makes me think that the decision was that we would let the Action system filter that stuff out. It currently behaves as if you had activated an action group with that action disabled, I think. Can look at it more this evening.

[SirDiazo]

Okay, I have some facts and some guesses to add to the conversation.

First, as far as I am aware, no stock action ever sets BaseAction.active to false.

Rather, I believe that this is a workaround that modders have discovered in order to hide an action from the player in the action groups editor GUI.

Also, calling a BaseAction whos .active field is false causes no errors, the action just doesn't execute. (This needs confirmation, my last tests on this were KSP 0.25 or 0.90)

So, it is safe to call a BaseAction.active that is false from a kOS perspective. My concern that caused me to open this issue is that the player will get confused as to why they are calling an action they can see through kOS, but the action isn't doing anything. Then they will blame kOS when the cause of the issue actually is the fact that the BaseAction.active = false flag is set.

For my purposes in AGX, as that is an alternative editor GUI, I do in fact hide any actions who are .active = false, but for kOS purposes it may be a better approach to not actually change kOS's behavior, just log a warning message to the console that an action has tried to execute and failed to because of the fact that BaseAction.active = false.

D.

[AlchemistCH]

Well, getting no result from calling an action that has been hidden based on the current state of the module is no different from calling a one-way action when it's already at that state (like calling "activate" on something that is already activated). Stock behavior is probably to do nothing on such calls, but not to hide it. On the other hand, something that wasn't available in the VAB in the first place may be a reason to show warning. I'm pretty sure that's what happens with RAPIER (but there are the same action names on several modules)... P.S. I've checked RAPIER in a save file and it actually has active=false only on the actions of the currently inactive module, but not on the currently active. Now the bigger question is how it behaves in VAB. P.P.S. Scratch that, it always has active=false on the action of the primary engine module, but fails to set that on the secondary one for some reason (bug?). Anyway, the action groups always land on MultiModeEngine and go through it. Great - the stock behavior in this matter is inconsistent.

[SirDiazo]

Well, getting no result from calling an action that has been hidden based on the current state of the module is no different from calling a one-way action when it's already at that state (like calling "activate" on something that is already activated). Stock behavior is probably to do nothing on such calls, but not to hide it.

Actually, this is a very good point. If a BaseAction.active = false is set, presumably it makes no sense for that action to be available at that time.

Trying to execute the Activate action on an already activated engine will do nothing just like trying to activate an action with .active = false set will do nothing.

Adding a warning in the log on only the action with .active = false set would be making it that the "action did not execute" error condition would be handled two different ways.

I'm not sure if this is a good or bad thing, that's a judgement call that you (the kOS devs) will have to make on how your error handling works (to be consistent with how other errors are handled).

[Dunbaratu]

To be honest, I've read this over and over and don't understand it. Is it still a problem? I don't even know how to label it in the issues list. Is it a bug? An enhancement?

[SirDiazo]

If this is an issue at all, it is a bug in that a command entered into kOS results in nothing happening. The if, and it's a big if, is the question if this is a bug at all because current behavior matches stock so it in fact may be a non-issue.

Issue: A BaseAction (the code object for a action that can be assigned to an action group) has a boolean value called .active that KSP checks when it is going to execute the action. If BaseAction.active is false, KSP doesn't even try to execute the action. If BaseAction.active is true, KSP tries to execute the action and runs the code inside the method tagged with the KSPAction property in C#.

Setting BaseAction.active to false is the only way found so far (that I know of anyway) to hide an action from the action editor in the VAB so on multi-select parts some mod makers have been using this field to hide actions that don't apply to a part's current mode.

However, setting the BaseAction.active field to false does not stop kOS from listing the action when you query a part for its list of actions so you can command kOS to execute an action whose BaseAction.active is false, but KSP effectively ignores the command and the action does not execute with no error message or any other feedback to the player. The action simply doesn't happen.

As I approach actions from a usage perspective in AGX, being able to execute a BaseAction whose .active field was false seemed wrong so I made the initial post that opened this issue.

In subsequent discussion it was brought up that there is a very similar case where a player can execute the "Activate Engine" action on an engine that is already running and the same case of "nothing happens" occurs.

As presumably it makes no sense for a BaseAction whose .active field is false to be executable (the mod maker set it that way for a reason) it is probably acceptable that commanding kOS to execute said action resulting in nothing happening is okay.

However, I am not a kOS dev so I don't feel like that is my call to make. In short:

1) Is it acceptable that when kOS is commanded to execute a BaseAction whose .active field is false for it to just silently fail with no feedback in the same manner that commanding the "Activate Engine" action on an already running engine silently fails? If yes, there is no issue here and this discussion thread can be closed.

2) Should there be some feedback to the player when a BaseAction whose .active field is false is commanded to execute? If yes then this issue needs to remain open and how the feedback to the player will be displayed needs to be decided. (I presume there is different methods of feedback to the player that could be chosen.) If the answer is no, there should be no player feedback, this issue can be closed.

Hope that clears things up

edit: Ugh, how do you do italics in these comments?

[Dunbaratu]

Okay, well consistency with the rest of kOS's field execution would make it be that if its invisible to the user the kOS script acts like it's not there. But if we did that then it would break execution rather than silently fail and that might be annoying.

I say leave it like it is.