Update and simplify implementation of arc, ellipse, and possibly rect

[GregStanton]

Increasing Access

This enhancement would simplify p5's source code and would reduce the mathematical prerequisites for understanding it. This would increase access among p5 contributors.

Most appropriate sub-area of p5.js?

• Accessibility
• Color
• Core/Environment/Rendering
• Data
• DOM
• Events
• Image
• IO
• Math
• Typography
• Utilities
• WebGL
• Build Process
• Unit Testing
• Internalization
• Friendly Errors
• Other (specify if possible)

Feature enhancement details

Proposal
Use modern features of the native canvas API to simplify the implementation of arc, ellipse, and possibly rect.

The problem
The 2D renderer's current implementation of arc and ellipse is complicated by the fact that it uses Bézier curves, which cannot exactly represent either primitive. The implementation is complicated enough that the source comments link to an outside paper for an explanation of the underlying mathematics.

The implementation of rect may also be more complicated than necessary, since rounded corners require clipping the radii and building the rectangle with the canvas's arcTo method.

The solution
The use of Bézier curves in p5's 2D implementation of arc and ellipse dates to p5's addition of ellipse on July 4, 2013. At that time, the canvas didn't have a native ellipse method. Now it does. The canvas's ellipse method actually draws elliptical arcs, so it could be used to implement both arc and ellipse in p5. This would be a significant simplification. Also, ellipse has been available in Chrome since November 2013, so bugs and browser compatibility don't seem like a significant concern.

The situation with rect is similar. After p5 already supported rectangles with rounded corners, the roundRect method was added to the native Canvas API. However, this addition is much more recent (browsers began supporting it in 2022, with Firefox support only being added in 2023).

Update:
Here's a task list to help us keep track of what work has been completed, since @tuminzee has already done the work for ellipse() (it just needs to be merged). Note that the task list does include rect(), based on the comments on this issue.

• Update and simplify implementation of ellipse()
• Update and simplify implementation of arc()
• Update and simplify implementation of rect()

[limzykenneth]

