p5.js
p5.js copied to clipboard
processing
Update and simplify implementation of arc, ellipse, and possibly rect
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
- [X] 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.
- [x] Update and simplify implementation of
ellipse() - [ ] Update and simplify implementation of
arc() - [ ] Update and simplify implementation of
rect()
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).
@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?
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!
so a good solution would be to create a blackbox using the web native canvas api, which can replace the complex implementation of _acuteArcToBezier ?
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.
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.
@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
@GregStanton can you have a look at https://github.com/processing/p5.js/pull/6499
I refactored ellipse function
If you agree on this implementation I will continue the same for the remaining functions
@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.
@GregStanton I have resolved your comments here https://github.com/processing/p5.js/pull/6499/commits/b73af847cc0c949630710fa162e1f95316cd3d63
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?
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.
smoothvs.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).
Great @GregStanton I will try my best to come up with tests, if not I will at least finish the PR with manual testing
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.
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.
