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

Stroke overlapping issue on co-planar shapes in WebGL #2938

Closed stalgiag closed 1 year ago

stalgiag commented 6 years ago

When two shapes are sharing a z-plane, their strokes render on top of all of the fills. image As @Spongman pointed out, this was reported in #2105 & #2510, and 'fixed' in #2511 and #2515. The problem is that fixes that addressed the stroke z-fighting ended up producing the popping behavior seen here.

After a discussion and the realization that Processing 3.3.6 also had the stroke overlap issue, the decision was made to leave the overlap and fix the popping for now as per @Spongman's fix.

This is a bug that has been passed back and forth with no obvious fix for both simultaneously. Creating an issue just in case someone encounters a simple fix, or future architectural changes make a fix easier. You can see the conversation here that outlines why the decision was made to prioritize the stroke popping over the overlap. An eventual fix would be great because as @kjhollen indicated in that discussion:

It doesn't really feel like a lesson in stacking and ordering objects in 3D graphics because it subverts the expectations of an end user. The fill ordering follows expectations (draw order) but the lines behave differently. It forces the end user to know that the lines are drawn in a separate render pass, at a different depth, and work around that. I think it's probably worth tracking this in another issue, in case someone has an idea for another render method that might fix it in the future!

davepagurek commented 1 year ago

Oops just noticed there's already an issue for this! I'm going to close the issue I just made and paste the examples/discussion from there over to here.


The problem

Currently, in order to make lines visible on top of 3D geometry at all angles, we move lines slightly closer to the camera. This has a few unfortunate side effects:

1. 2D code using the painter's algorithm looks different in WebGL.

Here's some example code which, in 2D, draws a second rectangle overtop of the first. In WebGL, the lines stick through:

Code2DWebGL
```js function setup() { createCanvas(170, 170, WEBGL); } function draw() { background(220); translate(-width/2, -height/2) stroke('black') fill('red') rect(20, 20, 100, 100) fill('blue') rect(50, 50, 100, 100) } ``` ![image](https://user-images.githubusercontent.com/5315059/214705666-87ec3bcd-1410-411f-8687-50819a328027.png) ![image](https://user-images.githubusercontent.com/5315059/214705725-277eb604-c666-4aba-a325-d4a2c9862020.png)

Live: https://editor.p5js.org/davepagurek/sketches/5L3gMnhB1

2. When rotating shapes in 3D, lines "pop" sometimes

In https://github.com/processing/p5.js/pull/5802, when testing higher-fidelity strokes, we noticed this effect, although it was also present before this change. Notice how some lines shift widths:

Live: https://editor.p5js.org/davepagurek/sketches/8SSUdaVhi

It seems to be because of this line that puts line vertices slightly above faces. If you keep rotating a face, there's an angle at which that scale value isn't enough to keep the line on top of the face, so it disappears.

Potential solutions

If we use a scale of 1 (no z change) for the line when it's perpendicular to the screen and slowly bring it closer to the camera it gets more perpendicular, it would fix the first problem. However, this would still introduce some issues: e.g. for lines on a cube, when one line is directly perpendicular to the camera, one connecting to it will not be. Increasing the scale on the non-perpendicular line might cause the joint between the lines to appear disconnected. Perhaps there's a similar method that wouldn't disconnect joints?

davepagurek commented 1 year ago

Small update: I mentioned that changing the scale as lines move away from the camera might disconnect edges for cubes. However, it seems like in practice, this doesn't happen actually, which is great! I've added something like this:

float facingCamera = pow(
  // The word space tangent's z value is 0 if it's facing the camera
  abs(normalize(posqIn-posp).z),

  // Using pow() here to ramp `facingCamera` up from 0 to 1 really quickly
  // so most lines get scaled and don't get clipped
  0.25
);
float scale = mix(1., 0.995, facingCamera);

I also noticed that in retained mode, we draw lines first and then fills, while in immediate mode, we draw lines last. I also moved the line draws last for all cases, so that if the lines have the same z as the fills, the lines show up on top.

Here's what that technique looks like:

BeforeAfter
![line-before](https://user-images.githubusercontent.com/5315059/214722498-f92384e9-921c-479d-9ccd-d33976658ed3.gif) [Live](https://editor.p5js.org/davepagurek/sketches/8SSUdaVhi) ![line-fix](https://user-images.githubusercontent.com/5315059/214722462-eee58cf4-5a6b-4d26-acbb-1515d972ce15.gif) [Live](https://editor.p5js.org/davepagurek/sketches/JBttrCMV-)

This also fixes the issue of lines going through coplanar shapes: image

Live: https://editor.p5js.org/davepagurek/sketches/aSFkpcttn

This doesn't fully fix the "popping" issue, but does seem to at least improve it while fixing the original coplanar issue. Does that seem like enough of an improvement to warrant adding this change?

stalgiag commented 1 year ago

Nice! The 'popping' is still noticeable but the stroke width feels more consistent with these changes. I also love seeing a fix for the coplanar issue. I support the change.

davepagurek commented 1 year ago

Going to close this now for organizational purposes because #5981 is merged in, but if the "popping" issue is still noticeable enough to be a problem we can open a new issue for that!

davepagurek commented 1 year ago

Never mind, opening this up again because I realized that PR isn't actually merged 😅 Must have mixed up my github notifications.

davepagurek commented 1 year ago

Closed for real this time with https://github.com/processing/p5.js/pull/6109!