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

Fix vector normalization in epaint/tessellator/add_line_loop. #5364

Closed
wants to merge 1 commit into from

Conversation

PangolinGiLA
Copy link

In epaint/tessellator/add_line_loop normal vectors of added points were not normalized correctly, they were divided by length squared, what caused their length to explode for some points close together.

Before:
before

After:
after

  • I have followed the instructions in the PR template

Copy link

Preview available at https://egui-pr-preview.github.io/pr/5364-normalization-fix
Note that it might take a couple seconds for the update to show up after the preview_build workflow has completed.

@lucasmerlin
Copy link
Collaborator

Seems like this causes a couple small differences in the snapshot tests, the biggest one is visible in the rendering test:
(the thinner one is the new, updated snapshot)

Bildschirmaufnahme.2024-11-11.um.14.28.34.mov

And some minor differences in the bezier test:
(the one where the top green line is more faint is the new one)
Also, notice how the triangle to close the window seems slightly worse antialiased.

Bildschirmaufnahme.2024-11-11.um.14.29.26.mov

Not sure which versions are more correct though, just wanted to share the differences I noted.

Here is a link to the snapshot test results: https://github.com/emilk/egui/actions/runs/11778645510/artifacts/2171312779

@PangolinGiLA You'll need to run the tests with UPDATE_SNAPSHOTS=true to update the snapshots before we can merge this

@lucasmerlin lucasmerlin added visuals Renderings / graphics releated epaint labels Nov 11, 2024
@@ -470,7 +470,7 @@ impl Path {
self.add_point(points[i], n1c / n1c.length_sq());
} else {
// miter join
self.add_point(points[i], normal / length_sq);
self.add_point(points[i], normal.normalized());
Copy link
Owner

@emilk emilk Nov 11, 2024

Choose a reason for hiding this comment

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

Dividing with length-squared is what gets the correct widths for a miter-joins. If this fails for some angles, let's figure out why. The CUT_OFF_SHARP_CORNERS is meant to catch this problem.

Copy link
Author

Choose a reason for hiding this comment

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

Oh sorry, I didn't realize that, I just changed it thinking normal vectors always have unit length, and this fixed my problem with rendering.

The problem appeared when I tried to render points distributed this way (this is the bottom part of the image above):
points

Copy link
Author

Choose a reason for hiding this comment

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

Oh I just read the issue linked in the code. I guess i should have done a little bit more research before opening the PR.

@emilk emilk marked this pull request as draft November 11, 2024 15:48
@emilk
Copy link
Owner

emilk commented Nov 13, 2024

I would love to have a proper solution to this problem, but this PR isn't it

@emilk emilk closed this Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
epaint visuals Renderings / graphics releated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants