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

fix(geometries): Fix stringifyFloat to correctly format number #73

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

saibotk
Copy link
Member

@saibotk saibotk commented Feb 10, 2024

We missed the "." here and thus PHP formatted numbers with the default precision of 6 and prefixed it with whitespaces.

Additionally the trims were adjusted, as we now do not need to trim any whitespaces. Also the trimming for the "." was made a rtrim instead to avoid wrongly stripping .1 to 1, even though this should never be a possible value that sprintf would produce.

See PHP docs: https://www.php.net/manual/en/function.sprintf.php

Thanks @ronnypolloqueri for reporting this. (ref #72)

We missed the "." here and thus PHP formatted numbers with the default precision of 6 and prefixed it with whitespaces.

Additionally the trims were adjusted, as we now do not need to trim any whitespaces. Also the trimming for the "." was made a rtrim instead to avoid wrongly stripping `.1` to `1`, even though this should never be a possible value that sprintf would produce.

See PHP docs: https://www.php.net/manual/en/function.sprintf.php

Thanks @ronnypolloqueri for reporting this.
@saibotk saibotk self-assigned this Feb 10, 2024
Copy link
Member

@ahawlitschek ahawlitschek left a comment

Choose a reason for hiding this comment

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

LGTM

@saibotk
Copy link
Member Author

saibotk commented Feb 10, 2024

@ahawlitschek I'm unsure whether we should fix it like this and get cases where the numbers are "more" precise, but for some is more imprecise, see the tests for example. Or we may fix this in a different way, maybe check out how postgis / geos does it under the hood.

Also i just saw that maybe such imprecision could cause invalid geometries, at least according to ST_AsText, when setting precision there a warning exists about that: https://postgis.net/docs/ST_AsText.html

And PostGis seems to handle the value 50.12345 from the test cases perfectly.
So i'd love to mirror this behaviour.

@saibotk
Copy link
Member Author

saibotk commented Feb 10, 2024

Okay update:

Seems like PostGIS handles them the same way internally:
CleanShot 2024-02-10 at 22 25 46
Or TablePlus is rounding those idk

But it's still weird and we should adjust our WKT generator to try to output the same strings as PostGIS does.
This part might be useful form PostGIS codebase, which takes care of printing the decimal: https://github.com/postgis/postgis/blob/5560fb5cd14162a2d170a464f9e2b13e8998b1f7/deps/ryu/d2s.c#L537

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.

2 participants