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

Try to fix RemoteTech steering again #1809

Merged
merged 4 commits into from
Sep 9, 2016

Conversation

hvacengi
Copy link
Member

@hvacengi hvacengi commented Sep 6, 2016

Fixes #1806

RT seems to clear the sanctioned pilots list when the vessel unpacks. There isn't a way for us to subscribe to an update to tell us when things get cleared, and there is no way to directly check the list. Since RT checks the list of sanctioned pilots when we attempt to register, and only adds if our handler isn't already registered, we can simply keep calling the method to add a sanctioned pilot. To cut down on the potential performance hit, I limited these calls to once every 25 ticks.

kOSVesselModule.cs

  • Update some logging to simplify references
  • Add counter to refresh RT sactioned pilot once every 25 ticks (1s real time in theory)

kOSVesselModule.cs
* Update some logging to simplify references
* Add counter to refresh RT sactioned pilot once every 25 ticks (1s real time in theory)
@@ -19,6 +19,7 @@ public class kOSVesselModule : VesselModule
private bool initialized = false;
private Vessel parentVessel;
private bool hasRemoteTech = false;
private int counterRemoteTechRefresh = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we begin this at 24 instead of at zero? That way it only waits 1 update before trying to steer for the very first time. Right now, I've had problems where the fact that steering is disabled for 1 second after a staging event during liftoff causes the aero effects to start deflecting the vessel from prograde in ways that feel a bit dangerously close to the flipover point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to make this change and try running with it like that during my stream, which should be starting in an hour.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I haven't observed any issues like that on my end. Something on your system must trigger the race more often than on mine.

We may need to tweak the value a little bit to ensure that we don't randomly find out that one tick is good enough for you, but two ticks might be common as well. If 24 works for you, go for it. If 23 or 22 doesn't give you the same aero effect issues, it might be a little more conservative of a choice.

Copy link
Member

@Dunbaratu Dunbaratu Sep 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I'll try it with 23 then. (I've also moved the hardcoded 25 to a private const int identifier instead just for readability and possible fiddling later if the physics rate ever changes in future releases.)

Dunbaratu and others added 3 commits September 8, 2016 16:25
Changed the starting value of the counter to 23
kOSVesselModule.cs
* Reset the counter to right before triggering when we unhook too, just
to head off another race condition.
@hvacengi
Copy link
Member Author

hvacengi commented Sep 9, 2016

@Dunbaratu I just merged your pull request and copied the same check in the UnHook code, just in case. If you're OK with that we should be ready to merge.

@Dunbaratu
Copy link
Member

great. yeah let's push the button.

@Dunbaratu
Copy link
Member

I'm not really in a position to do it myself right now (I have some temp junk currently checked out into my local working copy). If you want to push the button and merge, go ahead.

@hvacengi
Copy link
Member Author

hvacengi commented Sep 9, 2016

I will do so

@hvacengi hvacengi merged commit 3ccee67 into KSP-KOS:develop Sep 9, 2016
@hvacengi hvacengi deleted the issue-1806-RT_steering_again branch October 12, 2016 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants