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

feature/ATC-134 #209

Merged
merged 10 commits into from
Dec 19, 2016
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@
- Adds gulp-cli and adds [tools readme](tools/README.md) link to gulp issues with Windows [#194](https://github.com/n8rzz/atc/issues/194)
- Changes `routeString` to `routeCode` to better fit what it is and also fixs trancpercy in to the `routeModel` [#188] (https://github.com/n8rzz/atc/issues/188)
-.toUpperCase() is now called on intilization and removed from the getter

- Prevents collision detection for aircraft that are outside of our airspace [#134](https://github.com/n8rzz/atc/issues/134)
- Originally reported under [#736](https://github.com/zlsa/atc/issues/736)



Expand Down
6 changes: 4 additions & 2 deletions src/assets/scripts/aircraft/AircraftConflict.js
Original file line number Diff line number Diff line change
Expand Up @@ -124,14 +124,16 @@ export default class AircraftConflict {
* Check for collision
*/
checkCollision() {
if (this.aircraft[0].wow() || this.aircraft[1].wow()) {
if (this.aircraft[0].isOnGround() || this.aircraft[1].isOnGround()) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I like this name a lot better. doesn't leave anything to the imagination about what its for.

return; // TEMPORARY FIX FOR CRASHES BTWN ARRIVALS AND TAXIIED A/C
}

// TODO: enumerate the magic numbers.
// Collide within 160 feet
const airport = window.airportController.airport_get();

if (((this.distance < 0.05) && (this.altitude < 160)) &&
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remember to add a line between constants and conditional blocks

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. 👍

(this.aircraft[0].isVisible() && this.aircraft[1].isVisible())
(this.aircraft[0].isInsideAirspace(airport) && this.aircraft[1].isInsideAirspace(airport))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for setting airport to a const and sending that in to this method.

) {
this.collided = true;
const isWarning = true;
Expand Down
2 changes: 1 addition & 1 deletion src/assets/scripts/aircraft/AircraftController.js
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ export default class AircraftController {
remove = true;
}

if (aircraft.hit && aircraft.wow()) {
if (aircraft.hit && aircraft.isOnGround()) {
window.uiController.ui_log(`Lost radar contact with ${aircraft.getCallsign()}`);
speech_say([
{ type: 'callsign', content: aircraft },
Expand Down
87 changes: 49 additions & 38 deletions src/assets/scripts/aircraft/AircraftInstanceModel.js
Original file line number Diff line number Diff line change
Expand Up @@ -1382,7 +1382,7 @@ export default class Aircraft {
* @param data
*/
runReroute(data) {
// capitalize everything
// TODO: capitalize everything?
data = data[0].toUpperCase();
let worked = true;
const route = this.fms.formatRoute(data);
Expand Down Expand Up @@ -1473,7 +1473,7 @@ export default class Aircraft {
return ['fail', 'inbound'];
}

if (!this.wow()) {
if (!this.isOnGround()) {
return ['fail', 'already airborne'];
}
if (this.mode === FLIGHT_MODES.APRON) {
Expand Down Expand Up @@ -1704,6 +1704,42 @@ export default class Aircraft {
return this.approachOffset <= 0.048;
}

/**
* Checks if the aircraft is inside the airspace of a specified airport
*
* @for AircraftInstanceModel
* @method isInsideAirspace
* @param {airport} airport the airport whose airspace we are checking
* @return {Boolean}
* @private
*/
isInsideAirspace(airport) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need to do this, but in cases like this where you only need a few properties from a massive object you could destructure them:

isInsideAirspace(airport) { }
// becomes
isInsideAirspace({ perimeter, ctr_radius, ctr_ceiling }) { }

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Good information; wouldn't have thought of that.

Didn't implement here... thought it looked more clean just as airport since we used three of its properties.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

totally cool. I just wanted to call that out in case you ever need it in the future 😉

let withinAirspaceLateralBoundaries = this.distance <= airport.ctr_radius;
const withinAirspaceAltitudeRange = this.altitude <= airport.ctr_ceiling;

if (!_isNil(airport.perimeter)) { // polygonal airspace boundary
withinAirspaceLateralBoundaries = point_in_area(this.position, airport.perimeter);
}

return withinAirspaceAltitudeRange && withinAirspaceLateralBoundaries;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes!!! 👍

one thing to remember and I don't think we've gone over this yet, so I'll let this go as is this time. When a method returns a boolean or a constant is a boolean, it should be prepended with is. so instead of

return isWithinAirspaceAltitudeRange && isWithinAirspaceLateralBoundaries;

this reads a little better and you can immediately tell that those two things are booleans.

}

/**
* Aircraft has "weight-on-wheels" (on the ground)
* @for AircraftInstanceModel
* @method isOnGround
*/
isOnGround() {
const error_allowance_ft = 5;
const apt = window.airportController.airport_get();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you rename this airport? This is sort of a nit-pick, I know. I much prefer these things to be spelled out instead of shortened like this. Its just clearer about what, exactly, that var represents.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. 👍

const rwy_elev = apt.getRunway(this.rwy_dep || this.rwy_arr).elevation;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two things here:

  • apt.getRunway(this.rwy_dep || this.rwy_arr).elevation is confusing. is this something getRunway could do internally?
  • although not bad, I'm not a fan of the someMethod().property style. I know this is littered throughout the app and it works, I just find it confusing. if you need a property from the return of a method destructure it or use the returned object:
// destructure
const { elevation } = airport.getRunway();

// use object
const runway = airport.getRunway();
runway.elevation

In most cases I would lean towards the second option just for readability.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@n8rzz Yeah this whole block looks new but really it's all old; I just renamed it and then moved it so it was aligned alphabetically... but I've fixed the items you've pointed out with it.

And no, the Runway couldn't do it internally... the Aircraft maybe could, but in this particular case, I don't know if it would make much more sense. I could pull the this.rwy_dep || this.rwy_arr part into a variable, but other than that, I don't really know what else to do with it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erikquinn ha, we must have been commenting at the same time. I just added a few more inline about this.

const apt_elev = apt.position.elevation;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just use this inline on line 1738? whats the advantage of storing it in a const?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. 👍

const nearRunwayAltitude = abs(this.altitude - rwy_elev) < error_allowance_ft;
const nearAirportAltitude = abs(this.altitude - apt_elev) < error_allowance_ft;

return nearRunwayAltitude || nearAirportAltitude;
}

/**
* Aircraft is actively following an instrument approach and is elegible for reduced separation
*
Expand All @@ -1725,7 +1761,7 @@ export default class Aircraft {
*/
isStopped() {
// TODO: enumerate the magic number.
return this.wow() && this.speed < 5;
return this.isOnGround() && this.speed < 5;
}

/**
Expand Down Expand Up @@ -1980,7 +2016,7 @@ export default class Aircraft {
this.fms.setCurrent({ start_speed: this.fms.currentWaypoint.speed });
}

if (this.wow()) {
if (this.isOnGround()) {
this.target.altitude = runway.elevation;
this.target.speed = 0;
} else {
Expand Down Expand Up @@ -2136,7 +2172,7 @@ export default class Aircraft {
}

// If stalling, make like a meteorite and fall to the earth!
if (this.speed < this.model.speed.min && !this.wow()) {
if (this.speed < this.model.speed.min && !this.isOnGround()) {
this.target.altitude = Math.min(0, this.target.altitude);
}

Expand Down Expand Up @@ -2235,7 +2271,7 @@ export default class Aircraft {

// TURNING
// this.target.heading = radians_normalize(this.target.heading);
if (!this.wow() && this.heading !== this.target.heading) {
if (!this.isOnGround() && this.heading !== this.target.heading) {
// Perform standard turns 3 deg/s or 25 deg bank, whichever
// requires less bank angle.
// Formula based on http://aviation.stackexchange.com/a/8013
Expand Down Expand Up @@ -2290,7 +2326,7 @@ export default class Aircraft {
}
}

if (this.wow()) {
if (this.isOnGround()) {
this.trend = 0;
}

Expand All @@ -2300,7 +2336,7 @@ export default class Aircraft {
if (this.target.speed < this.speed - 0.01) {
difference = -this.model.rate.decelerate * window.gameController.game_delta() / 2;

if (this.wow()) {
if (this.isOnGround()) {
difference *= 3.5;
}
} else if (this.target.speed > this.speed + 0.01) {
Expand Down Expand Up @@ -2348,7 +2384,7 @@ export default class Aircraft {
const wind = window.airportController.airport_get().wind;
let vector;

if (this.wow()) {
if (this.isOnGround()) {
vector = vscale([sin(angle), cos(angle)], scaleSpeed);
} else {
let crab_angle = 0;
Expand Down Expand Up @@ -2387,22 +2423,10 @@ export default class Aircraft {
this.radial += tau();
}

// polygonal airspace boundary
if (window.airportController.airport_get().perimeter) {
let inside = point_in_area(this.position, window.airportController.airport_get().perimeter);

// TODO: this logic is duplicated below. abstract to new method
if (inside !== this.inside_ctr) {
this.crossBoundary(inside);
}
} else {
// simple circular airspace boundary
let inside = this.distance <= window.airportController.airport_get().ctr_radius &&
this.altitude <= window.airportController.airport_get().ctr_ceiling;
const isInsideAirspace = this.isInsideAirspace(window.airportController.airport_get());

if (inside !== this.inside_ctr) {
this.crossBoundary(inside);
}
if (isInsideAirspace !== this.inside_ctr) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remember to add a line between constants and conditional blocks

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. 👍

this.crossBoundary(isInsideAirspace);
}
}

Expand Down Expand Up @@ -2481,7 +2505,7 @@ export default class Aircraft {
});
}

if (this.terrain_ranges && !this.wow()) {
if (this.terrain_ranges && !this.isOnGround()) {
const terrain = prop.airport.current.terrain;
const prev_level = this.terrain_ranges[this.terrain_level];
const ele = Math.ceil(this.altitude, 1000);
Expand Down Expand Up @@ -2664,17 +2688,4 @@ export default class Aircraft {
removeConflict(other) {
delete this.conflicts[other.getCallsign()];
}

/**
* Aircraft has "weight-on-wheels" (on the ground)
* @for AircraftInstanceModel
* @method wow
*/
wow() {
const error_allowance = 5;
const apt = window.airportController.airport_get();
const rwy_elev = apt.getRunway(this.rwy_dep || this.rwy_arr).elevation;
const apt_elev = apt.position.elevation;
return this.altitude - (rwy_elev || apt_elev) < error_allowance;
}
}
2 changes: 1 addition & 1 deletion src/assets/scripts/aircraft/FlightManagementSystem/Leg.js
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ export default class Leg {

// TODO: refactor/abstract this boolean logic
// Remove the placeholder leg (if present)
if (fms.my_aircraft.wow() &&
if (fms.my_aircraft.isOnGround() &&
fms.legs.length > 0 &&
fms.legs[0].route === airport.icao &&
pairs.length > 0
Expand Down