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 pointerX & faceting #1777

Merged
merged 6 commits into from
Aug 10, 2023
Merged

fix pointerX & faceting #1777

merged 6 commits into from
Aug 10, 2023

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Aug 1, 2023

Return the usual euclidian distance to determine the winner across facets, not the squashed distance.

fixes #1776 cc @yurivish

Demo here https://observablehq.com/@observablehq/facet-and-pointerx-1777

I'm not adding that test in the repo, because it's not an interesting chart / use case.

@Fil Fil requested a review from mbostock August 1, 2023 11:08
@Fil Fil enabled auto-merge (squash) August 1, 2023 11:08
@Fil
Copy link
Contributor Author

Fil commented Aug 1, 2023

I've added a unit test for @tophtucker ; see the demo notebook.

@yurivish yurivish mentioned this pull request Aug 2, 2023
@mbostock
Copy link
Member

mbostock commented Aug 4, 2023

I’m afraid I don’t see any effective difference in behavior in the liborProjectionsFacet. I do see this improves the behavior in the case that @yurivish provided. Am I missing something? Can we make the test more obvious and still “interesting” or “valid”?

Screen.Recording.2023-08-04.at.3.24.13.PM.mov

Also, even in the fixed version, I think it’s a little surprising that the tip appears on the facets far below (because the closest point in the current facet exceeds the maxRadius option). I’d prefer that across facets we respect the maxRadius option, too.

src/interactions/pointer.js Outdated Show resolved Hide resolved
@Fil
Copy link
Contributor Author

Fil commented Aug 7, 2023

I agree that it's unnerving when the selection goes in the "wrong" facet, but I'm not sure how to determine that we're in the wrong facet (especially since a value can be outside the facet's frame). Here's a notebook that demonstrates a difficult case:
https://observablehq.com/@observablehq/closest-point-in-facets-1777
untitled (4)

On the left part of the frame, where points are alternatively in the blue and orange facets, the current branch is much better than main (main jumps between top and bottom).

However, the right part of the frame is still a bit unsettling. What should happen if the pointer is at the center of one of the large circles? The top circle belongs to the top facet, so we're tempted to reject the point that has the tip (based on euclidian distance); however, the orange circle belongs to the same facet as that point, so we should accept it (based on the "squished" horizontal distance).

Maybe the correct criterion is “if the pointer is in the frame fi, filter out solutions that belong to other facets, based on euclidian distance”. But we need to keep the solutions that are in the same facet, even if they're too far in euclidian distance. I'll try and implement that.


Regarding the unit test, I reckon it's subtle, and I could substitute a more obvious chart (maybe with a subset of libor, or with the test above). The difference in behavior is visible when I mouse in the following region:
Capture d’écran 2023-08-07 à 12 26 15

Fil added a commit that referenced this pull request Aug 7, 2023
…that are further than the euclidian distance

this solves the weirdness of pointer selection detailed in #1777 (comment)
@Fil
Copy link
Contributor Author

Fil commented Aug 7, 2023

The updated strategy is available by selecting #1777/6cb2d in the demo.

@mbostock
Copy link
Member

mbostock commented Aug 8, 2023

Thanks, that’s a very helpful demo! I want to look at this a little more but I think this is good. 👍

@mbostock
Copy link
Member

I tweaked this logic to be a little simpler (in my opinion): in addition to using the unscaled distance to compare across facets, if the pointer is outside the frame, then the scaling factors kx and ky are ignored.

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.

pointerX can fail with faceting
2 participants