Genhis / KRPC.MechJeb

kRPC service for MechJeb2 (Kerbal Space Program)
https://genhis.github.io/KRPC.MechJeb/
GNU General Public License v3.0
15 stars 8 forks source link

Can't make a node in maneuver planner operations #10

Closed Genhis closed 4 years ago

Genhis commented 4 years ago

Describe the bug When I call MakeNode() on any operation, it fails with the following error:

NullReferenceException: Object reference not set to an instance of an object
    KRPC.MechJeb.Maneuver.Operation.MakeNode () (at <80555fe871a148ce8f8a632019cf2177>:0)
    UnityEngine.DebugLogHandler:LogException(Exception, Object)
    ModuleManager.UnityLogHandle.InterceptLogHandler:LogException(Exception, Object)
    UnityEngine.Debug:LogException(Exception)
    KRPC.MechJeb.Logger:Severe(String, Exception)
    KRPC.MechJeb.Maneuver.Operation:MakeNode()
    System.Reflection.MethodBase:Invoke(Object, Object[])
    KRPC.Service.ClassMethodHandler:Invoke(Object[])
    KRPC.Service.Services:ExecuteCall(ProcedureSignature, Object[])
    KRPC.Service.Services:ExecuteCall(ProcedureSignature, ProcedureCall)
    KRPC.Service.ProcedureCallContinuation:Run()
    KRPC.Service.RequestContinuation:Run()
    KRPC.Core:ExecuteContinuation(RequestContinuation)
    KRPC.Core:RPCServerUpdate()
    KRPC.Core:Update()
    KRPC.Addon:FixedUpdate()

Expected behavior The MakeNode() method should return a Node, not throw an exception.

Code snippet to reproduce it (ideally in Python)

sc = conn.space_center
mj = conn.mech_jeb

# sc.target_body = sc.bodies['Mun']

# planner = mj.maneuver_planner
# hohmann = planner.operation_transfer
# hohmann.make_node()

circularize = mj.maneuver_planner.operation_circularize
circularize.time_selector.time_reference = mj.TimeReference.apoapsis
circularize.make_node()

Screenshots N/A

Version details kRPC.MechJeb: 0.5.1

Kerbal Space Program: 1.9.1 kRPC: 0.4.8 MechJeb2: 2.9.2.0

Originally posted by @yjchun in https://github.com/Genhis/KRPC.MechJeb/issues/9#issuecomment-620960391

Genhis commented 4 years ago

MechJeb 2.9.2.0 added support for Multi-Node maneuvers and deleted the MakeNode() method which my code wants to call.

Modifications need to be made to all operations; see https://github.com/MuMech/MechJeb2/commit/44ccd89d9f1116e9c3e0c89bd0dd0f0a4d760cba.

EnJiang commented 4 years ago

This modification causes an error for MechJeb < 2.9.2.0, because now the code tries to call MakeNodes() instead of MakeNode(). Just a suggestion but some backward compatibility would be great.

Genhis commented 4 years ago

@EnJiang Thank you for your suggestion. Sometimes it's difficult to keep things backward-compatible, especially in "pre-release" versions (kRPC.MechJeb API is not yet finalized, though it's coming close to v1.0) and it would be too much work to keep all things backward compatible, if not impossible (see commit https://github.com/Genhis/KRPC.MechJeb/commit/95603ab4fcf114552a1e4c46d3960dd041c3107c).

When I fixed this issue, I thought that backward compatibility is important mainly for client code, so I kept MakeNode() and marked it as deprecated. Why do you think that I should consider backward compatibility for MechJeb versions? Is there something that prevents you from updating MechJeb to the newer version?

EnJiang commented 4 years ago

No. In my case I just downgraded kRPC.MechJeb. It's just it took me some time to google and figure out why I can't make a node and it's a little bit frustrating. If backward compatibility is not economical to do, maybe some clearer indications telling the user maneuver planner won't work perfectly for this version MechJeb would also be good. All of these are pure optional : ) BTW, I've been wanting a programmatic way of MechJeb manipulating for a long time, your work is so great absolutely love it!

Genhis commented 4 years ago

@EnJiang If I may ask, why did you rather downgrade kRPC.MechJeb than upgrade the MechJeb2 itself?

By the way, on the Releases page, there is a compatible kRPC and MechJeb2 version for recent releases. Do you think I should mention it somewhere else as well or write other details?

EnJiang commented 4 years ago

@Genhis I guess it was just a bit handy because I had kRPC.MechJeb repo opened. I think I'll upgrade all of them to the latest version now.

By the way, on the Releases page, there is a compatible kRPC and MechJeb2 version for recent releases. Do you think I should mention it somewhere else as well or write other details?

Thank you! I will follow that.