p5.js icon indicating copy to clipboard operation
p5.js copied to clipboard

Line vert shader offsets too much on small coordinates

Open TiborUdvari opened this issue 1 year ago • 8 comments

Most appropriate sub-area of p5.js?

  • [ ] Accessibility
  • [ ] Color
  • [ ] Core/Environment/Rendering
  • [ ] Data
  • [ ] DOM
  • [ ] Events
  • [ ] Image
  • [ ] IO
  • [ ] Math
  • [ ] Typography
  • [ ] Utilities
  • [X] WebGL
  • [ ] Build process
  • [ ] Unit testing
  • [ ] Internationalization
  • [ ] Friendly errors
  • [ ] Other (specify if possible)

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

TiborUdvari avatar Sep 02 '24 14:09 TiborUdvari

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.

davepagurek avatar Sep 02 '24 16:09 davepagurek

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.

TiborUdvari avatar Sep 02 '24 16:09 TiborUdvari

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

davepagurek avatar Sep 02 '24 16:09 davepagurek

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 avatar Sep 03 '24 09:09 TiborUdvari

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 avatar Sep 03 '24 10:09 TiborUdvari

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 avatar Sep 03 '24 11:09 TiborUdvari

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 🥲

TiborUdvari avatar Sep 03 '24 13:09 TiborUdvari

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.

davepagurek avatar Sep 05 '24 14:09 davepagurek