JuliaGraphics / Luxor.jl

Simple drawings using vector graphics; Cairo "for tourists!"
http://juliagraphics.github.io/Luxor.jl/
Other
575 stars 72 forks source link

Base Subtract operator between Luxor.Point and a Number is incorrect #270

Closed j-adel closed 4 months ago

j-adel commented 10 months ago

When doing a simple math operation between a Point and a Number such as Point(1,1)+10 gets a correct answer. However, subtraction such as 10-Point(1,1) gets Point(-9.0, -9.0). It's worth noting that -Point(1,1)+10 =>Point(9.0, 9.0) and 10+-Point(1,1)=>Point(9.0, 9.0). It's only the case of Number-Point that it fails. I couldn't find the Point type in Cair so I'm not sure where this issue is rooted. I'm really loving the package btw great job! :D

cormullion commented 10 months ago

I’m not sure what it’s even supposed to do 😂

Probably this needs to be looked at

https://github.com/JuliaGraphics/Luxor.jl/blob/d7c261e92b709eaf02020419ecbd8874e934f9fd/src/point.jl#L28

Cairo doesn’t know about Points 😀

cormullion commented 10 months ago

Thanks for the kind words! I might have fixed this now (on master)... 🤞

Hoping to make another release this week...

j-adel commented 10 months ago

Awesome! For the record, I think this worked for me:

function Base.:-(num::Number, p::Point)
    return Point(num - p.x, num - p.y)
end

function Base.:-(p1::Point, num::Number)
    return Point(p1.x - num, p1.y - num)
end
cormullion commented 10 months ago

I think I might just have copy-pasted too carelessly back when I wrote this bit... :)

j-adel commented 10 months ago

Haha yeah I just saw it. Btw this wrapper is pretty much perfect for generative art. I've gotten good results really quickly with it. I hope to make an article guide about it soon :D

cormullion commented 10 months ago

Looking forward to that! It’s mostly designed for that kind of use - parameterized graphic design and similar - so I’m happy that you’re getting some results you like!

stale[bot] commented 8 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

hyrodium commented 8 months ago

Is it reasonable to have a method like +(::Point, ::Number)?

julia> using Luxor

julia> Point(1,2) + 1
Point(2.0, 3.0)

julia> [1,2] + 1
ERROR: MethodError: no method matching +(::Vector{Int64}, ::Int64)
For element-wise addition, use broadcasting with dot syntax: array .+ scalar

I think it would be enough with .+ operator.

julia> Point(1,2) .+ 1
Point(2.0, 3.0)

julia> [1,2] .+ 1
2-element Vector{Int64}:
 2
 3
cormullion commented 8 months ago

It would be a breaking change.

hyrodium commented 8 months ago

Yeah, should we release the breaking change? If so, I can open a PR to deprecate the methods.

cormullion commented 8 months ago

Perhaps the next version of Luxor should be version 4.0, with some breaking changes, supporting only Julia 1.9 and above (thanks to package extensions). It might be good to wait until Julia 1.10 is released first before releasing 4.0 ... 🤔

hyrodium commented 8 months ago

I was planning to release 3.x with deprecated methods (e.g. +(::Point, ::Number)), and then release 4.0 with removing the deprecated methods. However, we have other reasons for the next breaking release, so it would be okay not to release the next 3.x. 🤔