Both the native canvas ellipse and roundRect methods are pretty well supported across the board so I'm happy for this to go ahead as long as it doesn't change the user facing API (which it probably won't need to anyway).

[GregStanton]

@limzykenneth Great! I'm not sure when I'd be able to work on this. Would it be good to add a Help Wanted label?

[Qianqianye]

Great suggestions @GregStanton. Just added the 'Help Wanted' label.

[ghost]

Hello, i am new to open source and have been studying p5.js from somedays. I do get the lines after the tasks list, but i do not get the first line of the issue. Can someone please help me understand the issue?
Thanks!

[tuminzee]

@GregStanton I would like to help with this issue

[tuminzee]

so a good solution would be to create a blackbox using the web native canvas api, which can replace
the complex implementation of _acuteArcToBezier ?

[davepagurek]

I'm not sure I fully follow what you mean by blackbox, but I think the rest sounds correct! the implementations of arc and ellipse could both be updated to use the native canvas ellipse method for their implementations.

[GregStanton]

Hi @tuminzee! I'll add to @davepagurek's answer in case it helps.

From the user's perspective, p5's arc, ellipse, and rect are already black boxes. In each case, the user enters input to the box (like width and height), and the box outputs a shape. The user doesn't see how the shape gets made, so they can imagine the box is painted black.

However, developers contributing to p5 may need to look inside the boxes. If they look inside arc and ellipse, they'll see bezierCurveTo; everything will be simpler if you remove that and use the canvas's native ellipse method instead. Inside arc, developers will also see _acuteArcToBezier; it should be fine to delete that method entirely. After making your changes, you just need to be sure that the user won't know anything has changed at all!

I hope that helps.

P.S. If you want to simplify p5's implementation of rect, you can create rounded corners using the canvas's native roundRect method instead of its arcTo method.

[tuminzee]

@GregStanton this helps a lot 👍🏼
I will try my best to make this work, I will try first my changing what is inside ellipse , later will working on the arc and rect

[tuminzee]

@GregStanton can you have a look at #6499
I refactored ellipse function

If you agree on this implementation I will continue the same for the remaining functions

[GregStanton]

@tuminzee Excellent! Thanks for doing this. I didn't check every line, but this looks like what I had in mind. I did make a few minor comments about coding style on the commit itself.

[tuminzee]

@GregStanton I have resolved your comments here b73af84

I will update the PR with the addition of other functions as well.
Is there any tests which I should be writing for this?
Also I am only relying on the visuals, if it looks the same on https://p5js.org/reference/ and on my local I am pushing it, is there any other checks which I should perform before pushing it?

[GregStanton]

That's great @tuminzee! About testing, @Qianqianye, @davepagurek, and @limzykenneth will have a better idea of p5's usual process. I'll share my thoughts about it, and hopefully they'll be able to provide feedback.

Current tests: There are already some unit tests that should run automatically. However, the tests I found are pretty basic, so my inclination would be to add visual tests. Visual tests can be done manually, as you're doing, and it seems like that's the usual approach with p5 (although I'm not sure).

Automated visual tests: Another approach is to automate the visual tests. In this case, the idea is to run sample input through p5's current implementation of ellipse and create an image from that. Let's call this the base image. Then, you run the same input through your implementation and create another image. Let's call that the compare image. The test does a pixel-by-pixel comparison of the base image and the compare image. If there's a difference, the test fails.

Pros and cons:

• Advantages of automated visual testing include greater precision, reusability, and transparency. By transparency, I mean that others can inspect your tests and verify that you've covered the important cases. Ideally, it'd be good to have visual tests for typical usage, as well as edge cases (e.g. if we consider negative, zero, and positive cases for width and for height, there are nine cases). If we want to be really careful, I suppose we could even write tests under different settings (e.g. smooth vs. noSmooth).
• A disadvantage of automated visual testing is that it may take extra time up front (although it may save time in the long run, since the tests would be reusable in the future).

[tuminzee]

Great @GregStanton I will try my best to come up with tests, if not I will at least finish the PR with manual testing

[limzykenneth]

Visual tests are something I've been thinking about as well, it feels a bit difficult to implement so that the results are consistent across systems and runs but I think is still worth exploring. That said I think it probably fits as a separate issue and for this feature, manual tests should suffice for now.

[ksen0]

Hello @limzykenneth , I see this has been reopened - could I ask what the status is? I saw the issue that refactored ellipse, are the other functions still in need to refactor? Is there a separate task for the idea of visual testing? I would actually love to work on implementing the idea above, if it's salient and has not already been done.

[limzykenneth]

@ksen0 There is a small check list in the top issue body, essentially ellipse is done but not arc and rect yet.

[ksen0]

@limzykenneth thanks! I will work on this.

Sorry if this has been covered elsewhere and I didn't see, but npm test (on clean fork) fails with:

  1) lib/addons/p5.sound.js
       process documentation
         example #1 works:
     Error: Timeout of 4000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

(First time working on p5.js. npm run build worked)

[limzykenneth]

@ksen0 What version of node.js and npm are you working with? Also your OS and version may be helpful to identify the issue as well.

[ksen0]

@limzykenneth 🙏 Mac OS 13.0 (22A380)

% npm -v       
10.8.2
% node -v
v20.17.0

[limzykenneth]

@ksen0 Those should be fine. Do you have Chrome installed?

[ksen0]

@limzykenneth yes. But anyway, it's not really an immediate blocker that 1 test doesn't pass. The suite as a whole runs, so I'll make an attempt at this issue for now:) Thanks for your quick responses.

(Edited on aug 29 to add: I am working on this. Mode of PIE not working yet but otherwise arc is almost refactored to use ctx functions. I'm trying to figure out a sensible usage of canvas .ellipse and .arc since neither is exactly doing the p5 arc in PIE mode.)

[ksen0]

Hello @limzykenneth!

I just finished refactoring arc to simplify the code as discussed above (remove usages of Bézier curves + the associated helper function). Does this look alright? 94e1969

I tested with all the example arc code (https://p5js.org/reference/p5/arc/) as well as a few other manual tests, but there are no automated tests covering this.

I also haven't been able to sort of out my npm test problem. Chrome is fully updated, but same behavior as before, fails when it gets to the sound tests:

  1) lib/addons/p5.sound.js
       process documentation
         example #1 works:
     Error: Timeout of 4000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

EDITED: I also updated rect, both commits are in my fork: main...ksen0:p5.js:main it's my first time attempting a p5.js contribution, and I'm totally open to any feedback on this of course.

But not sure how to resolve my local failing tests issue.. what should I do? Should I make a pull request for one/both of the commits above, or do you have any suggestions on how I could investigate my local build issue (was there before any work I did)? Thank you!

Also, I realize it wasn't actually assigned to me, if someone else already did it, no worries. It was a nice introduction to the renderer. Sorry if actually working on it was not ok because it wasn't assigned, I'll just take it as a personal learning experience in that case.

[limzykenneth]

@ksen0 Please go ahead and file a single PR for both. The test can run as part of the PR so that can be another way to do it.

For the local test setup, as I cannot reproduce, I would need you to try to debug as much as you can if you are able to. No worries if not but just to note that I can't offer much else in terms of possible resolution in this case as I don't have enough information to debug.

[ksen0]

Thanks @limzykenneth ! I added the PR above.
I'll sort out my local problems:)