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

Gpx wpt mappoints #452

Merged
merged 10 commits into from
Mar 11, 2022
Merged

Gpx wpt mappoints #452

merged 10 commits into from
Mar 11, 2022

Conversation

bpatrik
Copy link
Owner

@bpatrik bpatrik commented Mar 8, 2022

No description provided.

Store waypoints from GPX files' <wpt> tags into MapPoints, while track points remain in MapPath.
In addition to path from getMapPath(), get wpoints from getMapPoints() and plot them on the marker layer.
Somehow the for loop `wpoints_loop` tends to continue infinitely, forcing stop with `break wpoints_loop` for now.
For testing displaying wpt markers on the gallery map
For testing of wpt markers from GPX file
Copy link
Owner Author

@bpatrik bpatrik left a comment

Choose a reason for hiding this comment

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

@@ -92,3 +109,8 @@ export interface MapPath {
lat: number;
lng: number;
}

export interface MapPoints {
Copy link
Owner Author

Choose a reason for hiding this comment

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

You can reuse the interface above. Maybe refactor it to something like MapCoordinates.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done.

@@ -387,14 +387,32 @@ export class GalleryMapLightboxComponent implements OnChanges {
for (let i = 0; i < this.gpxFiles.length; i++) {
const file = this.gpxFiles[i];
const path = await this.mapService.getMapPath(file);
const wpoints = await this.mapService.getMapPoints(file);
Copy link
Owner Author

Choose a reason for hiding this comment

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

I would add the code from getMapPoints to getMapPath, as both functions do a network call and that is wasteful (probably the browser would serve the second call from cache, but still). Also the two function has some common code. You can rename the function to something more generic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I've made it a single getMapCoordinates[][] instead, which returns the array of arrays coordinates = [path,wpoints] - so there's only one network call to the GPX file, but 2 calls to getElementsByTagName in getMapCoordinates(). I hope that's good enough :)

Copy link
Owner Author

Choose a reason for hiding this comment

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

That is perfect, thank you.

Added argument tagname to getMapCoordinates() to accept 'trkpt' or 'wpt';
replaced interfaces MapPoints and MapPath with MapCoordinates.
Passing 'trkpt' or 'wpt' to getMapCoordinates() to handle both paths and waypoints from GPX files.
Retrieve directly [path,wpoints] from getMapCoordinates[][]
Removed input parameter tagname, hardcoded instead the array tagnames=['trkpt,'wpt'] , and iterated through this array to get elements from the XML file at once and gather them in coordinates[ ] = [ track_path_points[ ], wpoints_points[ ] ].
if (file !== this.gpxFiles[i]) { // check race condition
return;
}
if (path.length === 0) {
continue;
if (path.length !== 0) {
Copy link
Owner Author

Choose a reason for hiding this comment

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

Can you clean up this part a bit? removing unnecessary comments, etc. Otherwise this PR looks good.

Removed unnecessary comments; replaced `break wpoints_loop` with `continue wpoints_loop` which caused infinite loops, let's see if it still happens and if I can fix it...
@bpatrik bpatrik merged commit 9104db6 into bpatrik:master Mar 11, 2022
@bpatrik
Copy link
Owner Author

bpatrik commented Mar 11, 2022

Thank you!

@zigmhount
Copy link
Contributor

Aaaah sorry, I did not realize that the PR was automatically updated with my last commit! It wasn't ready, the infinite loop is back! I just committed a real fix in cd541e3, shall I make a new PR for this?

@bpatrik
Copy link
Owner Author

bpatrik commented Mar 12, 2022

Ah it's okay. I might have refactored the code a bit. (Sorry about that :/ ).
Did a quick test and did not see the undefined issue with the refactored code.

It will take a while until it hits nightly as I messed up the CI pipeline.

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