-
Notifications
You must be signed in to change notification settings - Fork 127
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
use efficient methods to delete trkpts in track filter. #1155
Conversation
#include <cassert> // for assert | ||
#include <cmath> // for nan | ||
#include <cstdio> // for printf | ||
#include <cstdlib> // for abs | ||
#include <cstring> // for strlen, strchr, strcmp | ||
#include <ctime> // for gmtime, strftime | ||
#include <ctime> // for gmtime, strftime, time_t, tm |
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.
Is this still needed? We've been trying to drop ctime.
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.
strftime exposed to user in title option :(
@@ -329,11 +324,20 @@ void TrackFilter::trackfilter_pack() | |||
|
|||
route_head* master = track_list.first(); | |||
|
|||
if (!master->waypoint_list.empty()) { | |||
std::for_each(std::next(master->waypoint_list.cbegin()), master->waypoint_list.cend(), |
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'm excited that some day std::foreach can parallelize things automatically for us. Some day....
avg_dist = cur_dist; | ||
} | ||
|
||
if (cur_dist > 6378.14 && cur_dist > 1.2 * avg_dist) { |
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.
Took me a moment to recognize such a specific magic number. Isn't that a global (hehe) constant for us?
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.
These magic numbers were translations of previous values with units of radians or degrees. The old comment
Latitude spacing is about 27 feet per .00001 degree
was incorrect, it is 1.11m or 3.6ft.
It seems to be arbitrary, but I though 6378 meters was easier to understand that 0.001 radians. I have a hard time imagining this specific value is generally useful.
Would it be more useful to let the user pass this limit? I' not aware of any interest, complaints or use related to this option.
kDistanceLimit seems arbitrary as well, but again 1.11 meters was easier to understand than 0.00001 degrees of latitude or longitude at the equator. There was another magic number, ktoo_close = 0.000005 radians = 31.89 meters that we used to decide if we needed to check points for being the same, this magic has been replaced by kDistanceLimit which is used both to see if we should check, and to decide if the points are the same. 1.11m seems a bit tight to decide we are sitting still.
Perhaps it makes sense, and is generally applicable, to set kDistanceLimit to something like 4.9m https://www.gps.gov/systems/gps/performance/accuracy/
We had a long email 1/10/2019 about this. The test cases referred to are not available. I created one so we have some coverage, but it certainly isn't the real data that inspired this option.
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.
Algorithmically very clever. As waysl, thank you!
Ah, in the transformed forms, I recognize these magic numbers. They were,
indeed, pulled out of a hat.
No, let's not expose those knobs as Yet More Options.
…On Fri, Aug 11, 2023 at 5:07 PM tsteven4 ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In trackfilter.cc
<#1155 (comment)>:
> }
}
}
- if (cur_dist > .001 && cur_dist > 1.2* avg_dist) {
- avg_dist = cur_dist = 0;
+
+ if (avg_dist == 0) {
+ avg_dist = cur_dist;
+ }
+
+ if (cur_dist > 6378.14 && cur_dist > 1.2 * avg_dist) {
These magic numbers were translations of previous values with units of
radians or degrees. The old comment
Latitude spacing is about 27 feet per .00001 degree
was incorrect, it is 1.11m or 3.6ft.
It seems to be arbitrary, but I though 6378 meters was easier to
understand that 0.001 radians. I have a hard time imagining this specific
value is generally useful.
Would it be more useful to let the user pass this limit? I' not aware of
any interest, complaints or use related to this option.
kDistanceLimit seems arbitrary as well, but again 1.11 meters was easier
to understand than 0.00001 degrees of latitude or longitude at the equator.
There was another magic number, ktoo_close = 0.000005 radians = 31.89
meters that we used to decide if we needed to check points for being the
same, this magic has been replaced by kDistanceLimit which is used both to
see if we should check, and to decide if the points are the same. 1.11m
seems a bit tight to decide we are sitting still.
Perhaps it makes sense, and is generally applicable, to set kDistanceLimit
to something like 4.9m
https://www.gps.gov/systems/gps/performance/accuracy/
We had a long email 1/10/2019 about this. The test cases referred to are
not available. I created one so we have some coverage, but it certainly
isn't the real data that inspired this option.
—
Reply to this email directly, view it on GitHub
<#1155 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC3VAD3V5Z7Z3W25KSKZROTXU2UK3ANCNFSM6AAAAAA3NFQVAA>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
* use efficient methods to delete trkpts in track filter. * clean up some code issues flagged by windows tools.
This PR uses track_del_marked_wpts instead of track_del_wpt. track_del_marked_wpts scales with the number of points in the list, while track_del_wpt scales with the number of points in the list times the number of points deleted.
There are some intentional changes of behavior: