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(wms) : assign axis order param from source #2465

Merged
merged 2 commits into from
Dec 18, 2024

Conversation

airnez
Copy link
Contributor

@airnez airnez commented Nov 18, 2024

Description

WMSSource was'nt taking the axisOrder parameter into account when provided.

Motivation and Context

Consistency: This parameter was advertised in documentation while not being used afterwards.

@airnez airnez requested a review from Desplandis November 18, 2024 17:08
@airnez airnez force-pushed the fix/use-wms-axisorder-param branch from 47dfdd7 to 4f2d77b Compare November 18, 2024 17:09
Copy link
Contributor

@Desplandis Desplandis left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! It was indeed documented and missing...

I have however a few comments with our current implementation in iTowns. Reading geoserver's notes on the WMS specification, it seems that

  • The WMS 1.1 always uses east/north axis ordering.

    Shall we for WMS $<1.3.0$ queries default to wsen for all CRS (instead of just EPSG:4326)?

  • The WMS 1.3.0 specification mandates using the axis ordering as defined in the EPSG database.

    It would be nice again to not just handle the EPSG:4326 projection, and maybe use the projection definition from proj4 to determine the axis ordering +axis. This could be a nice addition to the CRS module.

@airnez
Copy link
Contributor Author

airnez commented Dec 4, 2024

@Desplandis EPSG:4326, EPSG:4269 and other CRS are hardcoded in proj4 as long/lat, not lat/long as described in epsg.org database. This means that we should redefine them by default in itowns in Crs module ? See https://github.com/proj4js/proj4js/blob/1f1494d14a0efcf078c6a9c27f82186ef82cfc53/lib/global.js#L2-L3

Moreover, what should we do if the itowns user does not define the +axis param in proj4 ? I guess that defaulting to wsen would be the option. We should then also implement other orientations like eswn or nesw.

If you are ok with this analysis I am going to push it here.

@airnez airnez force-pushed the fix/use-wms-axisorder-param branch from 4f2d77b to 1b63813 Compare December 5, 2024 15:12
@airnez
Copy link
Contributor Author

airnez commented Dec 5, 2024

@Desplandis added suggested features and rebased on master since Crs module was migrated to Ts

@airnez airnez force-pushed the fix/use-wms-axisorder-param branch from 1b63813 to 8410896 Compare December 5, 2024 15:18
@airnez airnez force-pushed the fix/use-wms-axisorder-param branch from 8410896 to 0929268 Compare December 5, 2024 15:31
@airnez airnez requested a review from Desplandis December 6, 2024 09:07
@Desplandis
Copy link
Contributor

Sounds good to me!

@Desplandis Desplandis merged commit 7d67ec4 into iTowns:master Dec 18, 2024
9 checks passed
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