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

Serialization: extrude, and altitudeMode incorrectly positioned? #24

Closed
ghost opened this issue Apr 2, 2022 · 5 comments
Closed

Serialization: extrude, and altitudeMode incorrectly positioned? #24

ghost opened this issue Apr 2, 2022 · 5 comments

Comments

@ghost
Copy link

ghost commented Apr 2, 2022

When I serialize KmlDocument (with version: kml::KmlVersion::V22) I get e.g.:

<Point>
	<coordinates>13.40984,52.51778</coordinates>
	<altitudeMode>clampToGround</altitudeMode>
	<extrude>0</extrude>
</Point>

which fails to validate against the KML v2.2 schema. It seems altitudeMode and extrude are incorrectly positioned, they should precede coordinates, i.e.:

<Point>
	<extrude>0</extrude>
	<altitudeMode>clampToGround</altitudeMode>
	<coordinates>13.40984,52.51778</coordinates>
</Point>

This validates against the v2.2 schema. (c.f. lines 885-887 in http://schemas.opengis.net/kml/2.2.0/ogckml22.xsd).

Would changing the order here be sufficient? (sorry still new to pull requests etc)

@pjsier
Copy link
Member

pjsier commented Apr 2, 2022

@blipmusic thanks for this issue! I hadn't taken a close look at the sequence restrictions since initially this was mostly focused on reading. Changing the order would fix the issue, feel free to open a PR and I'm happy to answer any questions if you run into any issues

@ghost
Copy link
Author

ghost commented Apr 4, 2022

And thanks for your work on this!

I forked the main branch here and simply changed write order for extrude, and altitudeMode for write_point and extrude, tessellate, altitudeModeGroup for write_geom_props. (made a mess of commits since I forgot return values changed...)

This validates against v2.2 so far.

So doing the same for this repo, should both of these go into the same pull request? Also against which branch, a new or main?

@pjsier
Copy link
Member

pjsier commented Apr 4, 2022

@blipmusic I took a quick look at your branch and it looks great! Usually I like PRs to come from non-main branches because it makes conflicts less likely later on, but I don't think that's a big deal in this case. And no worry about the commits, I usually do squash merges which combines them into a single commit anyway.

If you open a PR against the main branch here I can take a closer look, but from what I can tell it should be good. Thanks!

@ghost
Copy link
Author

ghost commented Apr 4, 2022

I made a PR, but eh I accidentally merged your other suggestion for child elements in Placemark, sorry! I reverted that before making the request. Hope you'll be able to squash the commits into some cleaner! :)

@pjsier
Copy link
Member

pjsier commented Apr 5, 2022

Closed by #25, thanks for pointing this out and for putting together the fix!

@pjsier pjsier closed this as completed Apr 5, 2022
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

No branches or pull requests

1 participant