jacob-pro / cspice-rs

Rust bindings to CSPICE
GNU Lesser General Public License v3.0
7 stars 3 forks source link

Return Rectangular for position functions rather than Vector3D #4

Closed cmleinz closed 1 year ago

cmleinz commented 2 years ago

Hey Jacob! Currently the return type for spk::position and other similar functions is a Vector3D.

pub fn position<'t, 'r, 'o, T, R, O>(
    target: T,
    et: Et,
    reference_frame: R,
    aberration_correction: AberrationCorrection,
    observing_body: O,
) -> Result<(Vector3D, SpiceDouble), Error>

However the CSPICE documentation says the following:

ptarg is a Cartesian 3-vector ... The three components of `ptarg' represent the x-, y- and z-components of the target's position.

Perhaps we should return Rectangular so as to streamline the conversion to different coordinate systems. Something like:

let (rect, _lt) = spk::position(...).unwrap();
let azel = coordinates::AzEl::from_rect(&rect, false, false);

Optionally we could also break apart the return type of functions which return [SpiceDouble; 6] into their position (Rectangular)

jacob-pro commented 2 years ago

Yep, in the case of spk::position returning Result<(Rectangular, SpiceDouble)> instead makes sense to me.

And for the others e.g. spk::easy_reader do you mean returning Result<(Rectangular, Vector3D, SpiceDouble)>` ?

Possibly even a struct may make more sense, since having a complex tuple can be a bit confusing?

struct State {
   position: Rectangular,
   velocity: Vector3D,
   light_time: SpiceDouble
}

I think also we should also change the definition of Rectangular to be completely independent of Vector3D?

/// Rectangular coordinates
#[derive(Copy, Clone, Debug, Default, PartialEq, From, Into, Deref, DerefMut)]
pub struct Rectangular([SpiceDouble; 3]);

Feel free to raise a PR or otherwise I am happy to look at it in the next few days

cmleinz commented 2 years ago

Great! That sounds like a good prescription. I'll work this and submit a PR sometime this week.

cmleinz commented 2 years ago

Should functions which return position and light time be returned as a tuple Result<(Rectangular, SpiceDouble)> and ones which return state return Result<State>? Or should ones which return state return Result<(State, SpiceDouble)>, where State is:

struct State {
    position: Rectangular,
    velocity: Vector3D
}

and SpiceDouble is the light time

jacob-pro commented 1 year ago

Closed in https://github.com/jacob-pro/cspice-rs/pull/5