Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
j-adel opened this issue Sep 6, 2023 · 12 comments · Fixed by #294
Closed

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

j-adel opened this issue Sep 6, 2023 · 12 comments · Fixed by #294

Comments

@j-adel
Copy link
Contributor

j-adel commented Sep 6, 2023

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
Copy link
Member

cormullion commented Sep 6, 2023

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

Probably this needs to be looked at

-(z::Number, p1::Point) = Point(p1.x - z, p1.y - z)

Cairo doesn’t know about Points 😀

@cormullion
Copy link
Member

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

Hoping to make another release this week...

@j-adel
Copy link
Contributor Author

j-adel commented Sep 6, 2023

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
Copy link
Member

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

@j-adel
Copy link
Contributor Author

j-adel commented Sep 6, 2023

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
Copy link
Member

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
Copy link

stale bot commented Oct 15, 2023

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.

@stale stale bot added the stale stale/inactive/superseded label Oct 15, 2023
@hyrodium
Copy link
Contributor

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

@stale stale bot removed the stale stale/inactive/superseded label Oct 23, 2023
@cormullion
Copy link
Member

It would be a breaking change.

@hyrodium
Copy link
Contributor

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

@cormullion
Copy link
Member

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
Copy link
Contributor

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. 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants