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

igc-xc-score integration #14

Closed
wants to merge 12 commits into from
Closed

igc-xc-score integration #14

wants to merge 12 commits into from

Conversation

mmomtchev
Copy link
Contributor

To be considered of alpha quality
Feel free to change colors/icons 😃
Still to be decided: what shall be and should there be any interaction between the scoring flight path and the planned flight path

@vicb vicb self-requested a review June 14, 2020 15:42
const ROUTE_STROKE_COLORS = {
[CircuitType.OPEN_DISTANCE]: '#ff6600',
[CircuitType.OUT_AND_RETURN]: '#ff9933',
[CircuitType.FLAT_TRIANGLE]: '#ffcc00',
[CircuitType.FAI_TRIANGLE]: '#ffff00',
};

const SCORE_STROKE_COLORS = {
Copy link
Owner

Choose a reason for hiding this comment

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

Do you plan to create a route instead of having a different path ?

this.faiSectors.setMap(null);
}
for (let e of [this.flight, this.closingSector, this.faiSectors, this.scoring.closing, this.scoring.path])
if (e)
Copy link
Owner

Choose a reason for hiding this comment

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

nit on style: I like to always have braces.

@@ -332,13 +355,116 @@ export class PathCtrlElement extends connect(store)(LitElement) {
}
}

protected launchScoring(): void {
if (this.tracks && this.currentTrack !== null && this.tracks[this.currentTrack]) {
let fixes: BRecord[] = [];
Copy link
Owner

Choose a reason for hiding this comment

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

nit const

};
if (this.worker !== null)
this.worker.terminate();
this.worker = new Worker('js/xc-score-worker.js');
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need to create a new Worker each time ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not that expensive, this is only when the user clicks the button. terminate allows the user to relaunch a new optimization even when the previous hasn't finished. Interrupting a running optimization and reusing the thread would add lots of complexity for a very marginal performance gain - currently once an optimization is launched, the thread won't yield the CPU until it is finished.

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, makes sense.
If the worked is not re-used then we can release it when it's done (this.worker = null).

Copy link
Owner

@vicb vicb left a comment

Choose a reason for hiding this comment

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

Wahou that is super nice.
I am so delighted that this is getting integrated into FlyXC !
Thank you very much for this contribution.

2 questions/remarks:

  • Do you plan to update the route instead of creating a new path ?
  • I think it would be easier to discover the optimizer if the action was in the left panel ("Optimize") and also as discussed this should probably be started automatically when not on a mobile.

I have some style remarks (braces, trailing commas but hopefully there should be lint rules in place for that and we don't need to spend time for that).

Super boulot, merci beaucoup !

@vicb vicb marked this pull request as draft June 14, 2020 16:17
@mmomtchev
Copy link
Contributor Author

https://coderwall.com/p/i817wa/one-line-function-to-detect-mobile-devices-with-javascript
Here is a practical and quite accurate solution

But then, what shall we do with the official league scoring? Both share the same output window

@vicb
Copy link
Owner

vicb commented Jul 1, 2020

With regards to mobile detection anything is fine to me as long as it stays simple. Pick whichever you prefer.

But then, what shall we do with the official league scoring? Both share the same output window

The idea is that we would use the turn points from your algo and let FlyXc compute the score / distance of that particular route. Does that answer your question or do I miss something ?

@mmomtchev mmomtchev marked this pull request as ready for review July 12, 2020 15:31
@vicb
Copy link
Owner

vicb commented Jul 13, 2020

Hey @mmomtchev thanks for the updates.

IIUC the scoring still does not integrate with the flyxc routes ?

Could you add more details in the PR description of what is implemented in this PR and what is left to do before we can merge ?

Thanks

longitude: this.tracks[this.currentTrack].fixes.lon[i],
pressureAltitude: this.tracks[this.currentTrack].fixes.alt[i],
gpsAltitude: this.tracks[this.currentTrack].fixes.alt[i],
valid: true,
Copy link
Owner

@vicb vicb Sep 2, 2020

Choose a reason for hiding this comment

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

Do we need to specify all the records that have a constant value ? (valid -> enl)

import { IGCFile } from 'igc-parser';

/* The joy of JS multithreading */
function filterFunc(o: any): object {
Copy link
Owner

Choose a reason for hiding this comment

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

Could we drop that an be explicit in what we pass instead of using filterFunc(something) ?

this.worker.terminate();
this.worker = new Worker('js/xc-score-worker.js');
this.worker.onmessage = this.updateScore.bind(this);
this.worker.postMessage({ msg: 'xc-score-start', flight: JSON.stringify(fixes), league: this.league });
Copy link
Owner

@vicb vicb Sep 2, 2020

Choose a reason for hiding this comment

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

It might be nice to use a protobuf (ArrayBuffer) instead of JSON.
It is faster to encode/decode and less memory intensive (does not encoded the field names).
The only thing is that we would need the [en/de]coding logic on the worker side which is ~3kB - probably not a big deal once the worker is cached.

(Note: this is a nice to have, not required for the optimization to be merged).

@vicb vicb force-pushed the master branch 2 times, most recently from 58fa3ad to 8d8a948 Compare November 10, 2020 04:25
@vicb
Copy link
Owner

vicb commented Jun 3, 2024

@flyingtof has been looking at that in #203

@vicb vicb closed this Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants