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 fix for small units #7206

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

TiborUdvari
Copy link
Contributor

Resolves #7200

Resolves #[Add issue number here]

Changes:

Screenshots of the change:

PR Checklist

Copy link

welcome bot commented Sep 3, 2024

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page!

@perminder-17
Copy link
Contributor

perminder-17 commented Sep 15, 2024

Hi @TiborUdvari , it's a great work.

Thank you for opening the pull request! I just tested the issues you were facing, specifically the offset for small coordinates being too high. Fortunately, it doesn't seem to reopen the initial issue (#6956).

Your implementation of dynamicScale and dynamicZAdjustment is great and working well.

I do have one question regarding the "popping" effect you are talking here #7200 (comment). I wasn't able to reproduce it using the code in your PR. Could you share the code where you encountered this effect? Additionally, if it’s still reproducible, does the logic @davepagurek mentioned (facingCamera == 1.0 ? 1.0 : mix(0.999, 0.995, facingCamera)) resolve the issue, or do we need to explore other solutions?

@TiborUdvari
Copy link
Contributor Author

I don't think I understood 100% the issues with the popping effects, so I didn't go further on this. What I didn't like about my solution is that it was a mix between two solutions and I didn't understand the whole problem space, so I didn't go further on it. It does apply a quick fix for what we have now though.

I had to monkey patch p5.xr with the previous version of the shader as a temporary solution, as it really affects most of the AR examples, as they use mostly real world coords, in meters.

Unfortunately didn't have the time to go deeper on this.

@perminder-17
Copy link
Contributor

perminder-17 commented Sep 18, 2024

I didn't like about my solution is that it was a mix between two solutions and I didn't understand the whole problem space

This might not be the cleanest solution, but given the current options (since we haven't found any better/cleaner solution yet) I think your solution works well. As for the flickering or popping effects, I couldn't reproduce them on my end. I also reviewed the reference documentation with your code, and everything looks fine to me. Thanks for the fix!

Copy link
Contributor

@perminder-17 perminder-17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll approve this from my side, and the maintainers can update or merge it when they have time.

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this so far! If you or anyone else has any further ideas on a new approach, feel free to make a new issue/PR, and in the mean time hopefully this improves the situation a bit.

@davepagurek davepagurek merged commit e624307 into processing:main Sep 20, 2024
2 checks passed
davepagurek added a commit that referenced this pull request Nov 9, 2024
davepagurek added a commit that referenced this pull request Nov 9, 2024
* Will high precision floats make lines render on top?

* Merge pull request #7206 from TiborUdvari/fix/line.vert-v1.10.0

Line.vert fix for small units

* Apply no z offset to lines when facing the camera

Line.vert fix for small units

* Add test case for ortho strokes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Line vert shader offsets too much on small coordinates
3 participants