Skip to content

Expand DMS format and fix sec rounding logic#10066

Merged
tyrasd merged 11 commits intoopenstreetmap:developfrom
laigyu:Adjust-DMS-display-and-add-new-format
Feb 2, 2024
Merged

Expand DMS format and fix sec rounding logic#10066
tyrasd merged 11 commits intoopenstreetmap:developfrom
laigyu:Adjust-DMS-display-and-add-new-format

Conversation

@laigyu
Copy link
Copy Markdown
Contributor

@laigyu laigyu commented Jan 13, 2024

re:#5144

try to let 45°,90°0'0.5" display as 45°,90°0'1"

precision error explain:
if the sec input is 0.5 ,after calculating it will not equal 0.5 but 0.499999999829015,
my soloution is round it by 10e-8 to fix it to 0.5
and do the origin display round 0.5=>1

new format
|35 11 10.1 , 136 49 53.8|| D M SS|
|35 11.168 , 136 49.896||D MM|

zoom center loc always use the user input ,
dont matter how display rounding and show,
always center correct.

re:openstreetmap#5144

Sec 15.56 -> 15.6 , 15.55-> 15.5

Most DMS format parsed by @mapbox/sexagesimal,
add a expandable matcher and two new format to cover some
Copy link
Copy Markdown
Collaborator

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Thanks! This is looking pretty good at a glance. Perhaps some tests would be a good idea, since there are several edge cases when we do the rounding manually.

Comment thread modules/util/units.js
Comment thread modules/util/units.js Outdated
@1ec5 1ec5 linked an issue Jan 13, 2024 that may be closed by this pull request
After calculating ,sec will has precision error,
ex:0.05 after cal -> 0.049999999,
I just use Math.round two time,
plz use 0.04999999 at sec and add 9 to see what happen.
@tyrasd tyrasd added the enhancement An enhancement to make an existing feature even better label Jan 17, 2024
Copy link
Copy Markdown
Member

@tyrasd tyrasd left a comment

Choose a reason for hiding this comment

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

Could you please explain the rationale behind adding the additional decimal precision? IMHO, the resolution of the single minutes is already far sufficient for the intended use case (i.e. navigating to a specified location). 0.1 seconds would only translate to a few additional pixels on the screen… A small negative side effect of showing the decimal is that in some locales (e.g. German) this would introduce the output of a decimal comma in the displayed coordinate which would not be supported as a valid input in the search box (e.g. 12°N, 12°3'4,5"W).

Are the D M SS / D MM formats really found in practice? I guess it would not hurt to support it additionally for convenience (e.g. when copy-pasting from spreadsheets/csv files with dedicated columns for each coordinate part). I'd agree that it would be required to add some unit tests for these routines. And maybe also add support for it with N/S and E/W suffixes in that format (e.g. 45 0 18 N, 90 19 0 W)?

Comment thread modules/util/units.js Outdated
Comment thread modules/ui/feature_list.js Outdated
@laigyu laigyu changed the title Expand DMS format and adjust sec to round to .1 Expand DMS format and fix sec rounding logic Jan 17, 2024
@tyrasd tyrasd force-pushed the Adjust-DMS-display-and-add-new-format branch from 9dfaa6c to 3b0f674 Compare January 31, 2024 14:24
@tyrasd
Copy link
Copy Markdown
Member

tyrasd commented Feb 2, 2024

Thanks for the additional test cases!

@tyrasd tyrasd merged commit 01d650d into openstreetmap:develop Feb 2, 2024
@laigyu laigyu deleted the Adjust-DMS-display-and-add-new-format branch April 5, 2024 05:15
bhousel added a commit to facebook/Rapid that referenced this pull request Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement An enhancement to make an existing feature even better

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DMS rounding errors, display truncation; expand DMS syntax

3 participants