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.45k stars 3.29k forks source link

Line vert shader offsets too much on small coordinates #7200

Open TiborUdvari opened 1 week ago

TiborUdvari commented 1 week ago

Most appropriate sub-area of p5.js?

p5.js version

1.10.0

Web browser and version

Chromium: 128.0.6613.85

Operating system

No response

Steps to reproduce this

The change here #7064 introduced a bug for small coordinates.

I'm working on updating p5.xr to work with v1.10.0. WebXR uses real world coordinates, as in 1 unit = 1 meter (handy !), so this rendering issue would come up all the time. See the difference with the old shader and the new one:

https://github.com/user-attachments/assets/4e1e6eed-80c3-49bd-b5db-2e5988bc6caa

https://github.com/user-attachments/assets/548fd066-e90c-4ac3-a560-3d40a23024fd

davepagurek commented 1 week ago

Definitely ok with going back to a variant of the old shader, but we will need to look back to the discussion of https://github.com/processing/p5.js/issues/6956 and figure out a solution that works for all these cases.

TiborUdvari commented 1 week ago

Haven't looked into the details yet, but since it seems to be related to large and small numbers, maybe we would need to pass in a uniform from the camera to know which case it's in and multiply the scale factor appropriately or displace based on that.

davepagurek commented 1 week ago

We have uPerspective == 1 to tell us if we've got a perspective camera, so we could use that to switch between the two behaviours. A single solution feels cleaner but obviously something that actually works is preferable haha. We maybe just need to add ample comments to the code to explain why the branch is necessary

TiborUdvari commented 1 week ago

I'm not a shader expert, but I'll give it a try.

Note to self, validation tests:

Current bug: https://editor.p5js.org/TiborUdvari/sketches/JSosdUVBx

Initial bug discussed in #6956 . https://editor.p5js.org/vicdoval/sketches/VsL1ILZ-_ (need to copy paste the url with the _ for some reason)

TiborUdvari commented 1 week ago

I came up with a solution that mixes both solutions with uPerspective as you said (see below).

I looked into it a little bit, and I don't think that we can really rely on this param, because I could see people wanting to turn linePerspective off and still using small units.

Screenshots using modified line.vert shader from below.

Screenshot 2024-09-03 at 12 49 41 Screenshot 2024-09-03 at 12 49 52
  // using a scale <1 moves the lines towards perspective camera
  // in order to prevent popping effects due to half of
  // the line disappearing behind the geometry faces.
  float scale = mix(1., 0.995, facingCamera);
  float scaleFactor = mix(scale, 1.0, float(uPerspective));  // scale when uPerspective == 1
  // Moving vertices slightly toward the camera
  // to avoid depth-fighting with the fill triangles.
  // Discussed here:
  // http://www.opengl.org/discussion_boards/ubbthreads.php?ubb=showflat&Number=252848  
  posp.xyz = posp.xyz * scaleFactor;
  posqIn.xyz = posqIn.xyz * scaleFactor;
  posqOut.xyz = posqOut.xyz * scaleFactor;

  // Moving vertices slightly toward ortho camera 
  // https://github.com/processing/p5.js/issues/6956 
  float zOffset = mix(-0.00045, -1., facingCamera);
  float zAdjustment = mix(0.0, zOffset, float(uPerspective)); // zOffset when uPerspective == 0

  posp.z -= zAdjustment;
  posqIn.z -= zAdjustment;
  posqOut.z -= zAdjustment;
TiborUdvari commented 1 week ago

I did PR #7206 based on distance, which works ... It mixes both approaches based on distance. It feels a little hard coded to me, let me know if there would be a better way to do this.

TiborUdvari commented 1 week ago

I've been looking into this a little bit more deeply, the v1.9.4 shader also has issues with z fighting / popping effect at close coordinates. You can see the popping effect. I've mostly been using noFill so I never noticed before.

float scale = mix(1., 0.995, facingCamera);

https://github.com/user-attachments/assets/b3250cc1-525e-40cb-b1ed-26b603f312f5

Never letting it hit 1 does the trick for small units at least.

float scale = mix(0.999, 0.995, facingCamera);

It would be nice to find a solution that works for all use cases 🥲

davepagurek commented 1 week ago

Hmm I think the problem with it never hitting 1 is that if you're using WebGL mode to do 2D content, then strokes will always appear on top of fills instead of respecting the last-drawn-wins approach of 2D mode, which this bug was originally filed for: https://github.com/processing/p5.js/issues/2938.

It could be that in addition to a mix, we have something like facingCamera == 1.0 ? 1.0 : mix(...) so that it "jumps" to 1 if it's exactly facing the camera and handles the 2D case. We'd need to check if that causes a sudden pop anywhere as you move your view around in a full 3D scene like in your test though.