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

Add kml:Location #7

Merged
merged 2 commits into from
Oct 11, 2021
Merged

Conversation

Nadhum
Copy link
Contributor

@Nadhum Nadhum commented Oct 9, 2021

Changes:
Add Location struct
Add Location reader and writer.

closes #4

Copy link
Member

@pjsier pjsier left a comment

Choose a reason for hiding this comment

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

This is looking great, thanks for the contribution! Had a few small comments but then this should be good to merge

Comment on lines 6 to 7
/// `kml:Location`, [10.10](http://docs.opengeospatial.org/is/12-007r2/12-007r2.html#542) in the KML

Copy link
Member

Choose a reason for hiding this comment

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

Minor thing, but to make sure the doc string works we can remove the extra space

Suggested change
/// `kml:Location`, [10.10](http://docs.opengeospatial.org/is/12-007r2/12-007r2.html#542) in the KML
/// `kml:Location`, [10.10](http://docs.opengeospatial.org/is/12-007r2/12-007r2.html#542) in the KML

pub latitude: T,
pub longitude: T,
pub altitude: T,
pub altitude_mode: AltitudeMode,
Copy link
Member

Choose a reason for hiding this comment

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

It looks like Location doesn't support altitude mode based on the description in the spec unless I'm missing something

Copy link
Contributor Author

@Nadhum Nadhum Oct 10, 2021

Choose a reason for hiding this comment

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

Yes, you're right, when I read the description of "altitude" Altitude of origin measured in meters and interpreted according to kml:altitudeMode. I thought maybe it can be set but now taking another look at it, the description was refereeing to the altitudeMode set on the Parent structure which is Model.

@@ -14,6 +15,7 @@ use crate::types::polygon::Polygon;
#[derive(Debug, Clone, PartialEq)]
pub enum Geometry<T: CoordType = f64> {
Point(Point<T>),
Location(Location<T>),
Copy link
Member

Choose a reason for hiding this comment

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

I am admittedly not very familiar with this project - but is Location really a Geometry?

Copy link
Member

Choose a reason for hiding this comment

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

At least it seems out of sync with the above comment, which says this enum represents http://docs.opengeospatial.org/is/12-007r2/12-007r2.html#432.

Copy link
Member

Choose a reason for hiding this comment

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

Good point! It's in a weird in-between here, since it's in the geometries section http://docs.opengeospatial.org/is/12-007r2/12-007r2.html#431 but not part of the AbstractGeometryGroup. It probably would make more sense to leave this and the other elements in this section that aren't in AbstractGeometryGroup out of this enum

@Nadhum Nadhum requested a review from pjsier October 10, 2021 18:23
Copy link
Member

@pjsier pjsier 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 making those changes! Looks good to me, and I'll leave it open for a bit in case anyone else has thoughts and then merge this in

@Nadhum
Copy link
Contributor Author

Nadhum commented Oct 10, 2021

Thanks, @pjsier would be happy to work on the other similar issues if they're up for grabs.

@pjsier
Copy link
Member

pjsier commented Oct 10, 2021

Sure! The other issues here are open, and I think once those are done that Model, Track, and MultiTrack will be doable

@pjsier pjsier merged commit 1a6b442 into georust:main Oct 11, 2021
@pjsier pjsier mentioned this pull request Oct 17, 2021
@pjsier pjsier mentioned this pull request Dec 11, 2021
This was referenced Sep 27, 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

Successfully merging this pull request may close these issues.

Add support for kml:Location
3 participants