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

A more accurate algorithm for approximating round line joins #8275

Merged
merged 3 commits into from
Jul 30, 2019

Conversation

mourner
Copy link
Member

@mourner mourner commented May 22, 2019

The algorithm we used for approximating round joins with triangles had several issues:

  1. Since the number of slices was linearly based on cosine of half angle between normals (rather than the angle itself), it led to wildly inconsistent approximation — sharper corners were over-approximated while flatter corners were under-approximated. Here, the left turn uses 2 triangles while the right one 6 (!):

image

  1. We used linear (rather than angular) interpolation between normals to generate triangles, which meant that triangles were inconsistent in size — those closer to the join normal were bigger and those closer to prev/next normals smaller:

Fig. 4: uneven slices

  1. Since we always generated a pie slice vertex in the middle, the number of approximating triangles could only be even, which lead to sometimes missing an opportunity for better approximation (e.g. using 2 where 3 would be better or using 6 when 5 would suffice).

These issues combined meant that we 1) generated more triangles than needed overall, 2) had inconsistent quality of approximation.

This PR changes the algorithm to use a more mathematically correct spherical interpolation, solving both issues. To avoid trigonometric calculations in this perf-sensitive section of the code, I adopted polynomial curve fitting approximation demonstrated here. The quality of approximation is now controlled by the constant DEG_PER_TRIANGLE, which is 30 degrees by default — this seems to be an OK value quality-wise, and reduces the number of GPU buffer size on a typical streets z11 view by ~4-5% (#8243). We could definitely tweak this though. Here's how this looks for oversized lines at the moment (3 triangles on the left, 2 on the right):

image

Pending benchmark results, review and a discussion of the constant.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality covered by render tests
  • document any changes to public APIs no changes
  • post benchmark scores
  • manually test the debug page

@mourner mourner requested review from kkaefer and ansis May 22, 2019 14:26
@mourner
Copy link
Member Author

mourner commented May 22, 2019

No noticeable changes in the benchmarks:

image
image
image

@mourner mourner marked this pull request as ready for review May 22, 2019 17:35
Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

@mourner IS there a way to quantify the improvements from this change? Should render tests get updated, or can we show whether amount of buffer data has reduced for a synthetic case?

@mourner
Copy link
Member Author

mourner commented May 29, 2019

@asheemmamoowala render tests should stay the same I think — these changes should be mostly not noticeable by design. I measured the impact locally with a debug page. How would you like to measure this instead?

@mourner
Copy link
Member Author

mourner commented May 30, 2019

cc @mapbox/gl-native — will need a port to native if we land this.

@asheemmamoowala
Copy link
Contributor

How would you like to measure this instead?

The OP for this PR states that:

  1. generated more triangles than needed overall,

I think it would be enough to document the approximate/average reduction in vertices or buffer size as part of the PR, until we have an automated way to measure that.

@mourner
Copy link
Member Author

mourner commented May 31, 2019

I think it would be enough to document the approximate/average reduction in vertices or buffer size as part of the PR, until we have an automated way to measure that.

@asheemmamoowala still confused by what you mean by "document" — there's the following bit in the PR description:

reduces the number of GPU buffer size on a typical streets z11 view by ~4-5%

@asheemmamoowala
Copy link
Contributor

reduces the number of GPU buffer size on a typical streets z11 view by ~4-5%

D'oh! I didnt see that. Sorry for the trouble!

Copy link
Contributor

@ansis ansis left a comment

Choose a reason for hiding this comment

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

This looks good but I'm wondering about the default value for degrees per vertex.

A 30px wide line results in some artifacts that are pretty visible:

Screen Shot 2019-07-17 at 2 25 29 PM

Ideally we could make accuracy depend on the width of the line but I know we don't have access to that right now and getting it would be hard.

Trying to think about other ways we could reduce artifacts without increasing the number of total triangles. Would the total number of geometry vertices in a layer be something we could branch on? If there are a lot of vertices reduce the quality. If there are fewer, increase it?

Do you have a sense of how much switching to 20 or 15 degrees would impact the buffer size?

@mourner mourner force-pushed the improve-round-join-tessellation branch from 0aeafdc to 858e45a Compare July 23, 2019 13:04
@mourner
Copy link
Member Author

mourner commented Jul 23, 2019

@ansis on my local test of streets-v11 with line layers only (less is better):

  • master: 13,296,410
  • 30 degrees: 12,591,730
  • 25 degrees: 12,839,554
  • 20 degrees: 13,119,920
  • 18 degrees: 13,285,592
  • 15 degrees: 13,653,826

So if we want to be close to original precision for the 90 degrees case while not regressing on the buffer size, we could change the value to 20 degrees for now, and look at ways to make that adaptive in a future PR.

Trying to think about other ways we could reduce artifacts without increasing the number of total triangles. Would the total number of geometry vertices in a layer be something we could branch on? If there are a lot of vertices reduce the quality. If there are fewer, increase it?

It might be hard to come up with a good heuristic for basing precision on number of points in a layer, since it depends on the style and data so much, but at least we can assume that if there are a lot of vertices, the lines are unlikely to have long straight segments where approximation differences would be very visible.

Perhaps we could come up with a heuristic based on max distance between points in a particular line?

Anyway, I think it's better to merge the PR without a heuristic and then explore this in a different PR.

@mourner mourner requested a review from ansis July 25, 2019 08:56
Copy link
Contributor

@ansis ansis left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for the measurements. Merge with whichever value you feel is best!

@mourner mourner merged commit 097aad1 into master Jul 30, 2019
@mourner mourner deleted the improve-round-join-tessellation branch August 1, 2019 11:39
alexshalamov added a commit to mapbox/mapbox-gl-native that referenced this pull request Aug 4, 2019
alexshalamov added a commit to mapbox/mapbox-gl-native that referenced this pull request Aug 5, 2019
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.

3 participants