Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Line vert shader offsets too much on small coordinates #7200

Closed
1 of 17 tasks
TiborUdvari opened this issue Sep 2, 2024 · 8 comments · Fixed by #7206
Closed
1 of 17 tasks

Line vert shader offsets too much on small coordinates #7200

TiborUdvari opened this issue Sep 2, 2024 · 8 comments · Fixed by #7206

Comments

@TiborUdvari
Copy link
Contributor

Most appropriate sub-area of p5.js?

  • Accessibility
  • Color
  • Core/Environment/Rendering
  • Data
  • DOM
  • Events
  • Image
  • IO
  • Math
  • Typography
  • Utilities
  • 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:

line-renderer-new.mov
line-renderer-old.mov
@davepagurek
Copy link
Contributor

Definitely ok with going back to a variant of the old shader, but we will need to look back to the discussion of #6956 and figure out a solution that works for all these cases.

@TiborUdvari
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

TiborUdvari commented Sep 3, 2024

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
Copy link
Contributor Author

TiborUdvari commented Sep 3, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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);
Screen.Recording.2024-09-03.at.14.39.40.mov

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
Copy link
Contributor

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: #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants