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

Clean up PositionModel and add tests #17

Closed
2 tasks done
n8rzz opened this issue Oct 5, 2016 · 8 comments
Closed
2 tasks done

Clean up PositionModel and add tests #17

n8rzz opened this issue Oct 5, 2016 · 8 comments
Labels
Milestone

Comments

@n8rzz
Copy link
Owner

n8rzz commented Oct 5, 2016

  • pull out .parseCoordinate() and put it in the unitConverters file
  • add tests for that function
@n8rzz n8rzz added the feature label Oct 5, 2016
@n8rzz n8rzz added this to the v3.0.x milestone Oct 5, 2016
@erikquinn
Copy link
Collaborator

Might it be wise to include an altitude property? It's needed for runway elevations, and could be useful for other things such as the fms doing vertical descent planning (fixA-fixB-fixC) when level at FL380, planning to cross fixC at FL210, the fms could simply store Coordinate objects at the specified position and height, and the flightpath would be interpolated between.

Or perhaps you were thinking a 3D position would be better as a separate class?

@n8rzz
Copy link
Owner Author

n8rzz commented Oct 5, 2016

Nope, my thoughts here are much more granular. Although that is a property that could belong to the PositionModel

The current PositionModel contains x and y and latitude and longitude properties that are derived from a coordinates argument on instantiation.

// PositionModel
constructor(coordinates = [], reference, magnetic_north = 0, mode) {
    // TODO: it might make more sense to abstract `coordinates` out to another Model object.
    this.latitude = 0;
    this.longitude = 0;
    this.elevation = 0;
    this.reference_position = reference;
    this.magnetic_north = magnetic_north;
    this.x = 0;
    this.y = 0;
    // TODO: make this a getter;
    this.position = [this.x, this.y];
    this.gps = [0, 0];

    return this.parse(coordinates, mode);
}

This new class would be responsible for only the coordinates and would handle all of the translations and parsing internally. The PositionModel would still handle position. The difference is that now it has access to this new model that has its own methods. we can then create getters and utility methods to give us the data we need when we need it:

get x() {
    return this.coordinates.x;
}

get latitude() {
    return this.coordinates.latitude;
}

this.gps = this.coordinates.translateToGPS();
this.position = this.coordinates.position;

// CoordinatesModel (this becomes this.coordinates.position in the example above)
get position() {
    return [ this.x, this.y ]
}

This means that the PositionModel would simply ask for its coordinates and not have to care about translating them.


Think about it like this:
A navigational Fix has a Position in geographical space made up of Coordinates (and other stuff).

Does this make sense? I'm not sure I'm making sense with all this. I think I'll just put together a quick codepen tonight to better explain what I'm talking about.

@n8rzz
Copy link
Owner Author

n8rzz commented Oct 5, 2016

And re: FMS stuff, yes. Only it would be using information from the FixModel and/or PositionModel.

@erikquinn
Copy link
Collaborator

Oh okay, so it is a separate class. I initially thought you were talking of taking the existing Position and turning it into a class Coordinate, but that's not the case. I see what you're getting at, and it makes sense to me... I think. 😆 So you're imagining something like the below?

let latitude = 39.538247;
let longitude = -77.860575;

let example = new Fix({
    name: "MCRAY",
    flyOver: false,
    rnav: false,
    position: new Position({
        elevation: null,
        gps: new Coordinate({
            y: latitude,
            x: longitude
        }),
        onscreen: new Coordinate({
            x: 10.615,
            y: 808.224
        })
    })
});

(Obviously not providing the values like that, but just as an example of the setup.)

@n8rzz
Copy link
Owner Author

n8rzz commented Oct 6, 2016

Ahh, yeah no, separate class. Your example is pretty much right on except for that the Fix would receive one single object and then internally create a Position which would in turn create the Coordinate instantiations.

There are a few more classes that I think we need that I haven't added issues for yet: StandardRoute, StandardRouteCollection and Waypoint.

The way I envision it, all the fixes will be instantiated independently of any StandardRoute (SID, STAR) when the airport loads they will live by themselves and have just enough information to know where and what they are. The CanvasController will use these models when rendering the view.

Then there will be this thing called a StandardRoute. It is what it sounds like, a SID or STAR or maybe even an IAP. This class will be a collection of Waypoints, which differs slightly from a Fix in that it has navigation-type information: min/max altitudes, speed restrictions, etc. These Waypoints will most likely contain a reference to a specific fix that can be referred to for fix-specific information.

There will then be a StandardRouteCollection which will live in the AirportModel (most likely) and will be responsible for brokering information between the Airport and the StandardRoutes.

To be honest, I'm not sure if all this is necessary but I am sure that there needs to be a better way to organize and reason about Fixes and StandardRoutes. Encapsulating them in class like this makes it much easier to pass them around.

I'm starting to put them down here so that once we're done with v3, I have notes that I can use to continue development. And, of course, provide a way to start conversations like this one. :-)

@n8rzz
Copy link
Owner Author

n8rzz commented Oct 6, 2016

oh, I almost forgot, in the current implementation of PositionModel, I was a little fuzzy on a property. What is gps for? I see lat/long, I assume those are for position calculations in relation to another object. Then there is x and y which I assume are for actually rendering the object. And then there is gps, which depending on conditions can be either lat/long or x/y.

So what is it actually used for? I see its not used too much, but I was a little curious.

@n8rzz
Copy link
Owner Author

n8rzz commented Oct 6, 2016

Something like this: https://codepen.io/n8rzz/pen/JRrrGP?editors=0010#0

@n8rzz n8rzz added refactor and removed feature labels Oct 10, 2016
@n8rzz
Copy link
Owner Author

n8rzz commented Oct 10, 2016

After looking at this some more, and trying a few different implementations, I don't think that this is the right way to go.

The concerns are too coupled between the PositionModel and its properties for this abstraction to make sense. I think a better way to go here would be to:

  • pull out .parseCoordinate() and put it in the unitConverters file
  • add tests for that function

and then let the PositionModel be the position model.

I have run across several instances where this class is being used simple as an instance to get the position values. That seems curious to me. There are other instances, like in the AirplaneModel that are instantiating a PositionModel and then setting their own elevation and magnetic_north properties separately from the PositionModel. That also seems curious to me.

More investigation is needed here...

@n8rzz n8rzz changed the title Create Coordinate class Clean up PositionModel and add tests Oct 21, 2016
@n8rzz n8rzz modified the milestones: v3.1.0, v3.2.0 Nov 16, 2016
@n8rzz n8rzz mentioned this issue Nov 25, 2016
@n8rzz n8rzz closed this as completed Nov 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants