-
-
Notifications
You must be signed in to change notification settings - Fork 34
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
Add score computation #144
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some quick comments.
Could you please describe what's working, what needs to be improved, what is not working.
Merci!
<div> | ||
<div>${this.score.circuit}</div> | ||
<div class="large"> | ||
${unsafeHTML(units.formatUnit(this.score.distance / 1000, this.units.distance, undefined, 'unit'))} | ||
</div> | ||
</div> | ||
<div class="collapsible"> | ||
<div>Points = ${this.getMultiplier()}</div> | ||
<div>Points = ${this.getMultiplier()} <br/>${this.league}</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe move the league to it's own div below and hide it on mobile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved into a div.
Is there a standard way to hide a div on mobile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding collapsible class is not sufficient. The collapse button on the bottom is not visible on mobile devices.
I added a another class and the league information will not be visible on devices with less than a minimum height (767px)
@@ -27,6 +28,7 @@ abstract class CZXCBase { | |||
|
|||
export class CzechLocal extends CZXCBase { | |||
name = 'Czech (ČPP local)'; | |||
code: LeagueCode = 'czl'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be a better way to do this - see how it's done for the tracker at https://github.com/vicb/flyxc/blob/2b869f5bc06ce845e5b58f84642ab3e7cd2f044b/libs/common/src/lib/live-track.ts#L37 and up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure it is the same thing... or may be I dont understand. I added some comments in code. Tell me if it's ok or if I am missing something.
abcef4d
to
1f2e3cf
Compare
There are some issues not resolved yet. |
1f2e3cf
to
c10b1b5
Compare
Do you mean that it's correct the first time and become wrong the second time?
How would the issue be triggered with the current code? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple comments.
I'll take a close look in the coming days.
Thanks!
<div> | ||
<div>${this.score.circuit}</div> | ||
<div class="large"> | ||
${unsafeHTML(units.formatUnit(this.score.distance / 1000, this.units.distance, undefined, 'unit'))} | ||
</div> | ||
</div> | ||
<div class="collapsible"> | ||
<div>Points = ${this.getMultiplier()}</div> | ||
<div>Points = ${this.getMultiplier()} <br/>${this.league}</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exactly
I meant that I had this issue with my code from the beginning. After re-reading the code, I think I found the cause : It is a problem of units (meters vs kilometers).
by
The problem is over. But I can't explain for sure why it was working the first time you compute the score and not the second time. I would suspect that a state somewhere is not cleaned / initialized properly. |
There is still sometimes a little difference between score computed the first time and the second time on the same track. An improvement could be to use the igc-xc-score solver in the League subclasses. |
f3b31dc
to
04a2547
Compare
CONTRIBUTING.md
Outdated
- `gcloud components install cloud-datastore-emulator` | ||
|
||
***run:*** | ||
- `gcloud beta emulators datastore start` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://cloud.google.com/sdk/gcloud/reference/beta/emulators/datastore
I think it's good to specify a --data-dir
inside the project then it's easy to delete this folder to start with a fresh DB.
You can also use eval $(gcloud beta emulators datastore env-init --data-dir=DATA-DIR)
for setting the env vars
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. Documentation fixed
- also add a first step for using docker containers for development
- use igc-xc-score - first implementation with quick and dirty UI for scoring - no result visible in UI
- may be not the best solution as other computations are performed by the existing code
…core solver - display planner menu after loading a track - close main menu after loading a track - 'compute score' menu item displayed when there is a current track - show current league in planner menu
- also keep dependencies of igx-xc-score only in improvedScorer.ts
- Add more info (class, type, activity) - Max zoom is now 10 instead of 12
- use igc-xc-score - first implementation with quick and dirty UI for scoring - no result visible in UI
- may be not the best solution as other computations are performed by the existing code
on mobile devices, the planner menu can be too high, and it can't be hidden (hide button is hidden by the speed/altitude/vario pannel)
use values returned by the scorer to compute the mean speed
a9c8ced
to
b12829d
Compare
I think I have replied to your remarks.
|
@flyingtof it would be easier to split this work into multiples CLs. A possible split would be: First CL: Create a There should be an This lib should probably lives in a This lib is only a wrapper around the There should be tests:
Second CL Add a This function should be called mutliple time to optimize a track (as should the Update the test to also test this API. Third CL Replace the current scorer with this new API - note that it is only used to optimize route, not track. Fourth CL Add the ability to score track - as you have implemented in the current CL. I think you should drive each CL to completion/merge before working on the next one. It will be much easier to review this way. The best way could be to leave this CL as is and start new CLs based on master. After that we should add the ability to score live tracks too. What do you think ? |
I agree. We can consider this pr as a prototype and rebuild features step by step |
No description provided.