Refactor into GPX-based route simulation engine#6
Conversation
Co-authored-by: simonech <61557+simonech@users.noreply.github.com>
|
Can you also update the workflow to run the CLI with the new version? @copilot |
…low update Co-authored-by: simonech <61557+simonech@users.noreply.github.com>
Co-authored-by: simonech <61557+simonech@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the VirtualSpeed application from a TCX-based power-to-velocity converter into a GPX-based route simulation engine. The changes replace the legacy XML parsing logic with a modular architecture consisting of model classes for track points and route segments, along with dedicated services for GPX parsing and route building.
Changes:
- Introduced new model classes (TrackPoint and RouteSegment) using C# records
- Created GpxParser service to extract GPS track points with elevation and timestamp data
- Created RouteBuilder service to segment routes into fixed-length intervals using Haversine distance calculations
- Replaced TCX-based CLI with GPX-based interface (
--gpxflag) - Updated sample data file and CI/CD workflow to use GPX format
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| VirtualSpeed/VirtualSpeed/Model/TrackPoint.cs | Defines immutable record for GPS track points with coordinates, elevation, and optional timestamp |
| VirtualSpeed/VirtualSpeed/Model/RouteSegment.cs | Defines immutable record for route segments with distance, elevation, and gradient data |
| VirtualSpeed/VirtualSpeed/Services/GpxParser.cs | Implements GPX XML parsing with namespace handling and data extraction |
| VirtualSpeed/VirtualSpeed/Services/RouteBuilder.cs | Implements route segmentation using Haversine distance formula with elevation interpolation |
| VirtualSpeed/VirtualSpeed/Program.cs | Complete rewrite of CLI to parse GPX files and output route statistics |
| VirtualSpeed/VirtualSpeed/route.gpx | Sample GPX file with 20 track points for testing |
| VirtualSpeed/VirtualSpeed/VirtualSpeed.csproj | Updated to include route.gpx instead of powerData.tcx |
| .github/workflows/build.yml | Modified to run application with GPX input and validate output format |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| System.Collections.Generic.IEnumerable<Model.TrackPoint> points; | ||
| try | ||
| { | ||
| myXmlDocument.Load(filename); | ||
| points = parser.Parse(filePath); | ||
| } | ||
| catch(Exception e) | ||
| catch (Exception e) | ||
| { | ||
| Console.WriteLine("Error while loading xml file : " + e.Message); | ||
| Console.WriteLine("Error while loading GPX file: " + e.Message); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The lazy enumeration from Parse() means parsing errors will occur after this try-catch block exits, when Build() materializes the IEnumerable. Consider calling .ToList() on the parsed points within the try-catch to ensure parsing errors are properly caught and reported with the "Error while loading GPX file" message.
| double lat = double.Parse(latAttr, CultureInfo.InvariantCulture); | ||
| double lon = double.Parse(lonAttr, CultureInfo.InvariantCulture); | ||
|
|
||
| var eleNode = node.SelectSingleNode("gpx:ele", ns) ?? node.SelectSingleNode("ele"); | ||
| if (eleNode == null) | ||
| continue; | ||
| double elevation = double.Parse(eleNode.InnerText, CultureInfo.InvariantCulture); | ||
|
|
There was a problem hiding this comment.
Missing error handling for malformed latitude and longitude values. If double.Parse fails, the exception will propagate up without a descriptive error message. Consider using double.TryParse and either skipping invalid points with a warning or providing a clearer error message indicating which track point has invalid coordinates.
| double lat = double.Parse(latAttr, CultureInfo.InvariantCulture); | |
| double lon = double.Parse(lonAttr, CultureInfo.InvariantCulture); | |
| var eleNode = node.SelectSingleNode("gpx:ele", ns) ?? node.SelectSingleNode("ele"); | |
| if (eleNode == null) | |
| continue; | |
| double elevation = double.Parse(eleNode.InnerText, CultureInfo.InvariantCulture); | |
| if (!double.TryParse(latAttr, NumberStyles.Float, CultureInfo.InvariantCulture, out double lat)) | |
| throw new FormatException($"Invalid latitude value '{latAttr}' in GPX track point."); | |
| if (!double.TryParse(lonAttr, NumberStyles.Float, CultureInfo.InvariantCulture, out double lon)) | |
| throw new FormatException($"Invalid longitude value '{lonAttr}' in GPX track point."); | |
| var eleNode = node.SelectSingleNode("gpx:ele", ns) ?? node.SelectSingleNode("ele"); | |
| if (eleNode == null) | |
| continue; | |
| if (!double.TryParse(eleNode.InnerText, NumberStyles.Float, CultureInfo.InvariantCulture, out double elevation)) | |
| throw new FormatException($"Invalid elevation value '{eleNode.InnerText}' in GPX track point."); |
| double elevation = double.Parse(eleNode.InnerText, CultureInfo.InvariantCulture); | ||
|
|
There was a problem hiding this comment.
Missing error handling for malformed elevation values. If double.Parse fails, the exception will propagate up without a descriptive error message. Consider using double.TryParse and either skipping invalid points with a warning or providing a clearer error message indicating which track point has invalid elevation data.
| double elevation = double.Parse(eleNode.InnerText, CultureInfo.InvariantCulture); | |
| if (!double.TryParse(eleNode.InnerText, NumberStyles.Float | NumberStyles.AllowThousands, CultureInfo.InvariantCulture, out var elevation)) | |
| throw new FormatException($"Invalid elevation value '{eleNode.InnerText}' in GPX track point."); |
| private readonly double _segmentLengthMeters; | ||
|
|
||
| public RouteBuilder(double segmentLengthMeters = 100.0) | ||
| { |
There was a problem hiding this comment.
Constructor allows non-positive segment lengths, which could cause issues. The gradient calculation at line 52 checks for positive segment length, but negative or zero values would cause logical errors elsewhere (e.g., the while loop at line 44 would never terminate if _segmentLengthMeters is negative). Consider adding validation in the constructor to ensure _segmentLengthMeters is positive.
| { | |
| { | |
| if (segmentLengthMeters <= 0) | |
| throw new ArgumentOutOfRangeException(nameof(segmentLengthMeters), "Segment length must be positive."); |
| var builder = new RouteBuilder(); | ||
|
|
||
| // try to open the xml file and abort if any exception is raised | ||
| System.Collections.Generic.IEnumerable<Model.TrackPoint> points; |
There was a problem hiding this comment.
Unnecessary fully qualified type name. Add 'using System.Collections.Generic;' at the top of the file and change this line to just 'IEnumerable points;' for consistency with the rest of the code.
Model/TrackPoint.csrecord (updated to useDateTimeOffset?)Model/RouteSegment.csrecordServices/GpxParser.cs– skip points missing elevation (instead of defaulting to 0); useDateTimeOffsetparsingServices/RouteBuilder.cs– addedstepDistance <= 0guard for duplicate GPS pointsProgram.csTCX logic with new--gpx <file>CLI entry pointroute.gpxsample file and update csproj (remove TCX reference, add GPX).github/workflows/build.ymlto run CLI with--gpx route.gpxand validate outputOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.