-
Notifications
You must be signed in to change notification settings - Fork 32
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
ME: iso19139 spatial extents #944
Conversation
Affected libs:
|
97a71f8
to
33e79e7
Compare
📷 Screenshots are here! |
33e79e7
to
f924559
Compare
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.
Thank you!! most of this looks really good. My main question is about the "bbox" property, which I wish we could avoid adding; using only geometries would make things simpler overall I think.
Also the logic for adding the GML namespace should not be needed, the XML document generation already adds all necessary namespaces at the top in the end :)
${parentPadding}` | ||
} | ||
|
||
function getGmlNamespaceAsNeeded( |
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 don't think you need that; the namespaces are added at the end when producing the XML document, see:
export function createDocument(rootEl: XmlElement): XmlDocument { |
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.
Actually, the way I parse/serialize the GML part with OpenLayers, I don't have the full access to the XmlDocument at that time, as OpenLayers only takes in native node objects, I have to use a string as a intermediate step at some point.
It's there that I'm missing the GML namespace.
If you know a way to have OL understand the Xml elements from the lib we use to parse the record, I'd be really interested.
I agree adding this namespace manually is ugly (and the serialization is ugly too, we change default namespace in the middle of the tree, but apparently it's allowed, so...).
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've tried a few things, I think this can be handled inside the readGeometry
function without any change to xmlToString
, see my other comment
I mostly agree, but as we are dealing with an array of spatial extents, I remove all the elements before writting the new ones, which in this case would mean changing the content of the original record, even when doing only partial edition. Is it OK for the original record to switch from a BBOX spatial extent to a Polygon spatial extent? |
Hmm, I didn't think of that. Maybe there could be a way to figure out that a Polygon is in fact a bbox? something like so: // this was generated with the help of chatgpt
function isBoundingBox(geom: Geometry) {
if (geometry.type !== 'Polygon') {
return false;
}
const coordinates = geometry.coordinates;
// is it a rectangle polygon with no holes?
if (coordinates.length !== 1 || coordinates[0].length !== 5) {
return false;
}
const [first, second, third, fourth, last] = coordinates[0];
// is it a closed ring?
if (first[0] !== last[0] || first[1] !== last[1]) {
return false;
}
// is it an axis-aligned rectangle?
return first[0] === second[0] && first[1] === fourth[1] &&
second[1] === third[1] && third[0] === fourth[0] &&
first[0] !== third[0] && first[1] !== third[1];
} |
So, I'm reading the PR on how to add new spatial extents, and in fact we will only add Polygons that are bboxes. So we end up with a record with several geometries, we first clean those from the XML as this is an array, but then we don't know which one was the original bbox, as all our added geometries are bboxes... It feels like keeping the bbox field in the model is less troublesome :/ |
}) | ||
findChildElement('gmd:polygon', false), | ||
firstChildElement, | ||
map((el) => readGeometry(el) as Polygon | MultiPolygon) |
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.
@jahow Can you explain why this cast to Polygon | MultiPolygon
, when the GML can be of any kind, and OL is parsing it correctly?
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.
sorry this can be removed, it is indeed useless!
986c82d
to
2d17380
Compare
2d17380
to
0521005
Compare
Description
This PR introduces support of the spatial extents in the ISO converters.
Both bbox and the first of multiple geometries are supported.
The description is supported only through the
MD_Identifier
tag.Architectural changes
The api-metadata-converter library now depends on OpenLayers.
Quality Assurance Checklist
breaking change
labelbackport <release branch>
label