Introducing genericity in Maneuver model

Orekit development
1

Hi,

I would like to introduce genericity in Maneuver force model.

The actual class definition:

public class Maneuver extends AbstractForceModel

Would become:

public class Maneuver<P extends PropulsionModel, M extends ManeuverTriggers> extends AbstractForceModel

This would allow for cleaner code, especially in classes extending the generic Maneuver class.
That way we could get rid of all the ugly casts in ConstantThrustManeuver.
For example:

public Vector3D getThrustVector(final AbsoluteDate date) {
        return ((AbstractConstantThrustPropulsionModel) getPropulsionModel()).getThrustVector(date);
    }

Would become:

public Vector3D getThrustVector(final AbsoluteDate date) {
        return getPropulsionModel().getThrustVector(date);
    }

Would any of you be against it for some reason?

Thanks,
Maxime

2

+1

I appreciate generic type since they reduce code duplication, avoid cast, and improve the code robustness.

3

Hey y’all,

So I’m not a fan of generic types when it’s more than one per class, I find it difficult to read but also to extend. But if you guys think that in that case it removes code duplication, let’s go.
Since @MaximeJ you want to modify those classes, could we take the opportunity to plug in the flow rates the ControlVector3DNormType that I introduced for impulsive maneuvers? I tried to do it myself back then but I was too scared not to be consistent everywhere so I dropped it.

Cheers,
Romain.

4

It’s not mandatory to do it so if some of you don’t like it there’s no rush/real need to do it.
Maybe it can make it hard to use in some cases, for example with the Python Wrapper?

It will also change API and users will have to instantiate maneuvers like this afterwards:
final Maneuver<PropulsionModel, ManeuverTriggers> maneuver = new Maneuver<>(attitudeprovider, maneuverTriggers, propulsionModel);
So it’s more verbose.

For now, it’s mainly the casts in ConstantThrustManeuver that I don’t like but I think we can live with them.

I’ll let the “poll” open to see if other developers/users have an opinion on this.

I must admit I’m not familiar with those. I thought it would it be used in ThrustPropulsionModel.getThrust() only but if it’s related to flow rates there’s probably more to it.
Feel free to open the issue, at least we won’t forget it.

5

Actually use of generic types with the Python wrapper is in my experience the ONLY thing that is easier, because you basically don’t need to write them :rofl:

I must admit I’m not familiar with those. I thought it would it be used in ThrustPropulsionModel.getThrust() only but if it’s related to flow rates there’s probably more to it.
Feel free to open the issue, at least we won’t forget it.

Good idea, issue opened.

Actually, now that you’ve kicked the hornest’s nest of potentially refactoring the maneuvers, I’ve got some view of my own:

  • Can we manage to find a common Interface between impulsive maneuvers and the others? The former are EventDetector and the latter ForceModel, but they both represent trajectory changes spanning from propulsion and somehow added to a propagator. Actually getAttitudeOverride and getIsp (with Double.POSITIVE_INFINITY that should remain an option in my opinion) are good candidates for abstract methods in common.
  • The class Maneuver should definitely be renamed, precisely because it only models continuous burns (as in non instantaneous) and as such (Field)ImpulseManeuver does not inherit from it which is counterintuitive.

Best,
Romain.

6

Impulsive maneuvers can be applied to all propagators, not only numerical ones. This is a great feature as it allows to support maneuvers in very fast search algorithms using for example Brouwer-Lyddane. When more accuracy is needed and maneuvers are small (like in station-keeping optimization), we also have dedicated code in SmallManeuverAnalyticalModel and J2DifferentialEffect.

Having the impulsive maneuvers be specialized EventDetector is weird, I agree, but allowing maneuvers in analytical propagators is really a major feature in Orekit.

7

I’m not criticizing the fact that impulsive maneuvers are event detectors. I think it’s great that they can be fed to analytical propagators. I meant interface in the Java sense. I believe it would make sense to unify like that the ForceModel and EventDetector ones, with a ManeuverProvider or something intended for propagators. I’m not familiar with the small maneuver and J2 differential ones, but they seem to me more like computing effects rather than providing the means to simulate burns in “real time”. So they could remain on their own.

Best,
Romain.

8

It could be written this way indeed but i believe that users would be more likely to write :

final Forcemodel maneuver = new Maneuver<>(attitudeprovider, maneuverTriggers, propulsionModel);

Or at least i hope so for them :sweat_smile:

That i clearly second when i look at ConfigurableLowThrustManeuver which seems like a weird way of using Maneuver.

Makes sense :+1:.

Cheers,
Vincent