Skip to content

BGDIINF_SB-2890 : fix coordinate search #498

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

Merged
merged 6 commits into from
Nov 1, 2023

Conversation

pakb
Copy link
Contributor

@pakb pakb commented Oct 30, 2023

@pakb pakb requested a review from ltshb October 30, 2023 13:11
Comment on lines -11 to +16
/^\s*([\d]{1,3})[° ]+([\d]+[.,]?[\d]*)[']?\s*[,/]?\s*([\d]{1,3})[° ]+([\d.,]+)[']?\s*$/i
/^\s*([\d]{1,3})[° ]+([\d]+[.,]?[\d]*)[']?\s*[,/]?\s*([\d]{1,3})[° ]+([\d.,]+)[']?\s*$/i
// 47°38'48'' 7°38'48'' or 47°38'48" 7°38'48"
const REGEX_MERCATOR_WITH_DEGREES_MINUTES =
/^\s*([\d]{1,3})[° ]+([\d]{1,2})[' ]+([\d.]+)['"]{0,2}\s*[,/]?\s*([\d]{1,3})[° ]+([\d.]+)[' ]+([\d.]+)['"]{0,2}\s*$/i
/^\s*([\d]{1,3})[° ]+([\d]{1,2})[' ]+([\d.]+)['′"″]{0,2}\s*[,/]?\s*([\d]{1,3})[° ]+([\d.]+)[' ]+([\d.]+)['′"″]{0,2}\s*$/i
// 47°38'48''N 7°38'48''E or 47°38'48"N 7°38'48"E
const REGEX_MERCATOR_WITH_DEGREES_MINUTES_AND_CARDINAL_POINT =
/^\s*([\d]{1,3})[° ]+\s*([\d]{1,2})[' ]+\s*([\d.]+)['"]*([NSEW]?)\s*[,/]?\s*([\d]{1,3})[° ]+\s*([\d.]+)[' ]+\s*([\d.]+)['"]*([NSEW]?)\s*$/i
/^\s*([\d]{1,3})[° ]+\s*([\d]{1,2})[' ]+\s*([\d.]+)['′"″ ]*([NSEW]?)\s*[,\/]?\s*([\d]{1,3})[° ]+\s*([\d.]+)[' ]+\s*([\d.]+)['′"″ ]*([NSEW]?)\s*$/i
Copy link
Contributor

Choose a reason for hiding this comment

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

Those regex changes has no impact at all, [] is a character set, duplicating a character in a character set has no impact. Same in character set we don't need to escape special characters like / with \. I would revert this changes.
However I propose that I simplify those regex in a separate PR as they are way too complex (too many useless characters) and not 100% correct, here some expamples of simplificaiton:

  • ([\d]{1,3}) => (\d{1,3}) character set here is not needed
  • [']? => '? same here character set is not needed
  • [° ]+ => \s*°\s* here the first regex would match °° while the corrected would only match a single ° character enclosed with 0 or many white spaces
  • [\d] => \d character set not needed
  • ([\d]+[.,]?[\d]*) => (\d+(?:[.,]\d+)?) in the first regext 23. would match while in the second not

Comment on lines 20 to +22
export const reprojectUnknownSrsCoordsToWGS84 = (x, y) => {
if (LV95.getBoundsAs(WEBMERCATOR)?.isInBounds(x, y)) {
// guessing if this is already WGS84, or Mercator or a Swiss projection (if so, we need to reproject it to WGS84)
if (LV95BoundsAsWebMercator.isInBounds(x, y)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In this function why checking backward ? I think it would make our life easier if we only support normal axis order and not try to guess wrong axis order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's because of the issue with CRS:84 and WGS:84, some maps function with a lat/lon pairing, and others with lon/lat, and you can't be sure it will be either one of them you receive.

For LV95 and LV03 backward checking, it is to mimic what was done in mf-geoadmin3 and help automatically correct users' mistake with X/Y

pakb added 5 commits November 1, 2023 09:29
when using non-fixed projection (hard-coded) in the parsing, but dynamically adapt to the one from the args
but base it on the default projection's center, and some re-projection pre-work
So there's no need to remind ourselves to set it up if we want to use our projection constants in tests (and use them with proj4)
@ltshb ltshb mentioned this pull request Nov 1, 2023
17 tasks
@pakb pakb force-pushed the feat-BGDIINF_SB-2890-coordinate-search branch from b0d5926 to eae7e53 Compare November 1, 2023 08:46
@pakb pakb requested a review from ltshb November 1, 2023 09:31
@pakb pakb merged commit 8165f69 into develop-lv95 Nov 1, 2023
@pakb pakb deleted the feat-BGDIINF_SB-2890-coordinate-search branch November 1, 2023 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants