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

roundRect requiring an array #20

Closed
jagenjo opened this issue Jul 8, 2021 · 6 comments
Closed

roundRect requiring an array #20

jagenjo opened this issue Jul 8, 2021 · 6 comments

Comments

@jagenjo
Copy link

jagenjo commented Jul 8, 2021

I can see that the new roundRect requires an array to specify the radius, even when the radius is constant among all corners.

Knowing that most of us are fighting the garbage collector constantly, I do not see it smart to force to pass an array with every call of roundRect.

More when other functions in canvas take advantage of the variable number of arguments, like drawImage.

I suggest to allow the radius parameter to be a number.

@Kaiido
Copy link

Kaiido commented Jul 12, 2021

That was already brought in whatwg/html#5619 (comment) which didn't receive much returns, in either ways.
I wonder if this point shouldn't be brought again on the PR directly now.

FWIW, I personally don't have a strong opinion on this, with a small preference for a single Array, as I feel it is cleaner to have a single signature, and that very long signatures were actually not a great design decision (over the years I often wondered if having a dictionary for drawImage wouldn't have made it more friendly). If you are that much fighting against GC, I guess you could simply store that Array somewhere and reuse it every frame, like you probably already do with a lot of other objects.
But once again, these are not strong oppositions from my side.

@fserb
Copy link
Owner

fserb commented Nov 17, 2021

I'm fine with it too.

@mysteryDate wdyt?

@mysteryDate
Copy link
Collaborator

I've actually had this on my todo list for a while. I totally agree with a single number for ergonomic reasons, didn't think abuot the garbage collector.

@Kaiido
Copy link

Kaiido commented Feb 10, 2022

@mysteryDate I just did update my polyfill to include this single radius option, however doing so I noticed that the current Chrome's implementation has some weird behavior with some stoopid inputs.

I'm not an IDL expert myself, but I believe that the JS value undefined should be converted to NaN by "unrestricted double" (I tested it with globalAlpha and indeed it is there, at least observably: the value did not change).
However, in roundRect undefined will be (observably) treated as 0: the rectangle will be drawn.
This is true for both roundRect(x, y, w, h, undefined) and roundRect(x, y, w, h, [undefined]).

Also the behavior of an Array as the first member of a list in radii seems similarly broken:
passing roundRect(x, y, w, h, [[20]]) treats radii as 0 (a rectangle with no rounded border is drawn), while if I'm not mistaken, [20] to unrestricted double should be 20.

Here is a fiddle with these observations you can check: https://jsfiddle.net/oxvqm597/
And here the same fiddle ran through my polyfill: https://jsfiddle.net/oxvqm597/1/

Could you please confirm that my reading is correct?

@mysteryDate
Copy link
Collaborator

@fserb
Copy link
Owner

fserb commented Aug 31, 2022

Closing this as we merged it! Thanks a lot for the feedback. :)

@fserb fserb closed this as completed Aug 31, 2022
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

No branches or pull requests

4 participants