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 barycentric extrapolation #1701

Merged
merged 9 commits into from
Jun 24, 2023
Merged

fix barycentric extrapolation #1701

merged 9 commits into from
Jun 24, 2023

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Jun 18, 2023

closes #1700

Some of the tests look worse, but it's an expected artifact of the delaunay triangulation.

@Fil Fil requested a review from mbostock June 18, 2023 16:55
@Fil Fil force-pushed the fil/fix-barycentric-extrapolation branch from 2201334 to 5436702 Compare June 18, 2023 19:04
@Fil Fil marked this pull request as draft June 18, 2023 22:10
@Fil
Copy link
Contributor Author

Fil commented Jun 18, 2023

Wait… It's still incorrect, since it doesn't check if the point is on the exterior side of the segment.

@Fil Fil removed the request for review from mbostock June 18, 2023 22:11
@Fil
Copy link
Contributor Author

Fil commented Jun 19, 2023

Double-checked and optimized: running the whole hull for each pixel was quite slow.

For a visual description of the “distance to a segment” calculation, see https://observablehq.com/@fil/distance-to-a-segment

@Fil Fil marked this pull request as ready for review June 19, 2023 09:41
@Fil
Copy link
Contributor Author

Fil commented Jun 19, 2023

Investigating a faster algo.

@Fil
Copy link
Contributor Author

Fil commented Jun 19, 2023

The faster algo is not faster (at least not yet), so let's stop here. For reference: https://observablehq.com/@fil/ray-out-of-a-convex-hull. It doesn't scale when the convex hull has tons of points, a situation that happens rarely in “nature”, but does when the points are generated on a circle.

@Fil Fil requested a review from mbostock June 19, 2023 13:51
@Fil
Copy link
Contributor Author

Fil commented Jun 19, 2023

I've now fixed the performance issue in #1705, bringing both algorithms to the same speed (both in theory and in practice). Leaving both PRs open, but I think I prefer the exact algorithm.

src/marks/raster.js Outdated Show resolved Hide resolved
@mbostock
Copy link
Member

The variant in #1705 fixes a few artifacts on the edge, so that looks like a good addition to me. I haven’t grokked the algorithm yet, but I enjoyed the visual explanation of the “distance to segment” and “ray out of a convex hull” notebooks!

Fil and others added 2 commits June 23, 2023 15:29
* An exact algorithm, which doesn't make use of a delaunay search

* repeat const

* fix a floating point precision issue

When points on the hull are collinear, the extrapolation fails in some regions. I've found a way to fix this by reprojecting the points (to make the hull slightly bulge out, which resolves the ties), but in the end it was too much code for a use case that is pretty bad anyway (the delaunay triangulation itself being unstable in that case). This seems to be quite enough.

* test barycentric data binding

---------

Co-authored-by: Mike Bostock <[email protected]>
return W;
};
}

// Extrapolate by finding the closest point on the hull.
function extrapolateBarycentric(W, I, X, Y, V, width, height, hull, index, mix) {
Copy link
Member

Choose a reason for hiding this comment

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

You’re still using I here instead of S (the earlier confusion I mentioned).

Suggested change
function extrapolateBarycentric(W, I, X, Y, V, width, height, hull, index, mix) {
function extrapolateBarycentric(W, S, X, Y, V, width, height, hull, index, mix) {

Copy link
Member

@mbostock mbostock 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 other than the I/S comment.

@Fil Fil merged commit 9246dae into main Jun 24, 2023
@Fil Fil deleted the fil/fix-barycentric-extrapolation branch June 24, 2023 07:06
Fil added a commit that referenced this pull request Aug 21, 2023
Fix barycentric extrapolation

Note: When points on the hull are collinear, the extrapolation fails in some regions. I've found a way to fix this by reprojecting the points (to make the hull slightly bulge out, which resolves the ties), but in the end it was too much code for a use case that is pretty bad anyway (the delaunay triangulation itself being unstable in that case). This seems to be quite enough.


closes #1700

---------
Co-authored-by: Mike Bostock <[email protected]>
chaichontat pushed a commit to chaichontat/plot that referenced this pull request Jan 14, 2024
Fix barycentric extrapolation

Note: When points on the hull are collinear, the extrapolation fails in some regions. I've found a way to fix this by reprojecting the points (to make the hull slightly bulge out, which resolves the ties), but in the end it was too much code for a use case that is pretty bad anyway (the delaunay triangulation itself being unstable in that case). This seems to be quite enough.


closes observablehq#1700

---------
Co-authored-by: Mike Bostock <[email protected]>
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.

barycentric extrapolation should not override interpolation
2 participants