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

[WIP] Check if a geometry is a linear ring before assigning trait #421

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

asinghvi17
Copy link
Contributor

@asinghvi17 asinghvi17 commented Apr 5, 2024

Fix #420 ;)

Needs tests.

@rafaqz
Copy link
Collaborator

rafaqz commented Apr 5, 2024

Runtime 😳

@asinghvi17
Copy link
Contributor Author

There is a wkbLinearRing type, but for some reason

julia> typeof(GI.getgeom(water1, 1))
ArchGDAL.IGeometry{ArchGDAL.wkbLineString}

returns a linestring, even though it's a polygon read through ArchGDAL. Maybe that's a bug in the retrieval code?

@rafaqz
Copy link
Collaborator

rafaqz commented Apr 5, 2024

I left some comments about this here:

https://github.com/yeesian/ArchGDAL.jl/blob/master/src/ogr/geometry.jl#L136-L146

Basically GDAL is weird with LinearRing. If you can work out a better way to handle that weirdness go for it.

But imagine what runtime traits will do to compile time and type stabiltity in e.g. GeometryOps apply ;)

(GDAL traits used to be all type unstable and fixing the trait in the type made a lot of things like 20x faster)

@asinghvi17 asinghvi17 changed the title Check if a geometry is a linear ring before assigning trait [WIP] Check if a geometry is a linear ring before assigning trait Apr 5, 2024
@evetion
Copy link
Collaborator

evetion commented May 26, 2024

Yeah I don't think this is fixable. GDAL doesn't treat LinearRing as a stand-alone thing (see https://gdal.org/api/ogrgeometry_cpp.html#_CPPv413OGRLinearRing), and hence there are no LinearRing traits for 2.5D etc (even if you would intercept it in getgeom(polygon)). A way out would be adding isring as a type information, but that's a rather large change. Another way out is adding new LinearRing25D types ourselves, diverging/patching from GDAL calls itself, which is also rather ugly. I fear there's no easy way out.

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.

Linear rings have a GeoInterface.trait of LineStringTrait
3 participants