Implement #189 #2097

VJC009 · 2023-07-09T00:26:13Z

Resolves #189

The purpose of this pull request is to add basic transition altitude functionality to the sim. Now every airport will have its own transition altitude (eg. 5000 at egcc, 7000 at engm, etc.). The transition level, for now, is assumed to be the same as the TA, but I will look into expanding that in the future.

I essentially created an array of objects containing icaos and the corresponding TA. A new file in utilities contains the method to detect which airport is selected, looks up the icao and the trans altitude, then passes that the radioUtilities.js and some simple messing about with if statements allow aircraft to correctly readback altitude and flight level below and above the transition altitude. American airports are not included in the array, so when no icao is found, 18000 is returned and that works as always!

I'm no master coder (I've not been doing javascript for long), so you may find my code displeasing to look at, and I may have messed up some naming/folder structure conventions. I apologise in advance!

render · 2023-07-09T00:26:15Z

Your Render PR Server URL is https://openscope-develop-pr-2097.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-cikvt9unqql1c3410jv0.

n8rzz

First, thank you for your pull request! We love contributions and appreciate you taking the time and effort to write some code!

Like you mentioned on Slack, prettier has done it's thing. It appears as though the way you have prettier configured isn't quite in line with what we use on this project. I'd encourage you to revert the radioUtilities changes and start any work in that file over. Check the diff here in the PR, most of the complaints are " vs '`' and a few other spacing issues with ternaries and arrays.

Next, I have some questions on your implementation .

Why did you decide to make transitionAltitudes a singleton?
How might your implementation change if we first looked for a transition Altitude in the airport file itself?
How might your solution change if the entire concept of a Transition Altitude was rolled up under the AirportController?

Couple format things to look out for:

Add an empty line above methods, functions, or control statements
hard coded numbers are called Magic Numbers because, without a name, they mean nothing to other developers. If you need to use a magic number, move it to a constants file and give it a name. ex: const TRANSITION_ALTITUDE_IN_FEET = 18000;. Also, look around the app, we may have this constant already.
When an if statement has a return at the bottom, there is no need for an else statement.

// typical if/else statement
if (condition) {
  return;
} else {
  return;
}

// this is equivalent to if/else and our preference
if (condition) {
  return;
}

return;

VJC009 · 2023-07-11T09:12:47Z

Cheers for the feedback! Sorry it took a while to reply, I've been busy with other stuff. Regards to the formatting, yeah I'll try and set up Prettier to use your eslint config file, shouldn't be too hard.

For your questions:

I wasn't too sure what a singleton was so I had to google it haha, but I decided to make it like that as I thought it was future proofing a little, as in it'd be easier to have a separate utilities file for transition altitudes in case someone wanted to add more utility, such as the function to calculate TA automatically with the current QNH in mind. It was also just an easier way for me to envision and develop my solution.
I'd imagine if we looked at the a/p files themselves, then we'd have to edit the json objects to include a new property like "transAlt" and access it in AiroprtModel.js, which seems to get the airportJson data and then access the new properties in the init method.
I'd imagine in AirportController.js would have a similar method as to the singleton to access the airports dictionary and get the transAlt property. Hopefully that makes sense.

I've also had a look for TRANSITION_ALTITUDE_IN_FEET constant and similar terms but can't find any constants that relate to transition altitude. Perhaps the search function is failing me? xD.

Cheers for your time, I'm learning new things haha, if you agree with my above thinking, please let me know and I'll start working on a solution that is inline with those practices and methods!

This reverts commit 6e117e7.

VJC009 · 2023-07-12T21:40:36Z

I've been unable to solve the no-cycle lint error. I'm not sure I understand it, as I don't believe there is a cycle. AirportController and AirportModel import each other but radioUtils only imports AirportController, so I'm unsure how that suddenly causes the es-lint error to trigger.

I believe it isn't possible to solve this, as for the transAlt to be read by radioUtil, the module needs to be imported.

Solution 1: The dependency cycle error is ignored.

Solution 2: I create a singleton that imports AirportController similar to my original solution and that houses the getTransAlt function. This is then imported into radioUtils and therefore ensures there is no cycle-error.

Let me know which solution is preferable, and I'll do that and complete the rest of the transition altitudes, as currently I've only entered it for EGCC

n8rzz

I really hate leaving PRs sit around this long. I'm sorry it took me so long to come back. I left a few more comments on things to change, but these things are pretty minor. The meat of this PR is looking quite good. Thank you for your time on this so far, we really appreciate it!

n8rzz · 2023-07-23T20:42:50Z

src/assets/scripts/client/airport/AirportController.js

@@ -191,6 +191,13 @@ class AirportController {
        return this.airports[icao.toLowerCase()];
    }

+    getTransAlt() {
+        if (Object.prototype.hasOwnProperty.call(this.current, 'transitionAltitude')) {


we tend to use lodash _has (https://lodash.com/docs/4.17.15#has) for these sorts of checks.

could you flip the logic here to return early when transistionAltitude is not set?

n8rzz · 2023-07-23T20:43:28Z

src/assets/scripts/client/airport/AirportController.js

@@ -205,6 +212,7 @@ class AirportController {
     * @method getInitialArrivalRunwayName
     * @return {string}
     */
+


no empty lines between dockblocks and methods. could you please remove this line?

n8rzz · 2023-07-23T20:44:15Z

src/assets/scripts/client/airport/AirportModel.js

+         * @type {number}
+         * @default null
+        */
+        this.transitionAltitude = null;


would it make sense to assign this, initially, as 18000? Or are you depending on this to be null later on?

n8rzz · 2023-07-23T20:44:24Z

src/assets/scripts/client/airport/AirportModel.js

@@ -376,6 +385,8 @@ export default class AirportModel {
        this.mapCollection = new MapCollection(data.maps, data.defaultMaps, this.positionModel, this.magneticNorth);
        this.defaultWind.speed = data.wind.speed;
        this.defaultWind.angle = degreesToRadians(data.wind.angle);
+        console.log(data);


could you remove this?

n8rzz · 2023-07-23T20:44:56Z

src/assets/scripts/client/utilities/radioUtilities.js

@@ -3,6 +3,7 @@ import _compact from 'lodash/compact';
 import _map from 'lodash/map';
 import { round } from '../math/core';
 import { tau, radians_normalize } from '../math/circle';
+import AirportController from '../airport/AirportController';


Github is complaining a circular dependency here. We are going to need to figure out a way around that, if we can.

n8rzz · 2023-07-23T20:45:53Z

src/assets/scripts/client/utilities/radioUtilities.js

@@ -400,10 +401,25 @@ export const radio_spellOut = (alphanumeric) => {
 export const radio_altitude = (altitude) => {


we might be able to get away from the circular dependency by passing in another arg here for transitionAltitude. Not sure if that's possible, but might help get out of the circular dependency. 🤷‍♂️

n8rzz · 2023-07-23T20:47:09Z

src/assets/scripts/client/utilities/radioUtilities.js

@@ -400,10 +401,25 @@ export const radio_spellOut = (alphanumeric) => {
 export const radio_altitude = (altitude) => {
    const alt_s = altitude.toString();
    const s = [];
+    const transalt = AirportController.getTransAlt();


Please use complete words for variables transalt -> transitionAltitude. I hate that there is a variable named s just above this. If you can give that a meaningful name too, that would be great but not required.

n8rzz · 2023-07-23T20:47:26Z

src/assets/scripts/client/utilities/radioUtilities.js

+                radio_names[alt_s[1]]
+            );
+        }
+        console.log(s);


please remove this.

feature finished + formatting

6e117e7

VJC009 requested a review from n8rzz July 9, 2023 00:26

VJC009 linked an issue Jul 9, 2023 that may be closed by this pull request

Transition Altitudes & Altimetry Units #189

Open

n8rzz reviewed Jul 9, 2023

View reviewed changes

Revert "feature finished + formatting"

bfd641c

This reverts commit 6e117e7.

VJC009 marked this pull request as draft July 11, 2023 13:35

VJC009 changed the title ~~feature finished + formatting~~ Implement #189 Jul 11, 2023

Vivek Chanda added 4 commits July 11, 2023 15:39

Feature working with egcc example

50bd811

pass lint checks

fc4f125

try fix weird lcov issue

cbb69b4

nope revert

c2344af

VJC009 self-assigned this Jul 11, 2023

cleanup

ca6d6a4

VJC009 marked this pull request as ready for review July 12, 2023 21:40

n8rzz requested changes Jul 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement #189 #2097

Implement #189 #2097

VJC009 commented Jul 9, 2023

render bot commented Jul 9, 2023

n8rzz left a comment

VJC009 commented Jul 11, 2023

VJC009 commented Jul 12, 2023 •

edited

n8rzz left a comment

n8rzz Jul 23, 2023

n8rzz Jul 23, 2023

n8rzz Jul 23, 2023

n8rzz Jul 23, 2023

n8rzz Jul 23, 2023

n8rzz Jul 23, 2023

n8rzz Jul 23, 2023

n8rzz Jul 23, 2023

		@@ -400,10 +401,25 @@ export const radio_spellOut = (alphanumeric) => {
		export const radio_altitude = (altitude) => {

Implement #189 #2097

Are you sure you want to change the base?

Implement #189 #2097

Conversation

VJC009 commented Jul 9, 2023

render bot commented Jul 9, 2023

n8rzz left a comment

Choose a reason for hiding this comment

VJC009 commented Jul 11, 2023

VJC009 commented Jul 12, 2023 • edited

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

VJC009 commented Jul 12, 2023 •

edited