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

With RT, Inter-vessel comms failing if other vessel is beyond load distance. #2457

Closed
Dunbaratu opened this issue Mar 14, 2019 · 4 comments
Closed
Labels
bug Weird outcome is probably not what the mod programmer expected.

Comments

@Dunbaratu
Copy link
Member

Dunbaratu commented Mar 14, 2019

Our docs about inter-vessel comms imply that delay over lightspeed is a thing with Remote Tech installed. This implies sending to a distant vessel should be possible. It claims that you can do Vessel("foo"):connection:delay to find out this number in seconds, which would be pointless if the other vessel is within loaded range just a few km away.

But it does not work now. Now any call to vessel("foo"):connection:isconnected will always return false when the other vessel is more than a few km away so that it is unloaded, regardless of what sorts of antenna or probe cores or "ModuleSPU"'s it has on it.. As far as I can tell, the root cause of this is that the function call HasFlightComputer(Guid) in the RemoteTechApi is now behaving differently than it used to. I don't normally use this feature and only just discovered this as I was trying to test another change I was working on.

It seems that now, RemoteTech's HasFlightComputer(Guid) is always returning False for any unloaded vessel, when it looks like before it used to work for distant vessels. Because of that change, kOS is claiming no connection exists to the vessel, and the ability to send a message from the current vessel to a distant vessel got broken.

I know that used to work because I know kOS has a lot of code in it to handle the fact that the target vessel might not have its PartModules loaded when sending it a message, and nothing in our source code file that calls Remote Tech's API has changed since 2016.

@Dunbaratu Dunbaratu added the bug Weird outcome is probably not what the mod programmer expected. label Mar 14, 2019
@Dunbaratu
Copy link
Member Author

@tomekpiotrowski - I know you have a lot of Remote Tech knowledge. Do you have any information about the above problem?

@KSP-TaxiService
Copy link

KSP-TaxiService commented Mar 15, 2019

@Dunbaratu Hi, I am the active maintainer of RemoteTech. Can you recall the time point when this was changed?

As far as I can tell, the approach of disposing Flight Computer instance was set up in 2015, near the beginning of RemoteTech repository according to this commit.

The signal processor instance of non-active vessel is to be disposed when (1) vessel switching occurs or (2) unloading of the non-active vessel when passing the physical 2.5km range. As a side-effect, the Flight Computer instance attached to the signal processing instance is disposed. This unloading converts this vessel to "proto-vessel" (without many active instances).

Flight Computer disposed
image

Proto Signal Processor replacing disposed Signal Processor
image

This approach is never changed since then.

Hope this clarifies.

@Dunbaratu
Copy link
Member Author

Dunbaratu commented Mar 15, 2019

@KSP-TaxiService - Thank you for the quick reply.

The kOS message queue integration with Remote Tech was code I didn't originally write, so I'm a bit fuzzy on its history. It's just fallen onto me to try to fix things with it, and I noticed that we were unable to send messages between vessels, and this call was what was denying the ability. I'm assuming that it used to work at one point and wasn't broken all along, but I don't know exactly when it broke.

Taking a look through the RT API, I think we may be able to replace the existing logic with a call to this instead:

 public static double GetSignalDelayToSatellite(Guid a, Guid b)

It looks like that can perform double duty for what we need - it can be both the signal delay query, and the "is it connected?" query, because it returns Double.Infinity when no connection exists.

@KSP-TaxiService
Copy link

For your information, this line below in GetSignalDelayToSatellite returns the output double.PositiveInfinity to the calling system if RT core instance is dead OR either Guid a or Guid b is not RT-enabled (eg debris).

if (satelliteA == null || satelliteB == null) return double.PositiveInfinity;

So it is indeed possible to fix this issue with the method above.

Dunbaratu added a commit to Dunbaratu/KOS-1 that referenced this issue Mar 20, 2019
The RT call HasFlightComputer(guid) will always return false
for any unloaded vessel.  Our protection check, IsAvailable(guid)
was there to stop us from calling API routines on vessels where
doing so would throw nullref, but it also used HasFlightComputer(),
which has this undesirable side-effect of failing on distant
unloaded vessels.

In the meantime since that code was written, Remote Tech has
made changes to their API routins that added protections against
these nullrefs so we can now unconditionally call them without
worry of a crash.  That removed the need for us to call
IsAvailable(guid) all over the place, but we still call
the other version of IsAvailable() (no guid), that is just
checking for whether RT exists period and not checking for
any specific vessel.

(Also, shifted some Infinity checks to the slightly more proper
Double.IsInfinity, just because IEEE suggests using that over
direct value equals).
erendrake added a commit that referenced this issue Mar 21, 2019
Fixes #2457 by removing not-working IsAvailable checks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Weird outcome is probably not what the mod programmer expected.
Projects
None yet
Development

No branches or pull requests

2 participants