JuliaGeometry / GeometryBasics.jl

Basic Geometry Types
MIT License
164 stars 54 forks source link

Make `Point` a subtype of `AbstractGeometry` (instead of `StaticVector`) #133

Closed plut closed 3 years ago

plut commented 3 years ago

The type Point instantiates StaticVector, hence is closed under addition. However, the difference of two (affine) points is not a point, but a vector. This becomes relevant e.g. when computing (signed) volumes, say

det(a::Point{2}, b::Point{2}, c::Point{2}) = det(b-a, c-a)
det(a::Vec{2}, b::Vec{2}) = a[1]*b[2]-a[2]*b[1]

So of course, this is easy enough to fix in my own code, simply by defining

Base.:-(p::Point, q::Point) = Vec(p.data .- q.data)

However, that simple fix is still piracy, and it would make more sense to fix this on the GeometryBasics side. Moreover, since a point is a geometric object, in my opinion a more correct subtyping would be

abstract type AbstractPoint{Dim,T} <: AbstractGeometry{Dim,T} end

This would also make it easier to write methods that apply to AbstractGeometry objects, naturally including points (e.g. affine transformations).

juliohm commented 3 years ago

@plut please check Meshes.jl, all these issues have been addressed there properly. The codebase is quite robust already.

plut commented 3 years ago

I already looked at Meshes.jl, but the relation between these two packages is not clear to me. Is Meshes.jl superseding GeometryBasics.jl? I find it a bit strange that the two packages, while claiming to work together properly, use incompatible types for points.

juliohm commented 3 years ago

Is Meshes.jl superseding GeometryBasics.jl?

I think so? I guess we already have all the functionality in GeometryBasics.jl except for the triangulation of point clouds, which is implemented here using a library written in other language, at least that is what I remember from when I looked at it last year.

I find it a bit strange that the two packages, while claiming to work together properly, use incompatible types for points.

Where did you find this claim? There was never a claim to achieve compatibility with GeometryBasics.jl. In fact, the entire Meshes.jl effort started because GeometryBasics.jl couldn't be easily fixed without breaking other downstream projects. I hope we will be able to slowly migrate packages from using GeometryBasics.jl to using Meshes.jl.

plut commented 3 years ago

OK, thanks for the explanation! I do indeed find Meshes.jl much, much cleaner than GeometryBasics, and am in the process of migrating my own project (expect to hear about it in a few weeks).

plut commented 3 years ago

Oh. I know where I saw that this was compatible. MeshIO.jl claims compatibility with Meshes.jl, but actually uses GeometryBasics.jl. Hence my impression that both were compatible.

juliohm commented 3 years ago

@plut if you are working on CAD-like opererations like extrusions, rotations, etc, please feel free to contribute to Meshes.jl directly. We have plans to add these operations as well.