Skip to content

Reading and converting gx:coord/gx:Track to Geometry#74

Open
agent10 wants to merge 4 commits intogeorust:mainfrom
ShashlikMap:main
Open

Reading and converting gx:coord/gx:Track to Geometry#74
agent10 wants to merge 4 commits intogeorust:mainfrom
ShashlikMap:main

Conversation

@agent10
Copy link

@agent10 agent10 commented Dec 21, 2025

  • I agree to follow the project's code of conduct.
  • I added an entry to the project's change log file if knowledge of this change could be valuable to users.
    • Usually called CHANGES.md or CHANGELOG.md
    • Prefix changelog entries for breaking changes with "BREAKING: "

This PR adds an initial support of gx:coord/gx:Track tags from KML extension.
This was also partly mentioned in #62.

@pjsier
Copy link
Member

pjsier commented Dec 22, 2025

@agent10 thanks for the PR! For #30 we're hoping to create it first as intermediate Track elements, and then have the ability to convert those to geometries. Here it looks like we're parsing to geometries directly, could you make that change? And then I'm happy to take a closer look

@agent10
Copy link
Author

agent10 commented Dec 22, 2025

@pjsier sure, this is my first time dealing with KML directly but I'll try! Thx!

@agent10
Copy link
Author

agent10 commented Dec 24, 2025

@pjsier Updated. I added an intermediate Track element.
Let me know if there is anything else you want to improve here.

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.

Sorry for the delay! A few more things I didn't mention initially

}
}

impl<T> From<Track<T>> for LineString<T>
Copy link
Member

Choose a reason for hiding this comment

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

Is there a use case for converting to kml::LineString over geo_types::Geometry::LineString? It seems like that's what we'd be looking for and is easier to work with

Copy link
Author

Choose a reason for hiding this comment

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

@pjsier i'm looking into this now, and in my case I have KML files where Track is inside Placemark, similar to the example I made for unit test
But the Placemark uses kml::Geometry internally, so that's why I've initially added a conversion to kml::LineString

Yeah, I'm going to add the general conversion to geo_types::Geometry::LineString but not sure how to deal with the use case above, pls advise..

Copy link
Member

Choose a reason for hiding this comment

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

Got it, thanks for flagging that! I think that also highlights a gap we can clean up here.

Right now we're only parsing child elements in geo_types::Geometry<T>::try_from for Kml::Folder, Kml::KmlDocument, and Kml::Document. Kml::Placemark is only checking geometry like you mentioned here.

I'll open a quick PR in a bit, and then you should just be able to add a branch to the segment in TryFrom that adds this specific element

Copy link
Member

Choose a reason for hiding this comment

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

@agent10 on second though, I looked into this more, and it would be a much more substantial change. To keep this simple and avoid mixing the items up too much I added a comment below, and then in a separate PR I might need to rethink how Placemark is implemented

Some(Geometry::MultiGeometry(self.read_multi_geometry(attrs)?))
}
b"Track" => {
geometry = Some(Geometry::LineString(self.read_track(attrs)?.into()));
Copy link
Member

Choose a reason for hiding this comment

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

I think we should be able to reference .coords here instead of the Track itself since LineString already has a From implementation for Vec<Coord<T>>

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.

3 participants