search

processing / p5.js

p5.js is a client-side JS platform that empowers artists, designers, students, and anyone to learn to code and express themselves creatively on the web. It is based on the core principles of Processing. http://twitter.com/p5xjs —
http://p5js.org/
GNU Lesser General Public License v2.1
21.47k stars 3.29k forks source link

Mixing vertex() with curveVertex() in beginContour() breaks in 2D mode #6555

Open davepagurek opened 10 months ago

davepagurek commented 10 months ago

Most appropriate sub-area of p5.js?

p5.js version

1.8.0

Web browser and version

Firefox 117

Operating System

macOS 14.0

Steps to reproduce this

Steps:

In beginShape:

  1. Create an outline using regular vertex
  2. Create a contour using curveVertex

Expected (works in WebGL mode): image

Actual results in 2D mode: image

It looks like 2D mode is combining all the vertices into one big path.

Snippet:

Broken 2D mode code: (live)

function setup() {
  createCanvas(200, 200);

  background(200);
  translate(width/2-50,height/2-50);

  beginShape();
  vertex(0, 0);
  vertex(100, 0);
  vertex(100, 100);
  vertex(0, 100);
  vertex(0, 0);

  beginContour();

  curveVertex(84, 91);
  curveVertex(84, 91);
  curveVertex(68, 19);
  curveVertex(21, 17);
  curveVertex(32, 91);
  curveVertex(32, 91);

  endContour();
  endShape();

  point(84, 91);
  point(68, 19);
  point(21, 17);
  point(32, 91);
}

Working WebGL code: (live)

function setup() {
  createCanvas(200, 200, WEBGL);

  background(200);
  translate(-50, -50);

  beginShape();
  vertex(0, 0);
  vertex(100, 0);
  vertex(100, 100);
  vertex(0, 100);
  vertex(0, 0);

  beginContour();

  curveVertex(84, 91);
  curveVertex(84, 91);
  curveVertex(68, 19);
  curveVertex(21, 17);
  curveVertex(32, 91);
  curveVertex(32, 91);

  endContour();
  endShape();

  point(84, 91);
  point(68, 19);
  point(21, 17);
  point(32, 91);
}
deveshidwivedi commented 10 months ago

Hi @davepagurek! I tried running a few similar codes to this and yes, mixing vertex() and curveVertex() in the same contour doesn't give desired output. Could it be due to the winding order in the two different modes? As for the solution, could we use different approaches like bezierVertex() or quadraticVertex()?

davepagurek commented 10 months ago

The main issue that jumps out to me isn't so much about winding order, but the fact that the non curved vertices on the outside become curved in 2D mode, and that the edges between the outside and the contour get connected. It leads me to believe that the bug is in 2D mode when it draws the paths, where it is mistakenly thinking all the vertices are curve vertices. The solution would be to identify where that is happening and correct it so that the output matches WebGL mode, where this already is accommodated.

GregStanton commented 10 months ago

Replying to https://github.com/processing/p5.js/issues/6555#issuecomment-1812547757 by @davepagurek:

It leads me to believe that the bug is in 2D mode when it draws the paths, where it is mistakenly thinking all the vertices are curve vertices. The solution would be to identify where that is happening and correct it

I recently put together a kind of verbal map of the relevant portions of the codebase to help with this kind of thing! It explains the location and purpose of the various working parts. The purpose was to make it easier to implement arcVertex() as proposed in issue #6459, but I wrote it partly as a general reference for work on this area. Given the way the code is currently structured and divided across files, I figured this could save some time.

However, I should note that I'm planning to submit a new issue that will involve general refactoring of this part of the codebase, as discussed on Discord. If there's a consensus in support of the refactoring, it might make sense to tackle that issue first, to make the current issue and #6459 easier to manage.

limzykenneth commented 10 months ago

I think if it make sense to have a general refactor of the codebase to clean up the implementation and use newer API, it would be worth to do it. It is a pretty complicated part of the overall library so I do enticipate it will take time so if you are starting to work on it let us know and keep us in the loop, we can help however we can and also to review new issues in that context as well.

deveshidwivedi commented 10 months ago

I'd really like to work on this! @limzykenneth @davepagurek. It does seem complicated but I could try. I will be very grateful for any help and guidance.

GregStanton commented 10 months ago

Thanks @limzykenneth! I just posted issue #6560, which should help us move forward with the open issues relating to the vertex functions, including this issue. Any help would be great!

If you're interested in helping @deveshidwivedi, my sense is that work on #6560 will probably happen first, but I hope others will share their thoughts about this.

deveshidwivedi commented 10 months ago

Sure @GregStanton! Thank you.

Forchapeatl commented 1 week ago

Hello @indefinit , @nickmcintyre , @Spongman , @hsab and @AdilRabbani . I am Forcha Pearl From Cameroon . I am sorry for dragging you people in to this issue . I have been cracking my head for days but I cannot seem to find the bug here and my experimental results keep getting worse. Please I wish like to beg for you help on this issue, any hints , books or directions will be very helpful to me.

GregStanton commented 1 week ago

Hi @Forchapeatl! This bug is part of a set of related issues. We've diagnosed the problem and have made a plan for solving it. The short explanation is that this problem is not specific to vertex() and curveVertex(). It's caused by the overall design of beginShape(), endShape(), and the various vertex functions. In case you're interested to learn more, I'll outline the main issues that are involved.

Vertex implementation: We plan to solve multiple bugs with a complete refactor of this area of the codebase. I'm starting to implement the plan now. To learn more about the work that's involved, check out #6560. Note that this is a large project, so the work will take some time.

Vertex usage: Currently, there is also a proposal to redesign the API for the vertex functions, in issue #6766. This will make it easier for p5.js users to understand how to make shapes with the vertex functions, and it will potentially allow for new types of shapes.

p5.js 2.0: Both of the issues above are a part of a large effort to upgrade p5.js to the next major version. To learn about this effort more generally, including a proposed timeline, check out #6678.

If you're interested in contributing, you might start by looking at the second issue above, for the API redesign. It contains a lot of discussion, but the main design is summarized at the very top. If you want to share questions or comments about the proposed design, that would be very helpful!