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 nearest interaction mode #5857

Merged
merged 1 commit into from
Nov 27, 2018
Merged

Fix nearest interaction mode #5857

merged 1 commit into from
Nov 27, 2018

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented Nov 23, 2018

This is the mode I always seem to need, so thought it could be useful to others too.

Did not use much time with the function itself, just combined nearest and x - so it can probably be improved.

Fixes #3615
Fixes #5371

@simonbrunel
Copy link
Member

What's the difference between {mode: 'nearest-x'} (your PR) and {mode: 'nearest', axis: 'x'}?

@kurkle
Copy link
Member Author

kurkle commented Nov 23, 2018

@simonbrunel
Copy link
Member

I don't think we should introduce a new mode to achieve this use case because it's a duplicate of {mode: 'nearest', axis: 'x'} but that actually works. Our current nearest implementation returns only the first matching point, which I think is wrong: it should return all points and in this case it would work as nearest-x. We added the axis option exactly for that purpose, to avoid duplicating modes (foo, foo-x, foo-y, see #4458).

Changing (fixing?) our nearest method would of course change the current behavior but since nearest with axis: 'x' or axis: 'y' doesn't really work (bug fix) and axis: 'xy' (default) most of the time returns only 1 point, I don't think it's a breaking change. If returning multiple points for axis: xy is an issue, we should instead expose a new option to limit the result (something like single: true/false or limit: Number or ...).

@etimberg I think you are the most familiar with that code, what do you think?

@etimberg
Copy link
Member

If I recall correctly, {mode: 'nearest', axis: 'x'} was added explicitly to return the first item. A lot of users found the prior behaviour of getting all things that intersect annoying. We had a number of bugs (I don't recall issue numbers at the moment) regarding that.

I don't think we should change the behaviour of the current API. If anything, I would support a new key multiple: <boolean> that defaults to false. If set to true, all intersecting items are returned.

@simonbrunel
Copy link
Member

simonbrunel commented Nov 23, 2018

I don't think we added axis: x|y to fix the nearest behavior but the index one (for horizontal bars). {mode: 'nearest', axis: 'x'} behaves very weird, it doesn't return the nearest item but the first at the nearest x (jsfiddle). It's actually very annoying (bug?):

5857

I really don't know when {mode: 'nearest', axis: 'xy'} would return multiple items except when there are exactly at the same position, in which case we currently pick the "biggest" point or the one at the lowest dataset (#3400). That "works" for axis: 'xy' but not for axis: 'x'|'y'. It's also arbitrary, let's say a user wants the smallest item?! It's also weird when the point radius changes when hovering:

5857-2

I don't think that's a consistent behavior, to me nearest should return all items. I'm also convinced that some users would like to display all values at the same position:

image

The picking logic really depends of the use case, even a multiple or single option is not ideal. For now, maybe we could simple check the axis logic in nearest and if x or y, returns all the items:

return axis === 'xy' ? nearestItems.slice(0, 1) : nearestItems;

Anyway, I would not expose a new nearest-x mode and duplicate code.

@kurkle
Copy link
Member Author

kurkle commented Nov 23, 2018

with 'multiple' option: (also fixed that very annoying (bug?))
https://codepen.io/kurkle/full/dQeMRd/

@simonbrunel
Copy link
Member

@etimberg What I don't like with the multiple approach (default: 'false') is that it should work with all modes to have a consistent API but that's not possible with modes that currently return multiple items.

@simonbrunel
Copy link
Member

@kurkle how the {mode: 'nearest', axis: 'x', multiple: false} works in your fiddle?

@kurkle
Copy link
Member Author

kurkle commented Nov 23, 2018

@kurkle how the {mode: 'nearest', axis: 'x', multiple: false} works in your fiddle?

var distanceMetric = getDistanceMetricForAxis('xy'); //always consider both axis when finding nearest

@etimberg
Copy link
Member

@simonbrunel good point. Thanks for making those gifs. I'm fine to return all in the axis x or y case but that might be considered a breaking API change for existing users (it's also not great to change the return type but we've done it elsewhere before)

@kurkle
Copy link
Member Author

kurkle commented Nov 23, 2018

Removed multiple, axis: 'xy' returns only single item (like before).
Updated pen: https://codepen.io/kurkle/full/dQeMRd/

@simonbrunel
Copy link
Member

@kurkle In your fiddle, {mode: 'nearest', axis: 'x', intersect: true} doesn't work because it should return mouse intersected items only (same for axis: 'y', intersect: true).

Instead, what do you think of this change, does it fixes your issue and works as expected?

// ...
var distanceMetric = getDistanceMetricForAxis(option.axis);
var nearestItems = getNearestItems(chart, position, options.intersect, distanceMetric);

if (axis !== 'xy') {
    return nearestItems;
}

// ... else previous picking code 
// ...

return nearestItems.slice(0, 1);

@etimberg the return type is the same (array) and currently the 'x' | 'y' behavior are not great, so I would consider this change as a bug fix :) If users want the current behavior of axis: 'x', they should go for axis: 'xy' instead.

@kurkle kurkle changed the title Add nearest-x interaction mode. Like index, but works with missing points. Add nearest with axis:x/y interaction mode. Like index, but works with missing points. Nov 23, 2018
@kurkle
Copy link
Member Author

kurkle commented Nov 23, 2018

@simonbrunel In other words, axis has no effect whatsoever with intersect: true.
shouldn't {mode: 'nearest', axis: 'x', intersect: false} also return only the nearest item instead by that same logic?

As a side note, sorting by area seems to fail when hover also occurs (point is enlarged).

@simonbrunel
Copy link
Member

simonbrunel commented Nov 23, 2018

In other words, axis has no effect whatsoever with intersect: true.

Not exactly. For example, you could have two points with large radius, under the mouse cursor, at the same x but different y. I would return both in case of axis: 'x' but only the closest if axis: 'xy'.

shouldn't {mode: 'nearest', axis: 'x', intersect: false} also return only the nearest item instead by that same logic?

I'm not sure, from my point of view, the axis: 'xy' behavior is incorrect and should return all points at the same x and y (but we don't want to change it to avoid breaking existing projects). Since the axis: 'x' and axis: 'y' are currently useless / bugged, I would go with the solution I suggested in a first time (if it fixes your issue of course).

