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

Improved hover detection for scatter plot fill tonext* #6865

Merged
merged 6 commits into from
Jan 30, 2024

Conversation

lumip
Copy link
Contributor

@lumip lumip commented Jan 23, 2024

Hover detection (and hover label placement) does not work correctly on scatter plot fills when using tonextx, tonexty, as demonstrated in #2165 . This is due to faulty construction of the hover detection polygons in these cases.

This PR contributes the following commits to address the issue:

  • add tests for hover detection for self, tonext, tonexty and tozery based on test mocks scatter_fill_corner_cases and scatter_fill_self_next, paying special attention to corner cases where current hover detection fails to detect the correct trace ( 4628741 )
  • fix the faulty polygon construction that caused the misdetections reported in hoveron: fills not getting the right areas #2165 in src/traces/scatter/plot.js. However, this still results in some misdetections, especially for traces with curved edges or "jagged" shapes (e.g. shape='vh'), where the detection polygon construction will not follow the visual edge. ( be658dd )
  • reimplement hover detection using the isPointInFill method of the SVGElement used to render the fill. This result in always correct detection of the hovered over trace. However, the SVGElement does not provide an easy means to compute hover label placement, so the detection polygons are still used for that. Close to curved edges, where polygons are not accurate, this can cause labels to end up in weird positions. ( 3eb4738 )
  • change the label fallback placement (when polygon-based placement fails, see above) to be at the cursor instead, which to me seems more sensible and simplified the code. ( 63d181a )

This is currently still not a perfect solution but I think this has reached a reasonable state now. I'm not sure whether the change in the last commit is desired but it seemed the more intuitive fallback to me after reviewing the changes of the third commit - please advise.

Further improvements for this would likely require either fundamental improvement of how detection polygons work to accurately model edges that are not simply straight lines between trace points, or a way to use the SVGElement to compute hover label positioning. I tried some approaches for the latter, but ultimately did not come up with a satisfying solution. I believe the current state is acceptable for the time being and a noticeable improvement over the current state.

Reminder:
After opening a pull request, developer:

  • should create a new small markdown log file using the PR number e.g. 1010_fix.md or 1010_add.md inside draftlogs folder as described in this README, commit it and push.
  • should not force push (i.e. git push -f) to remote branches associated with opened pull requests. Force pushes make it hard for maintainers to keep track of updates. Therefore, if required, please fetch upstream/master and "merge" with master instead of "rebase".

The tests rely on scatter mocks `scatter_fill_self_next`
and `scatter_fill_corner_cases` and test for a number of points
that are currently not correctly detected when hovered over
for tonexty and tozeroy fills.
Previous code would not correctly construct the polygons
used for detection whether the cursor is within the fill
region of a trace.

Some of the previously created tests fail with this as the
hover points in question cannot currently be correctly
detected using the detection polygons.
…ests in scatter plots.

However, SVGElement does not allow for an easy way to determine the positioning of
the hover label, so the polygons are still in use for that.
For curved edges of fills there is a chance that the hover detection polygons miss a point and the correct hover label position cannot
be determined. Previous code fell back to positioning the label
at the largest edge of any polygon of the trace of their combined
vertical midpoint, with undetermined behaviour if no polygon
overlapped the midpoint.
This change instead falls back to simply displaying the label at the
current cursor position, which simplifies code and results in less
confusing label placement.
@archmoj archmoj added bug something broken status: reviewable community community contribution labels Jan 23, 2024
@lumip
Copy link
Contributor Author

lumip commented Jan 23, 2024

By the looks of it, this will likely also address #2399 of the plotly.py repo.

@lumip lumip changed the title Improved hover detection for for scatter plot fill tonext* Improved hover detection for scatter plot fill tonext* Jan 24, 2024
draftlogs/6865_fix.md Outdated Show resolved Hide resolved
@archmoj
Copy link
Contributor

archmoj commented Jan 26, 2024

Looking excellent to me. 🎖️ 🏅 🥇
Over to @alexcjohnson

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 Looks great to me! tested by loading the line-shape-arrow mock and calling:

Plotly.restyle(gd,{fill:'tonexty',hoveron:'points+fills'})

On this branch it looks wayyy better than on master. Every once in a while I see the hover label jump around despite me not changing which shape I'm on top of, but it's still always labeling the correct object AFAICT. Very nicely done @lumip!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken community community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants