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

Added round corner property for rect() in WebGL mode #5789

Merged
merged 1 commit into from
Sep 9, 2022

Conversation

ShenpaiSharma
Copy link
Contributor

@ShenpaiSharma ShenpaiSharma commented Sep 7, 2022

Addresses #5001

Changes:

Finally decided to follow what processing does to round the corner of rect(). Still, here we use the retained-mode for drawing a rectangle if the user does not provide args for rounding the rectangle and use Immediate mode to round the rectangle corner if the user provides args for rounding corners.
As mentioned by @aferriss comment also, using vertex and quadraticVertex() to draw rounded corners has a few issues like:

  • Larger strokeWeight shows some artifacts.

image

  • texturing doesn't work while using vertex and quadraticVertex().

That's why followed retained-mode until any user deliberately wanted to use the rounded rectangle behavior.

Screenshots of the change:

Code:

function setup() {
 // Create the canvas
 createCanvas(720, 400, WEBGL);
 background(200);
 
 // Set colors
 fill(204, 101, 192, 127);
 stroke(127, 63, 120);
 
 // A rectangle
 rect(-100, -100, 200, 80, 40, 1, 5, 1);
}

Before:
image

After:
image

PR Checklist

@davepagurek
Copy link
Contributor

Hi, thanks for taking this on! How do you feel about manually creating more vertices with sin and cos to do the corner arcs rather than using quadraticVertex? Some benefits of this would be:

Downsides/complications that I can see:

  • It's more work
  • We'd have to decide how many vertices to break the corners down into

@davepagurek
Copy link
Contributor

I created an issue to discuss the stroke issues you mentioned: #5790

@ShenpaiSharma
Copy link
Contributor Author

Thanks, @davepagurek, for your insights, obviously that's a much better idea to implement the round edges using sin and cos, but looking at the complications, we actually thought of following this mid path and also processing follows the same implementation i.e., using vertex and quadraticVertex().
Also, it may reduce enough confusion for users trying to work with rect().

@davepagurek
Copy link
Contributor

That makes sense. Just for clarification, do you think there's a benefit of sticking with quadraticVertex vs using sin/cos? Thinking ahead, I can write a follow-up PR to add support for texture coordinates in rounded rect calls, but I'd probably do that by more easily by switching to sin/cos -- fixing #5722 in general is a bigger project, so I'd have to do the bezier math manually in the mean time, which is doable but I'm much less confident in my abilities to do that than to do some trigonometry 🙂 The structure you set up will be useful regardless (thanks!) but I want to make sure I wouldn't be missing something when doing so.

Also, if we're leaving texture coordinate support to another PR, do you mind changing your PR description to Addresses #5001 instread of Resolves so it keeps the issue open until then?

@ShenpaiSharma
Copy link
Contributor Author

Yes, I think using sin and cos will be much more preferable here, but once #5772 will be fixed, this solution will work fine too. Also, this will solve the problem faced by users working with rounded rect(). As users working with rounded rect now have to draw that with bezierVertex or quadraticVertex, and thus facing the same problem in passing the texture coordinates.

@ShenpaiSharma
Copy link
Contributor Author

ShenpaiSharma commented Sep 9, 2022

Also, if we're leaving texture coordinate support to another PR, do you mind changing your PR description to Addresses #5001 instread of Resolves so it keeps the issue open until then?

Thanks @davepagurek , Yeah, that makes more sense, will do that ASAP

@davepagurek
Copy link
Contributor

Great, thanks! After I finish work for the day I'll run the code just to double check everything and then merge this in.

@davepagurek davepagurek merged commit 5c2d252 into processing:main Sep 9, 2022
@Qianqianye
Copy link
Contributor

Thanks @ShenpaiSharma and @davepagurek!

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 this pull request may close these issues.

3 participants