`intersects(::Box{2,...Unitful.m...}, ::Box{2,...Unitful.mm...})` broken

kylebeggs commented 2 weeks ago

(jl_yN1lJH) pkg> st
Status `/tmp/jl_yN1lJH/Project.toml`
  [eacbb407] Meshes v0.46.0
  [1986cc42] Unitful v1.20.0

julia> using Meshes; using Unitful: mm

julia> b1 = Box(Point(0,0), Point(1,1))
Box
├─ min: Point(x: 0.0 m, y: 0.0 m)
└─ max: Point(x: 1.0 m, y: 1.0 m)

julia> b2 = b1 |> LengthUnit(mm)
Box
├─ min: Point(x: 0.0 mm, y: 0.0 mm)
└─ max: Point(x: 1000.0 mm, y: 1000.0 mm)

julia> intersects(b1, b2)
ERROR: MethodError: no method matching Box(::Point{2, CoordRefSystems.Cartesian{…}}, ::Point{2, CoordRefSystems.Cartesian{…}})

Closest candidates are:
  Box(::Point{Dim, C}, ::Point{Dim, C}) where {Dim, C<:CoordRefSystems.CRS}
   @ Meshes ~/.julia/packages/Meshes/EqjBS/src/primitives/box.jl:28

Stacktrace:
 [1] intersection(f::Function, box₁::Box{2, CoordRefSystems.Cartesian{…}}, box₂::Box{2, CoordRefSystems.Cartesian{…}})
   @ Meshes ~/.julia/packages/Meshes/EqjBS/src/intersections/boxes.jl:27
 [2] intersection
   @ ~/.julia/packages/Meshes/EqjBS/src/intersections.jl:101 [inlined]
 [3] intersect
   @ ~/.julia/packages/Meshes/EqjBS/src/intersections.jl:73 [inlined]
 [4] intersects(b₁::Box{2, CoordRefSystems.Cartesian{…}}, b₂::Box{2, CoordRefSystems.Cartesian{…}})
   @ Meshes ~/.julia/packages/Meshes/EqjBS/src/predicates/intersects.jl:30
 [5] top-level scope
   @ REPL[11]:1
Some type information was truncated. Use `show(err)` to see complete types.

kylebeggs commented 2 weeks ago

I think to fix this we just need to add a convert in the constructor here (https://github.com/JuliaGeometry/Meshes.jl/blob/master/src/primitives/box.jl#L22) but maybe you guys have a plan on how you are going about converting units. Is it even necessary to have the points of a box be the same units? Perhaps we can relax the type annotation in the struct?

juliohm commented 2 weeks ago

Thanks for catching this! We probably need a conversion method as you suggested, to make sure that both points (min and max) have the same CRS. The main assumption in the current design is that geometries and domains with multiple points must share a unique CRS.

kylebeggs commented 2 weeks ago

In that case, what unit should we always convert to? meters? How do you design a convert method for this? What if you have mm or cm? Its ambiguous

juliohm commented 2 weeks ago

We used a simple promote call to fix the issue in this algorithm. Promotion rules for geometries is something that we can also consider in the future.

Em ter., 2 de jul. de 2024, 18:04, Kyle Beggs @.***> escreveu:

In that case, what unit should we always convert to? meters? How do you design a convert method for this? What if you have mm or cm? Its ambiguous

— Reply to this email directly, view it on GitHub https://github.com/JuliaGeometry/Meshes.jl/issues/920#issuecomment-2204416153, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZQW3JQDLY4BWKJT3PJRZDZKMIWBAVCNFSM6AAAAABKIFPRRSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMBUGQYTMMJVGM . You are receiving this because you commented.Message ID: @.***>

JuliaGeometry / Meshes.jl

`intersects(::Box{2,...Unitful.m...}, ::Box{2,...Unitful.mm...})` broken #920