#3134 closed enhancement (fixed)
Reduce use of sqrt() calls when determining distances between points/lines/shapes
Reported by: | bretlambert | Owned by: | aboudreault |
---|---|---|---|
Priority: | normal | Milestone: | 5.6 release |
Component: | MapServer C Library | Version: | unspecified |
Severity: | normal | Keywords: | |
Cc: | sdlime, dmorissette |
Description
New distance calculation functions which don't call sqrt(), for use when speed is important (i.e., nested loops). User must call sqrt() on resulting value.
Attachments (2)
Change History (17)
comment:1 by , 15 years ago
Cc: | added |
---|---|
Milestone: | → 5.6 release |
Owner: | changed from | to
follow-up: 3 comment:2 by , 15 years ago
bretlambert, I think you modified the msDistancePointToSegment to msSquareDistancePointToSegment by mistake, am I right ? Or there is a part of the patch that is missing ?
follow-up: 4 comment:3 by , 15 years ago
Replying to aboudreault:
bretlambert, I think you modified the msDistancePointToSegment to msSquareDistancePointToSegment by mistake, am I right ? Or there is a part of the patch that is missing ?
forgot to point the place in the code: mapsearch.c: line 417, 482.
comment:4 by , 15 years ago
Replying to aboudreault:
Replying to aboudreault:
bretlambert, I think you modified the msDistancePointToSegment to msSquareDistancePointToSegment by mistake, am I right ? Or there is a part of the patch that is missing ?
forgot to point the place in the code: mapsearch.c: line 417, 482.
No; I reduced the use of sqrt() within that function from either 2 or 3 calls to only 1 per invocation. I should've made that clear in my description.
Changing l -> l_squared means:
1) you don't have to do l*l 2) you don't call sqrt() when you just want to check for 0 3) defers the actual calculation of the square root to the end of the function where the actual square root is needed to calculate the return value
< 0) tests to only call sqrt() on the value returned from the MS_MIN() macro removes a superfluous sqrt() call. |
follow-up: 6 comment:5 by , 15 years ago
Yes, but there is no msSquareDistancePointToSegment function.
comment:6 by , 15 years ago
Replying to aboudreault:
Yes, but there is no msSquareDistancePointToSegment function.
No, there is not. I didn't implement that in this iteration, as it wasn't immediately apparent to me at 1 am as to how to do so ("Math is hard!" --Barbie).
comment:7 by , 15 years ago
Ok, should I consider your patch as incomplete or, for the moment, modify it by reversing your changes of the line 471 and 482 ?
comment:8 by , 15 years ago
As discussed on IRC, we will take care of that as soon as the final patch is done.
by , 15 years ago
Attachment: | mapserver.sqrt.diff added |
---|
by , 15 years ago
Attachment: | msPolygonLabelPoint.sqrt.diff added |
---|
reduce sqrt() usage in msPolygonLabelPoint() (incorrect version uploaded previously)
follow-up: 10 comment:9 by , 15 years ago
The patch looks good to me. However, I did a simple modification. In your msPolygonLabelPoint.sqrt.diff patch, I've modified:
dist = sqrt(min_dist);
for
min_dist = sqrt(min_dist);
bretlambert, does that change look good to you too?
comment:10 by , 15 years ago
Replying to aboudreault:
The patch looks good to me. However, I did a simple modification. In your
Yes; I had uploaded the incorrect version initially, and uploaded the corrected version (which contains the change you made to the original) when I noticed the issue.
follow-up: 12 comment:11 by , 15 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Emm... your corrected version contains the issue (it uses dist instead of min_dist). Anyway.. I fixed it.
Thanks for the patch!!
Fixed and committed in r9353.
comment:12 by , 15 years ago
Replying to aboudreault:
Emm... your corrected version contains the issue (it uses dist instead of min_dist). Anyway.. I fixed it.
Thanks for the patch!!Fixed and committed in r9353.
When I look at the second diff I placed on trac, it shows the correct version. In any case, sorry for the mixup.
follow-up: 14 comment:13 by , 15 years ago
Just to show you my point: check the line 1116 of http://trac.osgeo.org/mapserver/attachment/ticket/3134/msPolygonLabelPoint.sqrt.diff , that line is incorrect, since you do a dist = instead of min_dist =.
Alan will take care of this one.