Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Projection Extension #485
Projection Extension #485
Changes from 13 commits
6ba4911
8356be7
13c5a75
dbc6eef
3799a23
b387331
7a9a1f9
0266e31
83706db
7d23542
4ac928c
bb9b745
bf32ab4
51a622a
8f18bcd
fe2902d
70ba5b2
7e953c3
6fe837c
eaff656
2a939bd
309a172
7b0452a
a0b93c9
db491b5
ad3652b
a7d48d8
0bc10cf
f40cc2c
e7458cd
848195d
d7e3229
855a29b
c68891d
fc02958
4581cc0
a580b5a
a4a61ec
594c433
a7cc6f1
ca78722
ab1d97d
3f28136
688faf3
c0c66c4
a00d1ed
1a26c36
7417553
fe6e476
a8b51d5
4842a63
1b1087e
2e2bf00
cdef610
f22ac65
9be47e9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does saying
imply a certain class of CRSs? Perhaps "[xmin, ymin, xmax, ymax]" instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the issue with min/max terminology is that for antimeridian-crossing boxes, that xmin is actually greater than xmax, which doesn't have the same implication with west/east
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about a CRS (e.g. EPSG:22182) where positive X is "northing"? I'd worry about mistakes being made if coordinates have to be flipped in the expression of extent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this align with the bbox definition in the item spec? In the spec its defined as [lower_left_corner, upper_right_corner]. From my experience, [xmin,ymin,xmax,ymax] is a modification of the traditional bounding box expression with corner coordinates. It is more intuitive and almost always correct (with the exception of antimeridian crossings as Phil mentions) but is less clear than the corner expression in my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should strictly follow the Item and Collection spec, which are following the GeoJSON and WFS3 spec. I think we should not diverge from that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with following the GeoJSON spec (I don't know about WFS3) is that the CRS is assumed to be 4326, so north is positive Y, east is positive X, etc.
At least that's how I'm interpreting the above.... so feel free to correct me.
No such assumption can be made of arbitrary CRSs. You'd have to project back into 4326 (or similar) to fully comply with the spec, completely negating the purpose of this extension spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @geospatial-jeff meant that the extension fields should at least match in name to the GeoJSON terms. In other words, bbox is used at the GeoJSON feature level to describe the bbox, so if the proj extension provides the bounding box in a different CRS it should follow the same naming convention.
It also avoids confusion with the "extent" field which is used at the collection level and has a quite different meaning (the bbox of the convex hull containing all member Items).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have named this
bbox
instead of extent if that's used in collection. I'm also okay leaving this out if it's too ill-defined outside of 4326.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we please make this a complete example and add it to the examples folder so that it can be used for validation etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if centroid should live next to the top level bbox? Projections seems very specific, and centroid seems more like a quick lookup in a standardized way if you are specifying that is has to be lat long (WGS84 I assume as well?)