Later, if requested, we can introduce a new option to limit the lookup to 1 item (the closest one) and make it work for all modes currently returning multiple items. I would not implement this feature in each mode, but in the calling code: interactions modes would return all items, sorted by distance to the mouse cursor, then we would select the first one (closest) if single: true (or whatever option we decide). (may not work as expected since it would change order of items in the tooltip)

@kurkle
Copy link
Member Author

kurkle commented Nov 24, 2018

created some more "tests"
using this PR: https://codepen.io/kurkle/full/rQvYqr/ <-- yesterdays version
using release 2.7.3: https://codepen.io/kurkle/full/YRLERz/
neither works very well.
your suggestion: https://codepen.io/kurkle/full/LXmOaK/ <-- updated to b9ce414
thats a clear winner.

update

In update b9ce414 sorting flicker re-appears (previous version always returned all nearestItems, not a slice after sort)

So the sorting problem remains when compared items are of same size and hover enlarges the one chosen by dataset index. Now its larger and the smaller one gets chosen on next iteration (mouse move).. and so on.

@kurkle
Copy link
Member Author

kurkle commented Nov 24, 2018

This is only somewhat related, but is there a use case where hover and tooltip modes should be different?
I'm suggesting that by default, those modes should be equal and use same configuration if not overridden.

This could be for example options.interaction. options.hover / options.tooltips would default to that for {mode, axis, intersect} and be overwritable separately if need be.

@simonbrunel
Copy link
Member

simonbrunel commented Nov 24, 2018

Thats a clear winner

Can you push these changes so we can review it. If we agree, docs and unit tests will need to be updated.

I'm suggesting that by default, those modes should be equal and use same configuration if not overridden.

That would have been great but I think it will break external code relying on the presence of tooltips/hover: { mode, axis, intersect } because they would not fallback to the new set of options. So I would keep that idea for the next major release (can you open a new ticket?).

@kurkle
Copy link
Member Author

kurkle commented Nov 24, 2018

Can you push these changes so we can review it. If we agree, docs and unit tests will need to be updated.

done. again.

@simonbrunel
Copy link
Member

Thank you @kurkle (and thanks for the fiddles, very helpful)

@etimberg can you experiment with these fiddles and let us know what you think of the latest changes?

@simonbrunel
Copy link
Member

Linking the following issues:

@benmccann
Copy link
Contributor

The blinking in 2.7.3 is pretty annoying, so would be fine with merging this

@etimberg
Copy link
Member

I tested https://codepen.io/kurkle/full/LXmOaK/ and I like the axis X and Y behaviours. There's still some flickering on the axis: xy tests at the rightmost point. Is it worth changing nearest XY to also return everything?

@kurkle
Copy link
Member Author

kurkle commented Nov 25, 2018

two more pens to consider
nearest returning all always
flickering fixed by ignoring hover in area calculation

hover ignoring code (true added as parameter when calling from sorting function):

getArea: function(ignoreHover) {
	ignoreHover = ignoreHover || false;
	var radius = ignoreHover && this.$previousStyle ? this.$previousStyle.radius : this._view.radius;
	return Math.PI * Math.pow(radius, 2);
},

@simonbrunel
Copy link
Member

Is it worth changing nearest XY to also return everything?

@etimberg @kurkle nearest + xy currently implements a specific use case, which I think is wrong. I would go for the first solution (always return all items) which also fixes #5371 and is more consistent with the x and y implementation. Then I would wait until someone report an issue before implementing an option to limit the number of items, though they can already do it using tooltips.filter.

Your thoughts?

@simonbrunel simonbrunel added this to the Version 2.8 milestone Nov 25, 2018
@kurkle
Copy link
Member Author

kurkle commented Nov 25, 2018

Your thoughts?

I agree. Saves couple bytes too.
Documentation does not agree, it needs to be updated.

Sorting those items could be useful, but that's another story again.

@etimberg
Copy link
Member

@simonbrunel I'm onboard with that solution

@kurkle kurkle changed the title Add nearest with axis:x/y interaction mode. Like index, but works with missing points. Fix nearest interaction mode Nov 26, 2018
@simonbrunel
Copy link
Member

@kurkle do you think you can add unit tests for nearest + axis: 'x' and nearest + axis: 'y'?

@kurkle
Copy link
Member Author

kurkle commented Nov 26, 2018

@kurkle do you think you can add unit tests for nearest + axis: 'x' and nearest + axis: 'y'?

Sure. But there's a thing.
intersect=true, should return the smallest item if more than 1 are at the same distance test does not fail currently. I guess it has to do with modifying chart after it was acquired in beforeEach

	chart.data.datasets[0].pointRadius = [5, 5, 5];
	chart.data.datasets[0].data[1] = 40;

	chart.data.datasets[1].pointRadius = [10, 10, 10];

Whats the proper way of fixing that?
adding chart.update();

Return all items that are at the nearest distance to the point.
Add unit tests for nearest + axis: 'x' and nearest + axis: 'y'
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

Thanks @kurkle

@simonbrunel simonbrunel merged commit f5437fe into chartjs:master Nov 27, 2018
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Return all items that are at the nearest distance to the point and add unit tests for nearest + axis: 'x' and nearest + axis: 'y'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants