Repository navigation
Enable other mods to push custom commands #233
Description
Activity
I'd just like to bump this to second it.
I'm looking to integrate Action Groups Extended and with the existing API in 1.6 I can do the delay on directly controlled actions easy enough, but I have no way of adding to the flight computer.
Now, I am stumbling on how exactly to make this general to all mods. Is there some way to hook in a callback? I'm not sure how else to link the commands back.
I'm installing RemoteTech now to test, depending on how things go I'll update this post.
Okay, after 10 minutes of experimenting, I think if RT adds two things, that will allow any mod to become compatible.
Doing things this way, all RemoteTech has to remember is a string that it spits back out when it is time to execute.
- Open an API call that adds a custom command to the flight computer. SOmething like:
RemoteTechAPI.AddToFlightComptuer(Vessel, GUIString, CommandString)
This would be triggered from the other mod's side. Vessel is the vessel identifier, GUIString is what will appear on the flight computers GUI, CommandString is simply a string that remote tech will spit back out when it is time to execute.
- Callback that other mods can subscribe to.
This is where a mod listens for it's command string to come back to it. On remote tech's side it is simply a callback that every time Remotetech hits a custom command, it spits the CommandString back out this callback. That means all subscribed mods see all CommandString, but it is the mods job to make sure it processes only the commands directed at it.
I'll see if I can produce some actual code, but before I spend too much time on it, does this approach work for you all?
I would recommend to use Guid and Confignodes in stead:
RemoteTechAPI.AddFlightComputerCommand(Guid vesselID, ConfigNode command)
Where the flight computer tries to look for GUIdescription field to provide the GUI description.
Using a confignode, it would be much easier to send more complex commands without having to do your own custom serialization of a string.And to add. The absence of a GUIdescription field within the confignode would mean that the command is simply not shown within the flight computer. If your mod is adding a lot of commands continuously, you wouldn't want to spam the command queue with them.
That is actually a good point, there are really 3 types of commands I can think of that this would need to support.
-
The other Mod does everything, RemoteTech just tells it when to activate. This is the example my previous post had. The external mod would add a command to the flight computer that when it was up, RemoteTech would tell the mod "Activate now" and the other mod does everything. This type of command would show on the Flight Computer GUI.
-
The other Mod does everything, but does not show on the Flight Computer. Pretty much the same, but this would be used for things that should not show on the Flight Computer, but should still be subject to signal delay. This would require adding a "remove command from Flight Comptuer" method to the API as well.
-
Mod enters the command, but RemoteTech executes. For stuff like "face prograde", or "set throttle", this would essentially mimic entering the same command via the flight computer's GUI, just done by a mod through code.
For that, a confignode makes much more sense. If RemoteTech is just bouncing the command back, it does not matter what is in the config node so it is much easier for the other mod to work with, and if it is a command RemoteTech will execute, as long as the confignode is in a format remotetech recognises, that works too.
As for Guid, that works too, any unique identifier for the vessel works and I think I've seen Guid used elsewhere in Remotetech's code.
-
Okay, i'm writing code for this and I think I have a working setup.
However, if a vessel has multiple potential flight computers, what would be the best way to get a reference to the correct computer so I can add the command? As the command is coming from outside, I don't have the internal reference commands currently use when assigned via the on-screen GUI.
(Also, is there a dev thread on the forums for this mod where I could put questions like this?)
Hi diazo,
I think it is not possible to have more than one flight computer on a
vessel, because the flight computer is bound to the vessels guid.We've no dev-thread :/ Should we create one?
Am 20.01.2015 06:37 schrieb "SirDiazo" notifications@github.com:Okay, i'm writing code for this and I think I have a working setup.
However, if a vessel has multiple potential flight computers, what would
be the best way to get a reference to the correct computer so I can add the
command? As the command is coming from outside, I don't have the internal
reference commands currently use when assigned via the on-screen GUI.(Also, is there a dev thread on the forums for this mod where I could put
questions like this?)—
Reply to this email directly or view it on GitHub
#233 (comment)
.Okay, I need to dig into the code some more then, I found the partModule references to the flight comptuer but not where it limits it to one per vessel Guid.
As for a dev thread, don't create one just for me. I just figured if there was an existing one it was an easier place to have this conversation.
Hi, i thought it was the guid. 😊 With this code you can get the current flight computer: https://github.com/Peppie23/RemoteTech/blob/master/src/RemoteTech/UI/TimeWarpDecorator.cs#L163
Ohh, thank you for that, saves me hunting that down.
Now, just to make sure I'm not short-circuiting something, it looks like this here is how I would add a command to the flight computer by passing it a new ICommand class I'm making for this:
However, when I look at the GUI buttons, they are using this () => RTCore... thing that goes straight over my head. (Modding KSP is a hobby and the sum total of my programming experience so I don't know a lot of stuff still.)
Is it okay if I find the correct flight computer and then use that Enqueue method I linked, or is there other stuff on that code behind the GUI button I need to hook into?
Yes the
Enqueueis the method you need.
Here is a more clear example:
https://github.com/RemoteTechnologiesGroup/RemoteTech/blob/master/src/RemoteTech/UI/AttitudeFragment.cs#L296And yes, you should create a new ICommand-Class. Maybe
ApiCommandbut this is up to you ;)That is pretty much everything I need I think.
I wrote my first try at the new ICommand class (I called it ExternalModCommand, you want me to put API in the name somehwere?) and got stuck on how to correctly add it to the flight computer to test it out.
Those two links you gave me look like exactly what I need so I'll see what progress I can make tonight.
no its ok with the name of the command.
When popping custom commands I'd recommend using a static callback<Guid,ConfigNode>, since custom commands would still be handled persistently and serializing callbacks would become very messy. This does require other mods to have a fair bit of persistence themselves, so it might be prudent to add a PersistenceLoaded flag to the configNode when it is saved to persistence. Just so other mods have the opportunity to ignore commands that have passed through persistence.
I would also like callbacks in the API for events such as OnDisconnected, OnConnected, OnUnpowered, OnPowered. Just so that other mods don't have to constantly check connectivity.2 remaining items
Alright, sorry about disapearing like that but Real Life happened.
My opinion on this is do what you need to the remotetech code so that it lines up with your expectations.
While I can make code work, I know I did not fully line up with what remotetech expects, notably where you changed the return from void to bool, I couldn't figure that out when I did this originally so it was kind of a big question mark.
I'll tweak AGX to match what you do, it's a single text string where I change the name of the method I'm looking for on the RT side of things.
As long as RT returns the confignode to me when the time is correct for the actions to execute, this will work for me.
np @SirDiazo take your time. But what i've realized is, that you can't push any kind of "custom data" from your side to the command, like "ActionGroup55" that you know what action should be triggered after popping the command on the flight computer 😰
You saved the completeexternalDataConfigNode into the ApiCommand and i removed this because i don't know how to save this object to the persistent file, without doing a ugly foreach over ech value -.- Maybe we should fix that.erm, I thought I had that working, you simply add the externalData config node using node.AddNode instead of foreaching over each value and using a node.AddValue.
Here's where the ConfigNode is added to the saving node
https://github.com/SirDiazo/RemoteTech/blob/1.7.0/src/RemoteTech/FlightComputer/Commands/ExternalAPICommand.cs#L35-L38and here's the loading of the node
https://github.com/SirDiazo/RemoteTech/blob/1.7.0/src/RemoteTech/FlightComputer/Commands/ExternalAPICommand.cs#L44Doing it this way saves it to the persistent file, it attaches to the confignode KSP passes us on the Save that represents the persistent file and therefore when KSP passes us the confignode from the persistent file when Loading, we can read the confignode as it is.
Note that I did make one assumption, that the Save/Load methods exposed in the AbstractCommand class are linked into the Save/Load methods of KSP for read/writing to the persistent file. As my tests of save/loading the confignode this way worked, I believed that was a safe assumption.
@SirDiazo Save/Load for each ICommand is called within the Save/Load methods of the FlightComputer, which in turn is called within the Save/Load methods of ModuleSPU which - as derived from PartModule - is called whenever the game saves or loads any PartModule.
So any persistent data handled by ICommands is ultimately handled on a PartModule level, not vessel or game level.
Now if this has any importance with what you are doing I don't know.A small note though. From a quick glance, it looks like you aren't invoking your reflected methods with a ConfigNode. This could be a problem for any mod that needs to do anything more complicated than toggling states. Say for example a fictional mod KerbalPartyHat wants to queue up a command, changing the color of Jebs joke pirate hat. They'd need to send and receive a ConfigNode representation of RGB or have a destinct method for each possible color. Passing ConfigNodes back and forth between mods give each mod maker a single easy accesspoint with which to receive data that they can then deserialize into whatever object they need.
I really like the inclusion of an Execute method. It does make passing ConfigNodes back and forth a somewhat more CPU taxing experience; serializing and deserializing for each active ExternalAPICommand for each gametick.
Perhaps the thing to do is to pass a class implementing IConfignode back and forth. It then shouldn't be a big issue for RT to handle persistence by serializing on Save and deserializing on Load. and we are then simply passing an object reference back to the mod.Why the restriction to active vessel by the way? Since all loaded Vessels with a SPU have a working FlightComputer, any loaded vessel should be subject to commands.
As the code is now, wouldn't it be possible to enqueue a command for one vessel, then switch to another, and have the command act for the other vessel?And another note. I don't think you should do your empty string checks by creating an empty string and evaluating equality each time. In stead you should use
!string.IsNullOrEmpty(SomeString)
think of the children... I mean; memory. 😄edit: I see @Peppie23 was busy coding while I typed. Does that qualify as a ninja? 😄
@JDPKSP i think your notes are all addressed to me
☺️ , because i changed SirDiazo's first version to the current.A small note though. From a quick glance, it looks like you aren't invoking your reflected methods with a ConfigNode
I do, with the given ConfigNode from the Api call ExternalAPICommand.cs#L229
Why the restriction to active vessel by the way? Since all loaded Vessels with a SPU have a working FlightComputer, any loaded vessel should be subject to commands.
I restrict this because i'm not sure what this can cause with the "saved" commands for every other flightcomputer. Thats why i limited this function to the current computer.
In stead you should use !string.IsNullOrEmpty(SomeString)
thanks 😅
@Peppie23 Good to hear this is cleared up. I was about to grab the latest source and start digging into this myself to double check my previous results.
@JDPKSP Thanks for the Save/Load clarification. I'm not sure what you mean by "From a quick glance, it looks like you aren't invoking your reflected methods with a ConfigNode." though. The entire point of this is to pass a piece of data to remotetech (an external mod telling remotetech that it wants to activate after a delay) and then pass a piece of data back (remotetech telling the external mod the delay has elapsed, activate now).
As a ConfigNode is KSP's method of saving data of choice, that seemed like a logical object to use.
My confusion then increases as a few lines later you say "Passing ConfigNodes back and forth between mods give each mod maker a single easy accesspoint with which to receive data that they can then deserialize into whatever object they need." which is exactly what this is doing.
Or is supposed to be doing anyway, is there something odd you've noticed that is doing something else?
@Peppie23 Yeah Just after writing that wall of text I noticed your commit.
Anyways, here's another wall o' text 😀
Regarding limiting ExternalAPICommands to only the active vessel.
I still think it would be nice to other modders to allow them to enqueue commands in other vessels than the current active vessel.
Personally I find a lot of use in the FlightComputer of other vessels. It makes for a good docking assistant. And who knows what use other modders could have for this functionality.And now looking through your commit, API.cs#L112.
I know, nitpicky. But formal logic compels me:smile::
this should either be!(externalData.HasValue("GUIDString") && externalData.HasValue("Executor") && externalData.HasValue("ReflectionType"))
or
!externalData.HasValue("GUIDString") || !externalData.HasValue("Executor") || !externalData.HasValue("ReflectionType")
Since the original statement is false if at least one of the values are present.
We'd want the statement to be false iff all the values are present.and regarding ExternalAPICommand.cs#L101. This could have some unintended consequences. Since any ExternalAPICommand that has an Execute function would force the FlightComputer into killrot mode when finished, even if the function had nothing to do with orientation. In cases where the player wants to keep a specific relative orientation, this would be very irritating, and possibly disastrous.
A bit related to this. How about including the option to toggle modes in the flight computer directly?
say for example change from API.cs#L107://exposed method called by other mods, passing a ConfigNode to RemoteTech public static bool QueueCommandToFlightComputer(ConfigNode externalData) { //check we were actually passed a config node if (externalData == null) return false; { RTLog.Verbose("No command was passed", RTLogLevel.API); return false; } //check our min value if (!externalData.HasValue("GUIDString")) { RTLog.Verbose("Passed command did not contain GUIDString", RTLogLevel.API); return false; } var externalVesselId = new Guid(externalData.GetValue("GUIDString")); // you can only push a new external command if the vessel guid is the current active vessel if (FlightGlobals.ActiveVessel.id != externalVesselId) { RTLog.Verbose("Passed guid is not the active Vessels guid", RTLogLevel.API); return false; } //get the VesselSatellite var sat = RTCore.Instance.Satellites[externalVesselId]; if (sat == null) { RTLog.Verbose("Vessel did not have an SPU", RTLogLevel.API); return false; } //get the flightcomputer var computer = sat.FlightComputer; if (computer == null) { RTLog.Verbose("Vessel did not have a flight computer", RTLogLevel.API); return false; } //if no Executor or ReflectionType is passed, check if Serialized forms of AbstractCommands are passed if (!(externalData.HasValue("Executor") && externalData.HasValue("ReflectionType"))) { bool CommandsCreated = false; foreach (ConfigNode n in externalData.nodes) { var command = FlightComputer.Commands.AbstractCommand.LoadCommand(n, computer); if (command != null) { CommandsCreated = true; command.TimeStamp = RTUtil.GameTime; computer.Enqueue(command); } } if (!commandsCreated) { RTLog.Verbose("Executor and or ReflectionType missing and no valid RT commands", RTLogLevel.API); } return CommandsCreated; } try { var extCmd = FlightComputer.Commands.ExternalAPICommand.FromExternal(externalData); computer.Enqueue(extCmd); return true; } catch (Exception ex) { RTLog.Verbose(ex.Message, RTLogLevel.API); } return false; }
For example: AGEext could add FlightComputer control to actionGroups.
@SirDiazo. I was probably just tired and very likely looking too quickly over some deprecated bit o' code. Night shift, that's my excuse 😄Test log with AGX and current version 1.7.0 (as of "Small Fixes #360" being added. (So does not include any of the code in @JDPKSP post above.)
Only two issues that I've noticed, both relatively minor.
Test setup is with AGX, so I only need the command send back once to activate an action group. Therefore my receive method in AGX always returns true regardless of what actually happens so RT does not keep trying to send the command. (See issue 2 below.)
One thing that will need to be clarified is what is the difference between ReflectionPopMethod and ReflectionExecuteMethod. As ReflectionPopMethod is what I got working first, that is what I've used for this test.
Therefore my test confignode had my return reflection string in ReflectionPopMethod only . ReflectionExecuteMethod and ReflectionAbortMethod did not exist as values (and so those two methods were not tested.)
Generally, things worked as expected and I intend to release the 1.7.0 version of the remotetech .dll that I did these tests with in the AGX thread as a place holder so people who want to use AGX with remotetech can do so until the official RT 1.7.0 release.
Issue 1: If the current vessel does not have a flight computer, RT throws a null ref when trying to queue the command. (Noticed on stock Kerbal 2). RT's error handling then catches the null ref and there is no further issue beyond the command being ignored (as it should be ignored as there is no flight computer), but this should probably have better error handling then letting RT throw a null ref.
Issue 2: To handle an external mod that does not return a bool, there needs to be a time out so RT does not keep sending the command forever. From the original implementation of this my receive method back from RT was void and did not return the bool RT expects. RT would send the confignode back to me via reflection every update frame as the method was void and so never sent back the True to tell RT to stop trying.
Oddly enough, 6 tries would work, followed by RT throwing a null ref on SPU.Update(). Then another 6 tries, followed by another null ref, repeat ad nauseum. Unlike the null ref in issue 1, this error is not handled cleanly by RT and is a red text null ref in the log. I did not notice anything else breaking due to this null ref and once I updated my mod to return true when it received the confignode back from RT this issue never happened again.
I suppose it is a question of how much time you want to spend hardening RT against errors in other peoples mods.
@SirDiazo your receive method should actually return false.
Take a look at FlightComputer.PopCommand()
The queue behaviour is such:for each enqueued ICommand where delay(+extra delay) is over, ICommand.Pop(FlightComputer fc) is called. This method is called only once and then the ICommand is removed from the command queue. If the pop method returns true, the ICommand is moved to a prioritized list of active commands.
each tick, each ICommand in the prioritized list of active commands gets its Execute method called. when the Execute method returns true, the command will be deleted.
Note that only one instance of a given priority (represented as an int) can be present within the list of active commands. For example AttitudeCommand has a priority of 0 (meaning it gets the top position of the active commands in the GUI.the Abort method is called if the ICommand being executed should abort. for most commands, this merely forces the Execute method to return true.
For something simple as an event, the command should be removed once it's pop method has been called. for example, see ActionGroupCommand
@Peppie23 Given that we aren't passing the FlightCtrlState to the ReflectionExecuteMethod (and please correct me if i'm wrong) maybe we shouldn't give ExternalAPICommands a priority of 0. Perhaps we could expose the priority variable to allow modders to set the priority (or assume something like 255 if they don't).
And another suggestion: How's about we add a GuiActive flag to ExternalAPICommand. Let's say a modder needed to pass a lot of commands. For example some form of control input that cannot be handled by FlightCtrlState. The GUI queue would instantly fill up with possibly hundreds of commands, one being enqueued each tick@JDPKSP Thank you for the explanation. It looks like I fell through a loop hole where I returned True on the Pop method, but as I had no Execute method to move to the priority list, RT just cleared the command so everything looked fine.
So for AGX, as I only need the call back once when it is time to activate the action group, I should use the Pop method, but return False to RT telling it I'm done with this command and it can be deleted.
Also, your explanation about how things work makes me think of a couple things.
-
Because the Execute method runs every frame, that means the reflection callback runs every frame. Back when I was figuring this reflection stuff out, other programmers warned me that reflection is a very expensive process in terms of CPU load and that calling it every frame is not ideal. Having said that, I can't think of another way to allow a command to stay showing on the flight computer GUI while the command is executing so this is more me tossing this out to see if you guys have any ideas for an alternate method then a request for making a change to the code.
-
External mod passing a lot of commands. I'm not sure how far this should be pursued. This comes back to how much time you want to spend hardening the mod against issues in other mods. As this method allows you to pass a config node, the other mod should collate everything into one confignode and only make a single request of RT to queue. If the other mod does not collate and tries to pass 50 string values as 50 individual requests, I'd argue that is the other mod's problem. Perhaps a throttle limit is needed so that one mod does not bring down the entirety of the RT network, but I have no clue how practical it would be to program such a thing.
-
@SirDiazo One way around the "slow reflection" issue is instead of doing reflection every time you create a delegate the first time then use the cached delegate from then on.
private Dictionary<string, Func<ConfigNode, object>> _delegateCache = new Dictionary<string, Func<ConfigNode, object>>(); /// <summary> /// Calls the rflection method. /// </summary> /// <param name="reflectionMember">Name of the reflection method</param> /// <returns>Object from the invoked reflection method</returns> private object callReflectionMember(string reflectionMember) { Func<ConfigNode, object> function; var key = this.ReflectionType + reflectionMember; if (!_delegateCache.TryGetValue(key, out function)) { Type externalType = this.getReflectionType(this.ReflectionType); MethodInfo methodInfo = externalType.GetMethod(reflectionMember, BindingFlags.InvokeMethod | BindingFlags.Public | BindingFlags.Static); function = (Func<ConfigNode, object>)Delegate.CreateDelegate(typeof(Func<ConfigNode, object>), externalType, methodInfo); _delegateCache[key] = function; } return function(this.prepareDataForExternalMod()); }
In order for other mods to more closely interface with RT, it should be possible for them to push custom commands in the flight computer, to then later have an event fired within the mod when the command is popped.
In pseudocode such a Command, inheriting from AbstractCommand, could include the following fields:
string guiName; //What to display in the visual queue
bool guiActive; //Whether to display the command in the visual queue at all
Object (or possibly ConfgNode) data; //the input data whith wich to call the event native to the mod