yeesian / ArchGDAL.jl

A high level API for GDAL - Geospatial Data Abstraction Library
https://yeesian.github.io/ArchGDAL.jl/stable/
Other
142 stars 26 forks source link

Return extent based on dimensionality. #319

Closed evetion closed 2 years ago

evetion commented 2 years ago

Before we always returned the 3d extent, even on 2d geometries.

rafaqz commented 2 years ago

Lol I have an unfinished PR for this too.

rafaqz commented 2 years ago

It should be possible to make this type stable? It could use is3d, which could also be type stable.

evetion commented 2 years ago

As in having two functions and passing Val(is3d()) right?

rafaqz commented 2 years ago

That won't be type stable cos is3d is currently runtime.

I mean make is3d depend on the type instead of on a GDAL function. Then it will constant prop at compile time without using Val.

evetion commented 2 years ago

Ok, will do, but then we need to wait on #316.

rafaqz commented 2 years ago

We don't need to wait tho it doesn't change the types

evetion commented 2 years ago

Your PR introduces the AbstractGeometry{T} right? So

function GeoInterface.is3d(
        ::GeometryTraits,
        geom::Union{map(T -> AbstractGeometry{T}, ztypes)...},
    )
        return true
    end

would require #316

rafaqz commented 2 years ago

Yeah you're right. My other PR for this just used a Union in the geometry types before I just changed the abstract types.

I might add is3d to the other PR as it falls under type stability improvements. Then this PR can just call is3d.

Edit: I guess even getcoorddim can be a compile time trait. In which case this PR is fine as-is.

evetion commented 2 years ago

To be fair, I would opt for using GI.is3d here. I like to make all GeoInterface methods compile time based and leave the ArchGDAL functions calling GDAL as is.

rafaqz commented 2 years ago

Yeah that sounds like a good idea.

But also it turns out to be not so easy to do this at all because there is no wkbPointZ. So out points are type unstable between 2/3 tuples. We probably need to ecode the dimensionality as an additional parameter in the point type.

evetion commented 2 years ago

There's wkbPoint25D right? It's confusing, I agree.

rafaqz commented 2 years ago

Yeah but no Z when there is ZM? I cant tell how its all mean to work.

evetion commented 2 years ago
wkbPoint =>    X Y 
wkbPoint25D => X Y Z
wkbPointM =>   X Y M
wkbPointZM =>  X Y Z M

And to make it more confusing, for the extended geometries, they didn't use 25D, but Z again (wkbCircularStringZ to wkbTriangleZ)

rafaqz commented 2 years ago

Yeah thats what threw me... there are Z geometries and 25D geometries.

evetion commented 2 years ago

If @rafaqz agrees, this can now be merged as is. is3d can be sped up in the other PR(s).

rafaqz commented 2 years ago

Yep let's do that.

evetion commented 2 years ago

Note that I can't merge because we need at least 1 reviewer approving ;)

yeesian commented 2 years ago

Thank you for this!