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

Adds Flight Controls (partially addresses #15) #30

Merged
merged 3 commits into from
Mar 28, 2023

Conversation

dmadisetti
Copy link
Contributor

@dmadisetti dmadisetti commented Mar 27, 2023

Copilot doing a lot of the heavy lifting!

Would be nice to have a testing scaffold (see #29), but for now ensuring the end points work by hand

Notably, x_trim are new but exposed in the API. I thought I would put them in here. Relatively easy to add position changes as well.

Added endpoints:

  • throttle
  • pitch [trim]
  • yaw [trim]
  • roll [trim]
  • wheelsteer [trim]
  • wheelthrottle [trim]
  • lights
  • break
  • gear

Sample script:

import krpc
import matplotlib.pyplot as plt
from time import sleep

print(krpc.__version__)
conn = krpc.connect()
vessel = conn.space_center2.active_vessel
print("eccentricity", vessel.orbit.eccentricity)
print("major", vessel.orbit.semi_major_axis)
print("initial apoapsis", vessel.orbit.apoapsis)
print("initial periapsis", vessel.orbit.periapsis)

apoapsis = [] 
periapsis = [] 
print("GO!")

t = list(range(100))

for s in t:
  apoapsis.append(vessel.orbit.apoapsis)
  periapsis.append(vessel.orbit.periapsis)

  # Auto Burn in cycles
  if s % 10 == 0:
    if (s // 10) % 2 == 0:
      vessel.control.throttle = 1
      vessel.control.pitch = -1
    else:
      vessel.control.throttle = 0
      vessel.control.pitch = 1

  sleep(0.5)

plt.plot(t, apoapsis)
plt.plot(t, periapsis)
plt.savefig("burn")

and result.

Setting pitch is the same as tapping W. Doing pitch_trim is the same as holding W. I don't remember the behavior of the original API

image

@djungelorm
Copy link
Member

Adding the trim APIs lgtm. I'm happy for you to add new APIs, or ignore existing ones if they don't make sense in KSP2. Just want to keep the krpc2 API as close to the krpc1 API as is sensible!

Relatively easy to add position changes as well

Are you referring to translation controls, e.g. for docking? I agree would be good to add them to. Feel free to do in a separate PR if you want.

@djungelorm
Copy link
Member

Setting pitch is the same as tapping W. Doing pitch_trim is the same as holding W. I don't remember the behavior of the original API

In the original API, setting pitch is the same as holding W. There was no way to "tap" W. What does "tap" mean? Is W held for one physics tick?

@dmadisetti
Copy link
Contributor Author

Hmm, I'll have to look into it a bit more and update the doc strings. Just qualitatively, what I noticed was that setting pitch=1 caused the pitch indicator to jump to the set value from 0, then return. I don't know if this was for a single physics tick or some set amount of ticks, but it would be worth reading through some ILCode to find out.

Setting pitch_trim kept the value constant. I'll have to see what happens when the client disconnects

We can also rename this to be consistent with krpc1. pitch_trim -> pitch and pitch -> incremental_pitch or pitch_impulse or something

@dmadisetti
Copy link
Contributor Author

Are you referring to translation controls, e.g. for docking? I agree would be good to add them to. Feel free to do in a separate PR if you want.

I believe so, which would best be put in with the RCS control endpoints, so that would be another PR

@djungelorm
Copy link
Member

djungelorm commented Mar 27, 2023

We can also rename this to be consistent with krpc1. pitch_trim -> pitch and pitch -> incremental_pitch or pitch_impulse or something

Yes please name them like this to preserve consistency. I prefer pitch_impulse. Not sure how useful the impulse version would be though?

@djungelorm
Copy link
Member

Thinking more about inputs like pitch/roll/yaw, the way krpc1 works, is there is a PilotAddon class that manages the actual controls passed to the vessel. One instance of this class exists per vessel. https://github.com/krpc/krpc/blob/main/service/SpaceCenter/src/PilotAddon.cs

I'm wondering if we want something similar here, rather than using InternalVessel.ApplyFlightCtrlState() directly. Then we can maintain the following behavior:

  • If a script sets pitch=1, then disconnects, pitch should revert to zero. (The client disconnecting is akin to the player letting go of the W key). This logic is handled in PilotAddon
  • Allows us to add the Control.InputMode RPC later. (This lets you set controls to either "additive" where the control inputs are added to the vessels current inputs, or "override" which force sets the vessel to have the given control inputs).
  • If I remember correctly, we did it like this in krpc1 so that multiple things can control the vessel. For example, your autopilot script might be controlling pitch/roll/yaw, but the player can use the keyboard to temporarily override it (when the control mode is set to additive)

(Note that inputs like throttle don't need this - they can just be set and forgotten about)

@dmadisetti
Copy link
Contributor Author

Yes please name them like this to preserve consistency. I prefer pitch_impulse. Not sure how useful the impulse version would be though?

Use-case of someone writing a controller using KRPC. Could also be feasible for some PID logic. I think more options don't hurt.

Thinking more about inputs like pitch/roll/yaw, the way krpc1 works, is there is a PilotAddon class that manages the actual controls passed to the vessel. One instance of this class exists per vessel.

Thoughts on getting this in, and then creating an issue / plan for PilotAddon. Is that something that could live in krpc-core and we just use it directly? For this PR, I'll do the rename and clean up the doc strings (but later, I'm supposed to be working lol)

@djungelorm
Copy link
Member

OK sounds good to me. We can merge this and plan PilotAddon later.

krpc-core doesn't depend on Unity/KSP1/KSP2, so can't live in there. PilotAddon is quite dependent on KSP1/KSP2 code so probably needs to live in krpc/krpc2 repo. It's not much code though.

@dmadisetti
Copy link
Contributor Author

I did a bit more experimenting and some aero reading! Trim is typically used to offset constant forces. For instance, we know we always want to provide a pitch value for a front heavy plane to counteract gravity- so we set the trim to a positive pitch so that we don't have to manually hold the pitch position.

This is consistent in KSP. There's the little blue line on the orientation indicators, that moves when I set the trim. A bit more reading, and found you can do this manually with ALT-W or S or whatever. TIL. You can play around with this yourself, but here's me programmatically setting the pitch trim (in orange) and then playing with W-S (then reading the pitch value):

image

Aside: Weird that KSP doesn't do it's own clamping until eval?

So I think not doing the rename is the correct answer. Either control should be done through a different interface than the incremental api (i.e. SetFlightControlState vs ApplyFlightControlState), OR PilotAddon should handle trying to set a fixed orientation value.

@djungelorm
Copy link
Member

djungelorm commented Mar 28, 2023

Thanks for doing the digging on trim vs non-trim. AFAIK this is different to how KSP1 works, and after playing with it a bit more I prefer it! No need for PilotAddon. This is one of those KSP1/2 differences I think that we should just embrace. However, we'll just need to be really clear in the documentation that setting Control.Pitch does not behave the same as kRPC1 (and that Control.PitchTrim behaves more similarly). Don't worry about documentaton changes now though, we can do that later in a separate PR if you like.

I'm happy with the changes for Throttle/Roll/Pitch/Yaw and trim versions. I tested them out and they work fine.

However the other RPCs don't seem to work (Lights, Gear, WheelSteering and WheelThrottle and the trim versions). I tried manually setting them using a python console, with both stock Aeris-K2 plane and stock Inquisitive rover, and they don't do anything in-game. Also reading their values doesn't match what is shown on the in-game hud.

@djungelorm djungelorm added this to the 0.2.0 milestone Mar 28, 2023
@dmadisetti
Copy link
Contributor Author

Did you try brakes as well? I'll strip these from the PR, and will play with it later. No point adding dead code. Gear and Lights did not work for me when I tested them either

@djungelorm
Copy link
Member

Ah yes I tried brakes too, same thing, no effect.

@djungelorm
Copy link
Member

Thanks for updating the docs comments too.

@djungelorm djungelorm merged commit cf324ca into krpc:main Mar 28, 2023
@djungelorm djungelorm mentioned this pull request Mar 28, 2023
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants