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

Calculate Bounds when not in original GPX file #71

Merged
merged 2 commits into from
May 6, 2024
Merged

Conversation

miqwit
Copy link
Contributor

@miqwit miqwit commented Mar 19, 2024

Issue #28.

In the original phpGPX the bounds are taken from the GPX file, when it exists. However, when it does not in the file, it's worth computing it.

Getting the bounds can be useful to display a segment on a map, so we can indicate where to zoom the map (the zoom must include the for bounds). The bounds are in fact the max and min latitudes and longitudes : four values, to geographical points.

I respected the philosophy of the library, creating a BoundsCalculator and added tests. Most of the code is related to tests.

* @return array of array Only two points with latitude and longitude that correspond to the
* most northwestern and southeastern points of the track
*/
public static function calculate(array $points): array
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is basically my addition here. The rest of the PR is to use this code (in Segment and Track - but not Route) and testing.

* @param ?float $maxLatitude
* @param ?float $maxLongitude
*/
public function __construct(?float $minLatitude, ?float $minLongitude, ?float $maxLatitude, ?float $maxLongitude)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the constructor a bit more handy for my tests.

@@ -127,5 +128,8 @@ public function recalculateStats()
$this->stats->averagePace = $this->stats->duration / ($this->stats->distance / 1000);
}
}

list($northWest, $southEast) = BoundsCalculator::calculate($this->getPoints());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where I call the calculator, for a given Segment. Not that I reused the logic of a calculator per data to retrieve. However, each calculator is looping on all the Points, so maybe optimization can be done (one loop to get all calculation we desire). For a future refactoring maybe.

src/phpGPX/Models/Track.php Show resolved Hide resolved
@Sibyx
Copy link
Owner

Sibyx commented Mar 22, 2024

Looks pretty good @miqwit! Let me know when the PR will be ready for the review.

@miqwit
Copy link
Contributor Author

miqwit commented Mar 26, 2024

It is now. Checks are passing and unit tests as well. I just don't know if I need to also implement the bounds for the route. I did for the track and the segment. If you judge so, please tell me in the review, and I'll do it the same way, if this way suits you. Also I added missing tests not directly related to my code, but that I needed. This should be helpful for the project, let me know otherwise.

@Sibyx Sibyx marked this pull request as ready for review April 5, 2024 13:01
@Sibyx Sibyx self-assigned this Apr 5, 2024
@Sibyx Sibyx self-requested a review April 5, 2024 13:02
Copy link
Owner

@Sibyx Sibyx left a comment

Choose a reason for hiding this comment

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

Hello! Thank you very much for your contribution. Looks pretty good. Firstly I didn't want to break compatibility with PHP 5.6 but I guess nobody should use it anymore. After your changes we will lift the compatibility support for PHP 7.2+

Sorry for such delay.

@Sibyx Sibyx merged commit 7131274 into Sibyx:master May 6, 2024
1 check passed
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.

None yet

2 participants