search

JuliaImages / ImageDraw.jl

Drawing Package for JuliaImages
Other
27 stars 17 forks source link

RectanglePoints confuses me #69

Open tp2750 opened 2 years ago

tp2750 commented 2 years ago

When using CartesianIndex to define a rectangle, the resulting rectangle is not bounded by the pixels mentioned in the CartesianIndex.

Example:

julia>  RectanglePoints(CartesianIndex(1,2), CartesianIndex(3,4))
RectanglePoints(Point(1, 2), Point(3, 4))

But Point(1, 2) is the pixel with CartesianIndex(2,1).

The definition says:

 RectanglePoints(p1::CartesianIndex{2}, p2::CartesianIndex{2}) = RectanglePoints(Point(p1[1], p1[2]), Point(p2[1], p2[2]))

To me it would make a lot more sense to define it as 

 RectanglePoints(p1::CartesianIndex{2}, p2::CartesianIndex{2}) = RectanglePoints(Point(p1[2], p1[1]), Point(p2[2], p2[1]))

Of course this would be breaking.

Am I just thinking about this the wrong way?

opiateblush commented 2 years ago

I think you're right, it should be the other way around like you proposed. Mixing array indices and image coordinates can be confusing sometimes.

johnnychen94 commented 2 years ago

Agreed. I also found this confusing (https://github.com/JuliaImages/ImageDraw.jl/pull/40#pullrequestreview-452089317) so I'm supportive of a redesign. Just that I myself probably don't have the time to do it in the near future. A new package seems a more doable approach to make it less breaking. If there's anyone who wants to do it himself, I'll be very happy to comment and review.

tp2750 commented 2 years ago

Thank you for looking into this @johnnychen94 .

Would it be OK to just change the implementation of RectanglePoints(p1::CartesianIndex{2}, p2::CartesianIndex{2}) ? Of course all internal uses in ImageDraw.jl, and other packages in JuliaImages, should be changed at the same time. After all, ImageDraw.jl is below version 1.0, so breaking changes should be allowed.

I also vaguely recall that there is a way to search registered packages for dependences, so it should also be possible to submit pull requests to those affected.

johnnychen94 commented 2 years ago

Would it be OK to just change the implementation of RectanglePoints(p1::CartesianIndex{2}, p2::CartesianIndex{2}) ?

Might not be so trivial. At least we have to go through the entire codebase to ensure all types use the same coordinate convention.

I also vaguely recall that there is a way to search registered packages for dependences, so it should also be possible to submit pull requests to those affected.

JuliaHub has this feature https://juliahub.com/ui/Packages/ImageDraw/Gb86X/0.2.5?page=2 But keep in mind that there are potentially many non-registered projects, including the JuliaImages documentation https://github.com/JuliaImages/juliaimages.github.io. Thus a safer way to introduce this fundamental breaking change is to create a new package with a similar API, and let people actively switch to the new package. It's not an easy work to track down to this coordination swap change. Another reason that I'm in favor of rewriting is, that rewriting this package with Graphics or GeometryBasics dependencies could make it easier to introduce more features or compose with other ecosystems.