Cursor scale value sync #216

EmiPhil · 2020-05-12T16:11:26Z

Adds a scales array option to the cursor.sync of the signature [xKey, yKey] (default [null, null]) and includes a new demo showcasing functionality.

#215

EmiPhil · 2020-05-12T16:17:01Z

Hmm something funky is happening to the ranger selection period with this code. Selecting a range in the ranger always snaps to the end of the chart. I'll keep looking at it...

EmiPhil · 2020-05-12T20:06:39Z

Okay so now horz/vert scaling works as expected.

Having both the vertical and horizontal ranger synced at the same time would require some more effort. I think that having vertical rangers is pretty niche (and having both is even nicher) though so I'd throw it on the back burner. Basically because the cursors are synced, it moves the horizontal ranger vertically when adjusting the vertical ranger and vice versa. So there would need to be some sort of logic somewhere to prevent that behavior on rangers (maybe an isRanger flag or something to that effect could help).

leeoniya · 2020-05-13T06:25:40Z

thanks @EmiPhil

i'm a bit busy tomorrow but i'll review this in the next day or two. in the meantime, please rebase, squash the commits and exclude .gitignore and all /dist files to reduce merge/review noise (i'll build them myself as a followup).

also the /dist/uPlot.esm.d.ts will need to be updated with the new cursor.sync.scales option:

uPlot/dist/uPlot.esm.d.ts

Lines 238 to 244 in abacd02

    
           /** sync cursor between multiple charts */ 
        
           sync?: { 
        
           	/** sync key must match between all charts in a synced group */ 
        
           	key: string; 
        
           	/** determines if series toggling and focus via cursor is synced across charts */ 
        
           	setSeries?: boolean; // true 
        
           };

EmiPhil · 2020-05-14T15:50:41Z

Should be ready for review. The bit inside of the cacheMouse function makes sense to me (perhaps there is a better way of calling valToPos and posToVal than using self and src? I'm not sure).

The part I'm less confident about is the scaling sync. I haven't played with the code of the library enough yet to even know if putting logic in mouseup function makes the most sense (not to mention if it is missing edge cases etc).

EmiPhil · 2020-05-15T16:23:00Z

I'll loop back to this issue once we finish off #219 because getting the scaling to work will become the next issue otherwise

leeoniya · 2020-05-15T16:29:41Z

agreed. i'd want to land that first since it's more straightforward.

leeoniya · 2020-05-17T02:52:50Z

src/uPlot.js

-			_x = plotWidCss * (_x/_w);
-			_y = plotHgtCss * (_y/_h);
+			// check for value based syncs in the src plot
+			let [xKey, yKey] = src.cursor.sync.scales;


unless there's a reason why these config opts would differ between publisher (src) and subscriber (self), let's just pull this off of the syncOpts.scales.

leeoniya · 2020-05-17T02:58:33Z

src/uPlot.js

+			// check for value based syncs in the src plot
+			let [xKey, yKey] = src.cursor.sync.scales;
+			if (xKey != null)
+				_x = self.valToPos(src.posToVal(_x, xKey), xKey);


we can definitely bypass self.valToPos() since we're not gonna need canvas pixels, let's call the lower-level getXPos() and getYPos() directly. src.posToVal() will probably stay (it's a bit cheaper).

leeoniya · 2020-05-17T03:04:10Z

src/uPlot.js

-								scaleValueAtPos(select[LEFT], xScaleKey),
-								scaleValueAtPos(select[LEFT] + select[WIDTH], xScaleKey),
-							);
+						if (drag.x && !(src != null && !src.cursor.drag[xScaleKey])) {


similar to comment above, unless src and self have different opts, we can just use the local drag[xScaleKey]. also, not loving the double-negatives here.

it looks like src != null is serving the same purpose as e == null at the top of mouseUp() (to check if this is a synced event or a local event). in that case we should keep it uniform. i'd be okay switching to src != null everywhere since it seems a bit clearer.

even better would be to alias to a better name at the top const isSyncReq = src != null;

leeoniya · 2020-05-17T03:07:33Z

src/uPlot.js

-									);
+									// we are changing scale based on a sync request which may not
+									// have the same scale that we do
+									if (src != null && !src.cursor.drag[k])


local drag[k]?

leeoniya · 2020-05-17T03:12:55Z

dist/uPlot.esm.d.ts

@@ -115,6 +115,8 @@ declare class uPlot {
 declare namespace uPlot {
 	export type AlignedData = readonly (number | null)[][];

+	export type CursorScales = [string, string];


please rename to SyncScales

leeoniya · 2020-05-17T03:15:31Z

left some comments. most related to the likelyhood of sync'd plots having different scales opts. i think we can assume they will all match, so we can switch to using local vars instead of stuff off of src.

might have missed something, but that's round 1.

also, please rebase. commit message should be "allow cursor & drag sync using scales instead of coords. close #215."

EmiPhil · 2020-05-17T13:31:57Z

Thanks, I should be able to get to this tomorrow morning.

In terms of behaviour, right now what I was trying to achieve with this pull is that if you specify the scale keys it will use the values of the data at that point (maybe we can call this absolute position or something) and if they are null it uses the % method (maybe we can call this relative position?)

Does that API make sense for you?

EmiPhil · 2020-05-17T13:40:33Z

Oh I should also mention that I think we may need to be able to sync the x and y separately. I don't want to get too into the details in just a comment from my phone right now so I'll write up something more comprehensive about what I mean later.

leeoniya · 2020-05-17T20:28:16Z

Does that API make sense for you?

i'm confused. we had a whole discussion about it in #215, i understand both the intent and API :)

Oh I should also mention that I think we may need to be able to sync the x and y separately.

are they not already synced separately?

leeoniya · 2020-05-18T16:17:09Z

@EmiPhil let me know if you think we can wrap this up in the next couple days since i'd like to cut a new version. otherwise we'll push it to the next release cycle. thanks!

EmiPhil · 2020-05-18T17:54:07Z

I won't guarantee it! I just finished up some work stuff and will be working on this for the rest of the afternoon. As long as there aren't any surprises I hope to finish soon

EmiPhil · 2020-05-19T01:26:34Z

So this is much more cleaned up now.

When I talk about "sync x and y separately" I'm thinking ahead on how to make it work with both rangers at the same time. It's really tough to put into words so I can try to throw together an example at some point. And there's definitely a possibility that what I'm thinking about will be trivial to do! But that is for tomorrow to look at.

leeoniya · 2020-05-19T04:53:06Z

demos/sync-cursor-ranger.html

+							if (vert) {
+								let top = uRanger.valToPos(initYmax, 'y');
+								let height = uRanger.valToPos(initYmin, 'y') - top;
+								let width = uRanger.root.querySelector(".over").getBoundingClientRect().width;


uRanger.bbox.width / devicePixelRatio should give you the same (and more elegant)

leeoniya · 2020-05-19T04:54:04Z

demos/sync-cursor-ranger.html

+							} else {
+								let left = Math.round(uRanger.valToPos(initXmin, 'x'));
+								let width = Math.round(uRanger.valToPos(initXmax, 'x')) - left;
+								let height = uRanger.root.querySelector(".over").getBoundingClientRect().height;


can pull this from uRanger.bbox, too

leeoniya · 2020-05-19T05:08:56Z

src/uPlot.js

@@ -1485,14 +1485,18 @@ export default function uPlot(opts, data, then) {
 		return sc.min + pct * d;
 	}

+	function yScaleValueAtPos(pos, scale) {
+		return scaleValueAtPos(plotHgtCss - pos, scale);


since scaleValueAtPos() already contains an internal check for xScaleKey to auto-set the dim. why not just move the automatic pos inversion into there as well, instead of making a dedicated yScaleValueAtPos().

leeoniya · 2020-05-19T05:29:58Z

I'm thinking ahead on how to make it work with both rangers at the same time.

oh, i see. this might indeed be tricky, not to mention ultra-niche :D. i'm already somewhat hesitant to include the y-ranging demo for the same reason - i've never seen one or felt like i needed one. having the y ranger also makes the demo more complicated by adding an opts generator that uses vert conditionally :\

i'd like to leave any further sync complication out of the core. even if it's not a lot more code, every additional nuance makes maintenance more difficult, and the returns here are way past diminishing here, IMO.

EmiPhil · 2020-05-19T15:26:44Z

Fair enough. If you don't want it in the core that makes this issue easier! If we take out the y axis example should we just put this example code into the ranger example that already exists?

EmiPhil · 2020-05-19T15:30:40Z

Oh one thing that may not be as niche is wanting an x ranger but still wanting to be able to zoom on the y axis on the main chart. I'll play with that to see how it behaves (that's going to be the behavior I'm looking for, I agree the y ranger is weird, though it does really help to make sure the code is working!)

leeoniya · 2020-05-19T15:53:12Z

If we take out the y axis example should we just put this example code into the ranger example that already exists?

yeah i think that makes sense. i feel like the default behavior of sync should maybe change to [xScaleKey, null]. syncing x by position would still be useful for things like comparing different time periods across multiple charts, but that would be the less used case.

leeoniya · 2020-05-26T19:01:55Z

src/uPlot.js


-			if (select.show && (drag.x || drag.y))
+			if (select.show && (dragX || dragY))


during mousedown, these will always match drag.x and drag.y, right?

if that's the case, we can leave this line unchanged and actually pull the dynamic dragX and dragY variables into the local else scope (line 1649), since they will no longer be needed anywhere else.

leeoniya · 2020-05-26T19:02:29Z

(I think that as soon as you are zooming x and y it doesn't make sense after a drag to auto zoom the axis, you have put that responsibility in the hands of the user doing the dragging).

yeah, for sure.

EmiPhil · 2020-05-26T19:10:04Z

Yep and moved

leeoniya · 2020-05-26T19:10:21Z

actually, relying on only drag.x and drag.y breaks autoscaling in the dygraphs-style zooming :(, (see zoom variations). also single-clicking in that demo does weird things.

EmiPhil · 2020-05-26T19:11:55Z

On branch 6a8 or b56? 6a8 is garbage I pushed it too eagerly

leeoniya · 2020-05-26T19:20:18Z

i just pulled b56 and zoom variations has the mentioned issues. i don't see how we can really get away from relying on the dynamic/shared dragX and dragY because the autoscale behavior will depend on what actually happened rather than the drag.x and drag.y opts - those should just serve to manage the behavior during dragging. we can just reset dragX and dragY to the original opts in mouseup or something.

EmiPhil · 2020-05-26T19:37:21Z

Okay I got the clicking thing fixed, but I need some clarification for the expected behavior of the dygraphs one. We want it to act completely like the just x and y ones and so auto scale the other axis after zoom?

leeoniya · 2020-05-26T19:39:47Z

We want it to act completely like the just x and y ones and so auto scale the other axis after zoom?

well, there's not really a way to auto-scale x, but for y, yes.

and not just the dygraphs one, but any time there is unidirectional dragging, which includes x-only and your omni/uni mixed-case variant as well (when below the uni threshold).

EmiPhil · 2020-05-26T19:50:41Z

Okay yeah. And I think the limitation on auto scaling has more to do with sync than with the x / y.

EmiPhil · 2020-05-26T19:55:34Z

This all seems to work well enough? Behavior for non sync charts behaves without limitations, but if we are syncing then we just use the drag.x and drag.y values to prevent some of the sync charts from auto scaling while others don't.

EmiPhil · 2020-05-26T20:03:19Z

Most recent push brings back the setSelect function that should have never been nuked

leeoniya · 2020-05-26T21:16:40Z

This all seems to work well enough?

there's definitely some oddities that happen with syncing exactly to match the src selection during unidirectional dragging. in the sync-cursor demo, if you drag along x in the smaller chart that is auto-scaled to have a min of 10%, you'll see that 10% min reflected in the main chart and will frequently end up with an empty main chart after zooming - but you only dragged x!

perhaps we do need to pub dragX and dragY to all subscribers so they know if they need to match an explicit x and/or y drag.

also, timezones-dst demo seems to have lost the sync'd setSelect?

EmiPhil · 2020-05-26T21:20:21Z

For the timezone-dst I think that should be fixed by d6c7304. I accidentally deleted the setSelect hook cause I thought it was leftovers.

For the second one, it is odd but myabe not wrong? Like if you think about it they are supposed to be synced together but at render they are out of sync. So when you zoom the x of the zoomed in chart you are effectively zooming x/y on the non synced charts.

EmiPhil · 2020-05-26T21:22:24Z

I guess my thought is that if you are explicitly syncing the x/y key of two charts, you would expect the two scales to reflect each other. I think in this sync cursor demo that isn't the intent - it should really only sync the x. (Especially considering the units aren't event the same across charts)

EmiPhil · 2020-05-26T21:29:40Z

And yeah taking out the "y" key and the uni/y drag in the cursor opts in that demo behaves more how you would want this type of chart to behave. (What does it even mean to sync y across different units? This is why we left the ability to have y be relative imo)

EmiPhil · 2020-05-26T21:32:16Z

12d0ddd includes an updated cursor-sync demo to reflect the changes I just mentioned

leeoniya · 2020-05-26T21:32:51Z

yes you're right, i didn't actually mean for my original branch to permanently include those sync-cursor changes [1] in the demo, it was more just playing around. i'll give this a final review later today.

[1] e3a877e#diff-6fc4e995c333753aaa6e5d2c73a8660e

leeoniya · 2020-05-26T21:35:01Z

since scales: ["x"] is the default behavior now, you can probably skip any changes to sync-cursor.

EmiPhil · 2020-05-26T21:36:37Z

👍

And the change in 6d0ee4a is to fix a visual issue where if you are only syncing one or the other axis it wasn't always showing the whole select on the other chart.

…ya#215

leeoniya · 2020-05-27T00:55:27Z

5f265d1 was a winner :)

outstanding work! i will give you 20% of profits from uPlot. unfortunately, 20% of $0 is still $0 🤣

i'm gonna do some followups like merging the rangers into one demo and other minor stuff.

EmiPhil force-pushed the cursor-scale-value-sync branch 2 times, most recently from f8e8b57 to 2aa0b9d Compare May 14, 2020 15:43

EmiPhil mentioned this pull request May 16, 2020

timeline plots #188

Closed

leeoniya mentioned this pull request May 16, 2020

Highlight weekends / interval example #220

Open

leeoniya reviewed May 17, 2020

View reviewed changes

EmiPhil force-pushed the cursor-scale-value-sync branch from 2aa0b9d to 2277fe1 Compare May 19, 2020 01:22

leeoniya reviewed May 19, 2020

View reviewed changes

leeoniya reviewed May 26, 2020

View reviewed changes

EmiPhil force-pushed the cursor-scale-value-sync branch 2 times, most recently from 6a8f367 to b564b1e Compare May 26, 2020 19:08

EmiPhil force-pushed the cursor-scale-value-sync branch from b564b1e to 0b9b87a Compare May 26, 2020 19:51

EmiPhil force-pushed the cursor-scale-value-sync branch from 0b9b87a to d6c7304 Compare May 26, 2020 20:02

EmiPhil force-pushed the cursor-scale-value-sync branch from d6c7304 to 6d0ee4a Compare May 26, 2020 21:17

EmiPhil force-pushed the cursor-scale-value-sync branch from 6d0ee4a to 12d0ddd Compare May 26, 2020 21:31

allow cursor & drag sync using scales instead of coords. close leeoni…

5f265d1

…ya#215

EmiPhil force-pushed the cursor-scale-value-sync branch from 12d0ddd to 5f265d1 Compare May 26, 2020 21:40

leeoniya merged commit 5f265d1 into leeoniya:master May 27, 2020

leeoniya mentioned this pull request May 29, 2020

Expose dragging boolean #246

Closed


		if (select.show && (drag.x \|\| drag.y))
		if (select.show && (dragX \|\| dragY))

Cursor scale value sync #216

Cursor scale value sync #216

Conversation

EmiPhil commented May 12, 2020

EmiPhil commented May 12, 2020

EmiPhil commented May 12, 2020

leeoniya commented May 13, 2020 • edited Loading

EmiPhil commented May 14, 2020

EmiPhil commented May 15, 2020

leeoniya commented May 15, 2020

leeoniya May 17, 2020 • edited Loading

Choose a reason for hiding this comment

leeoniya May 17, 2020

Choose a reason for hiding this comment

leeoniya May 17, 2020

Choose a reason for hiding this comment

leeoniya May 17, 2020 • edited Loading

Choose a reason for hiding this comment

leeoniya May 17, 2020

Choose a reason for hiding this comment

leeoniya May 17, 2020

Choose a reason for hiding this comment

leeoniya commented May 17, 2020

EmiPhil commented May 17, 2020

EmiPhil commented May 17, 2020

leeoniya commented May 17, 2020

leeoniya commented May 18, 2020

EmiPhil commented May 18, 2020

EmiPhil commented May 19, 2020

leeoniya May 19, 2020 • edited Loading

Choose a reason for hiding this comment

leeoniya May 19, 2020 • edited Loading

Choose a reason for hiding this comment

leeoniya May 19, 2020

Choose a reason for hiding this comment

leeoniya commented May 19, 2020

EmiPhil commented May 19, 2020

EmiPhil commented May 19, 2020

leeoniya commented May 19, 2020 • edited Loading

leeoniya May 26, 2020

Choose a reason for hiding this comment

leeoniya commented May 26, 2020

EmiPhil commented May 26, 2020

leeoniya commented May 26, 2020 • edited Loading

EmiPhil commented May 26, 2020 • edited Loading

leeoniya commented May 26, 2020

EmiPhil commented May 26, 2020 • edited Loading

leeoniya commented May 26, 2020 • edited Loading

EmiPhil commented May 26, 2020

EmiPhil commented May 26, 2020

EmiPhil commented May 26, 2020

leeoniya commented May 26, 2020

EmiPhil commented May 26, 2020

EmiPhil commented May 26, 2020 • edited Loading

EmiPhil commented May 26, 2020

EmiPhil commented May 26, 2020

leeoniya commented May 26, 2020

leeoniya commented May 26, 2020 • edited Loading

EmiPhil commented May 26, 2020

leeoniya commented May 27, 2020

leeoniya commented May 13, 2020 •

edited

Loading

leeoniya May 17, 2020 •

edited

Loading

leeoniya May 17, 2020 •

edited

Loading

leeoniya May 19, 2020 •

edited

Loading

leeoniya May 19, 2020 •

edited

Loading

leeoniya commented May 19, 2020 •

edited

Loading

leeoniya commented May 26, 2020 •

edited

Loading

EmiPhil commented May 26, 2020 •

edited

Loading

EmiPhil commented May 26, 2020 •

edited

Loading

leeoniya commented May 26, 2020 •

edited

Loading

EmiPhil commented May 26, 2020 •

edited

Loading

leeoniya commented May 26, 2020 •

edited

Loading