feature/ATC-134 #209

erikquinn · 2016-12-15T08:52:19Z

Resolves #134.

Prevents collision detection between aircraft when they are outside the airspace. Admittedly not the most elegant solution, but resolves the issue nonetheless.

n8rzz

I think this is a little more elegant than you think. It's clean and readable. 👍 👍

Just a few syntactical changes I'd like to see before we merge this in. But otherwise, good work!

n8rzz · 2016-12-15T13:06:10Z

src/assets/scripts/aircraft/AircraftInstanceModel.js

+        if (airport.perimeter) {    // polygonal airspace boundary
+            inside = point_in_area(this.position, airport.perimeter);
+        } else {    // simple circular airspace boundary
+            inside = this.distance <= airport.ctr_radius && this.altitude <= airport.ctr_ceiling;


why not initialize inside with this condition and then only update inside if airport.perimeter? This has the same effect as the if/else block, but its a little cleaner because now there is no else block.

Fixed. 👍

n8rzz · 2016-12-15T13:06:40Z

src/assets/scripts/aircraft/AircraftInstanceModel.js

-                this.crossBoundary(inside);
-            }
+        const isInsideAirspace = this.isInsideAirspace(window.airportController.airport_get());
+        if (isInsideAirspace !== this.inside_ctr) {


remember to add a line between constants and conditional blocks

Fixed. 👍

n8rzz · 2016-12-15T13:07:28Z

src/assets/scripts/aircraft/AircraftConflict.js

@@ -130,8 +130,9 @@ export default class AircraftConflict {

        // TODO: enumerate the magic numbers.
        // Collide within 160 feet
+        const airport = window.airportController.airport_get();
        if (((this.distance < 0.05) && (this.altitude < 160)) &&


remember to add a line between constants and conditional blocks

Fixed. 👍

n8rzz · 2016-12-15T13:09:55Z

src/assets/scripts/aircraft/AircraftInstanceModel.js

+     * @return {Boolean}
+     * @private
+     */
+    isInsideAirspace(airport) {


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 }) { }

👍 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.

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

n8rzz · 2016-12-15T13:10:29Z

src/assets/scripts/aircraft/AircraftConflict.js

        if (((this.distance < 0.05) && (this.altitude < 160)) &&
-            (this.aircraft[0].isVisible() && this.aircraft[1].isVisible())
+            (this.aircraft[0].isInsideAirspace(airport) && this.aircraft[1].isInsideAirspace(airport))


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

n8rzz · 2016-12-17T00:08:00Z

src/assets/scripts/aircraft/AircraftInstanceModel.js

+            withinAirspaceLateralBoundaries = point_in_area(this.position, airport.perimeter);
+        }
+
+        return withinAirspaceAltitudeRange && withinAirspaceLateralBoundaries;


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.

n8rzz

Code looks good!

But it seems this has created a new issue where planes on the ground, when spawned, crash into each other. This may be localized to the Great Britain airports, but it is definitely a thing.

I'm also getting an error when changing airports:

TypeError: Cannot read property 'poly' of null at area_to_poly

Which seems to come from airport.perimeter getting passed as null. I saw that you had a _has() check, which is good, but _has() doesn't care about value. Only that a key exists.

There is also now a conflicted file. So when you investigate the above, be sure to also pull in develop.

…sOnGround()

n8rzz

Looking better, just a few things to change. Please let me know if you have any questions.

n8rzz · 2016-12-18T23:35:03Z

src/assets/scripts/aircraft/AircraftConflict.js

@@ -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()) {


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

n8rzz · 2016-12-18T23:49:59Z

src/assets/scripts/aircraft/AircraftInstanceModel.js

+    isOnGround() {
+        const error_allowance_ft = 5;
+        const apt = window.airportController.airport_get();
+        const rwy_elev = apt.getRunway(this.rwy_dep || this.rwy_arr).elevation;


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.

@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.

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

n8rzz · 2016-12-18T23:51:22Z

src/assets/scripts/aircraft/AircraftInstanceModel.js

+     */
+    isOnGround() {
+        const error_allowance_ft = 5;
+        const apt = window.airportController.airport_get();


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.

Fixed. 👍

n8rzz · 2016-12-18T23:52:37Z

src/assets/scripts/aircraft/AircraftInstanceModel.js

+        const error_allowance_ft = 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;


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

Fixed. 👍

n8rzz · 2016-12-19T02:00:57Z

src/assets/scripts/aircraft/AircraftInstanceModel.js

+    isOnGround() {
+        const error_allowance_ft = 5;
+        const airport = window.airportController.airport_get();
+        const runway = airport.getRunway(this.rwy_dep || this.rwy_arr);


is this assuming that one of this.rwy_dep || this.rwy_arr will always be null (or undefined)? Why not just create a getter in the AirportModel for something like activeRunway or something?

get activeRunway() { return this.rwy_dep ? this.rwy_dep : this.rwy_arr; }

this is the one instance where I am ok with ternaries, by the way. In a case like this I think it makes sense and is just as readable as the alternative

that getter simply doesn't make sense though... it's not in relation to the airport, it has to do with each individual aircraft. Aircraft.rwy_dep is the runway they are assigned or have already used to take off. Aircraft.rwy_arr is the runway they are assigned or have already used to land. It's not the active arrival runway and active departure runway; it's what each aircraft used, and it's stored for reasons of retrieving the appropriate rwy sections within instrument procedures.

In the current way the game works, each aircraft will only have one of rwy_dep or rwy_arr, because we don't have departures come back and land. So this simpleton solution is one that works at this stage in time. It could be made a getter of the AircraftInstanceModel, but this is a very bizarre, hard to name, only-going-to-be-needed-here operation. We're essentially asking "Hey aircraft, what runway have/did/will you use(d) at the airport, regardless of what you used it for"? The only information we'd be likely to need in other cases is "What runway did/will you depart?" or "What runway did/will you use to land?"

yeah, I understand. and you're right, the getter would be in the AircraftInstanceModel.

Essentially whats happening here, from what I understand, is 'give me the runway you used/or are going to use'. This is where the getter would be helpful because it can do that logic on its own. Thus, anything calling the getter simply says 'give me the runway info' and the getter figures out which one it should send back. and then everything is right in the world.

But, if this isn't used all over the place, it probably isn't really worth the effort.

added #225 so if we ever decide to fix it, we have an issue to refer to :-)

it's not like it's a lot of effort, but this is literally the only place it would be used. Adding it would increase readability, but shouldn't single-use methods be avoided if possible? If not, I could just add another method to the pile-- no biggie.

…TC-134 Conflicts: - CHANGELOG.md

…eature/ATC-134

erikquinn added the feature label Dec 15, 2016

erikquinn added this to the v3.2.0 milestone Dec 15, 2016

erikquinn requested a review from n8rzz December 15, 2016 08:52

n8rzz self-assigned this Dec 15, 2016

n8rzz requested changes Dec 15, 2016

View reviewed changes

erikquinn added 3 commits December 15, 2016 12:56

Create AircraftInstanceModel.isInsideAirspace method

92a142c

Only trigger collision if aircraft are inside the airspace

dd45970

Update changelog

6cea57e

erikquinn force-pushed the feature/ATC-134 branch from 2b3cb4c to 6cea57e Compare December 15, 2016 17:59

erikquinn requested a review from n8rzz December 15, 2016 18:00

n8rzz temporarily deployed to atc-dev-pr-209 December 16, 2016 02:52 Inactive

n8rzz reviewed Dec 17, 2016

View reviewed changes

n8rzz requested changes Dec 17, 2016

View reviewed changes

n8rzz requested a deployment to atc-dev-pr-209 December 17, 2016 00:45 Pending

n8rzz temporarily deployed to atc-dev-pr-209 December 17, 2016 00:46 Inactive

n8rzz temporarily deployed to atc-dev-pr-209 December 17, 2016 02:36 Inactive

erikquinn temporarily deployed to atc-dev-pr-209 December 18, 2016 22:40 Inactive

Merge develop

62a25e9

erikquinn force-pushed the feature/ATC-134 branch from 156daa1 to 62a25e9 Compare December 18, 2016 22:41

erikquinn temporarily deployed to atc-dev-pr-209 December 18, 2016 22:41 Inactive

erikquinn added 3 commits December 18, 2016 18:04

fix improper use of _.has / _.isNil

90bd8a4

Rename and repair method for weight-on-wheels logic from .wow() to .i…

3bde815

…sOnGround()

Replace all references to .wow() with .isOnGround()

2e4cb1b

erikquinn temporarily deployed to atc-dev-pr-209 December 18, 2016 23:16 Inactive

erikquinn requested a review from n8rzz December 18, 2016 23:17

n8rzz requested changes Dec 18, 2016

View reviewed changes

Clean up AircraftInstanceModel.isOnGround() method

d8b7239

erikquinn temporarily deployed to atc-dev-pr-209 December 19, 2016 01:45 Inactive

n8rzz reviewed Dec 19, 2016

View reviewed changes

erikquinn mentioned this pull request Dec 19, 2016

KSFO zlsa/atc#753

Closed

n8rzz added 2 commits December 18, 2016 20:25

Merge branch 'develop' of https://github.com/n8rzz/atc into feature/A…

dad8644

…TC-134 Conflicts: - CHANGELOG.md

Merge branch 'feature/ATC-134' of https://github.com/n8rzz/atc into f…

13a69c8

…eature/ATC-134

n8rzz deployed to atc-dev-pr-209 December 19, 2016 02:25 View deployment

n8rzz approved these changes Dec 19, 2016

View reviewed changes

n8rzz merged commit 02c6844 into develop Dec 19, 2016

n8rzz deleted the feature/ATC-134 branch December 19, 2016 02:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/ATC-134 #209

feature/ATC-134 #209

erikquinn commented Dec 15, 2016 •

edited by Dabea

Loading

n8rzz left a comment

n8rzz Dec 15, 2016

erikquinn Dec 15, 2016

n8rzz Dec 15, 2016

erikquinn Dec 15, 2016

n8rzz Dec 15, 2016

erikquinn Dec 15, 2016

n8rzz Dec 15, 2016

erikquinn Dec 15, 2016

n8rzz Dec 16, 2016

n8rzz Dec 15, 2016

n8rzz Dec 17, 2016

n8rzz left a comment •

edited

Loading

n8rzz left a comment

n8rzz Dec 18, 2016

n8rzz Dec 18, 2016

erikquinn Dec 19, 2016

n8rzz Dec 19, 2016

n8rzz Dec 18, 2016

erikquinn Dec 19, 2016

n8rzz Dec 18, 2016

erikquinn Dec 19, 2016

n8rzz Dec 19, 2016

n8rzz Dec 19, 2016

n8rzz Dec 19, 2016

erikquinn Dec 19, 2016 •

edited

Loading

n8rzz Dec 19, 2016

n8rzz Dec 19, 2016

erikquinn Dec 19, 2016

feature/ATC-134 #209

feature/ATC-134 #209

Conversation

erikquinn commented Dec 15, 2016 • edited by Dabea Loading

n8rzz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n8rzz left a comment • edited Loading

Choose a reason for hiding this comment

n8rzz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikquinn Dec 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikquinn commented Dec 15, 2016 •

edited by Dabea

Loading

n8rzz left a comment •

edited

Loading

erikquinn Dec 19, 2016 •

edited

Loading