Cursor scale value sync

[EmiPhil]

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]

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]

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]

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:

https://github.com/leeoniya/uPlot/blob/abacd02d19403048dbfd30b2004238e203a25598/dist/uPlot.esm.d.ts#L238-L244

[EmiPhil]

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]

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]

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

[leeoniya]

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]

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]

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]

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]

@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]

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]

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]

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]

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]

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]

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.

[EmiPhil]

Okay will do. The latest push fixes up your last commend in uPlot.js and makes clicking behave a bit smarter.

[EmiPhil]

Latest push adds an early return to cacheMouse if _x or _y are negative (it wasn't properly getting rid of the cursor lines on the other chart without it).

Also removes the x/y example and adds sync cursor to the ranger example (which lets us get rid of some of the plugin logic).

PS I would have liked to test the sync-cursor.html demo on my local comp but neither firefox or chrome allow me to load the data from the json file.

[EmiPhil]

Hmm to get double clicking the main chart to work as expected I called hideSelect on synced charts that are still showing the select box. Actually I think the behavior would be opposite, on double clicking the main chart if the setScales drag property is false on sync charts set the ranger to max. I'll work on that.

[EmiPhil]

Okay now we check for drag.setScale. If true we clear, if false we expand. This should be ready for (final?) review

[leeoniya]

initial testing: /demos/sync-cursor.html no longer zooms properly. also, /demos/timezones-dst.html.

the ranger demo simplification is 💯. we can now drop the unused hack vars 🎉 :

let viaRanger = false;
let viaZoom = false;

[EmiPhil]

In the dst example (probably the same error as sync-cursor) the problem is that when self needs to set its scales it hides its select and then publishes the change (so now the sync charts receiving the request get a src where select has been cleared).

Easy fix is to publish first, do work after, but in most of the functions the workflow is work first publish after. I'll push the easy fix + html changes soon and you can comment

[leeoniya]

hmm, mouseup is not really perf-sensitive, and it's quite odd if subscribers update before the publisher. i'd feel better if we cached the values temporarily and published with those, but still at the end.

[EmiPhil]

Okay so now we store the select in a const at the start and reassign it at the end if we pub

[leeoniya]

nice. looking a lot better.

i'd like to avoid drag.setScale affecting setSelect behavior, even if it means having to handle selection hiding in the demo via hooks. keeping them separate means that there's no magic interaction you need to account for when using the setSelect and setScale hooks.

actually, never mind. this makes sense.

[leeoniya]

i'm still not seeing proper behavior in the other demos, though. zooming does not match scales ranges in synced charts.

partial solution is to do a deep copy here: const prevSelect = copy(select);, but the scales are still somewhat mis-matched when doing this.

[EmiPhil]

Huh. This is the weirdest thing. Console logging before and after:

left (before) = 104 left (after) = 104.00000000003473

I'll figure it out soon hopefully...

[EmiPhil]

Oh I found the problem :facepalm:

[EmiPhil]

It was insufficient to cache the select position because posToVal returns a value based on the current scale which changes if drag.setScale is true (which is why pub before work was working).

So, we have to cache the calculated values for all of the min and maxes before we set the scales. Then we can use those precalculated values directly if the update comes via src.

[leeoniya]

thanks @EmiPhil

getting this working properly is turning out to be quite a doozy, but i've made some more progress. your latest version unfortunately needs a lot of hacks to work, so here's what i did. i started with your previous version and re-worked the mouseUp's syncRequest to work more as a "push" rather than a "pull". this way, we simply compute the scales for the selection at the source and add them as an additional param to pub(). this avoids all the caching nonsense and becomes more straighforward.

take a look at this branch: https://github.com/leeoniya/uPlot/tree/cursor-scale-value-sync

it appears to be working for x, though the x + y seems to be buggy on double-click in the tweaked cursor-sync demo. this demo also doesnt technically have multiple (or differing) y scales in the charts, so that might be good to test. maybe you can resurrect your y-ranger and test it out. i'm off to get some sleep!

thanks!

[EmiPhil]

Oh yay! The cache thing felt very hacky. I'll look at the code soon

[EmiPhil]

For future reference I was finally able to get the sync-cursor example working by using the web server for chrome extension which mounts the local filesystem. Without it all browsers were blocking the demos from loading json data via file:/// urls

[leeoniya]

i usually just execute http-server in the repo dir: https://www.npmjs.com/package/http-server

[leeoniya]

another thought i had is:

why do we need to do anything in mouseup at all if the cursor & selection sync already accounts for scale keys?

i think the issue is that cursor & select coords are clamped. if we could opt out of clamping when isSyncReq (or always), then we could probably remove all mouseUp changes (they still feel a bit like a hack). the cursor & select would still be visually clipped by the parent container (.over).

this might be another route to explore and i think is the actual solution here, honestly.

[EmiPhil]

I'll look into that

[EmiPhil]

Yeah so this api looks a lot better for sure!

I need to play more with the sync cursor example but I think this api fixes it. If you set the y key to null then it uses the relative position for all of them (which I think makes sense) but if you set the "y" key it behaves weird in a way that I think may be correct. Since two of the scales are using % they map correctly and the MB one maps weird but it's a bit like...of course it maps weird, those aren't the same numbers. I think.

I have a meeting so I'll loop back to this in about 1-2 hrs

[EmiPhil]

Okay!

I would recommend you play with the demos in this order:

sync-cursor

1. As is. MB is mapped to % because they are both on the y scale key.
2. Set the "y" scale sync option to null. Now they are mapped by relative position.
3. Optionally turn uni off, it works as expected.

zoom-fetch

1. As is. This is like the kitchen sink. The selection box is properly scaled between the charts and ticks at the uni points at the same time on both. This was also true for the difference sized charts in sync-cursor but it is a lot more noticeable here. The selection box of the ranger is also shows the full zoom of the main chart. I made it purple so you could really see what is going on with it.

I'm aware that most rangers don't do that and I had some ideas for only doing it when you are trying to sync x and y, but the more I played with it the more it just felt right to do it that way. Like if you don't do it that way and you sync the cursor to y then you get this (to me) odd behavior where the select box of the ranger is taller than you can ever get the cursor.

For the behavior most people are used to with a time ranger you need to set the ranger opts to:

                cursor: {
                    y: false,
                    points: {
                        show: false
                    },
                    drag: {
                        setScale: false,
                        x: true
                    },
                    sync: {
                        key: "moo",
                        scales: ["x"]
                    }
                }

and the main opts to:

                cursor: {
                    drag: {
                        x: true
                    },
                    sync: {
                        key: "moo",
                        scales: ["x"]
                    }
                }

If you keep the ability to drag y on the main chart without it on the ranger you get this odd behavior where moving left and right shows on the ranger even if you are only zooming y so there is some work to do there.

Ps I'm not gonna bother to rebase & squash the pushes until we are settled cause it's a bit of a pita.

[EmiPhil]

If you keep the ability to drag y on the main chart without it on the ranger you get this odd behavior where moving left and right shows on the ranger even if you are only zooming y so there is some work to do there.

I mean "odd" is maybe the wrong word because the ranger is meant to zoom x. The only way it would know not to is for it to see the src chart. But since updateCursor doesn't happen through the sync mechanism it would require another variable for the src drag values. I think that if you are wanting to zoom y on the main chart the recommended opts should probably be to go with the method where it zooms the ranger too.

[EmiPhil]

The ranger with everything on reminds be a bit of the mini maps that are in programs like photoshop actually.

I also noticed that single clicking on the sync-cursor demo breaks everything so I'm not too sure what is going on there. Edit: it definitely has something to do with cursor: lock. I have 0 experience with that particular feature though

Edit 2: Looks like the fastest way to get odd behavior may be (?) locking the cursor while your cursor is out of range of the other charts. For ex when you scale the MB to the % and you lock it at 60MB when the % charts are only going to 25/30. And then if you click around a bunch it seems like they kind of get out of sync on who is currently locked or not, so some charts are locked while others aren't (which may actually be the desired state? Maybe we should only lock if !isSyncReq)

[leeoniya]

the locking function is meant to lock the cursor across synced charts. for example to evaluate some legend values across multiple charts (perhaps some scrolled off-screen), or maybe take a screenshot with the cursor positioned at a specific location. the function is click to lock, click again to unlock. these clicks can happen on any of the synced charts with the same key and must lock/unlock all charts together.

i'll need some time to play around and review the changes here. cacheMouse is very perf-sensitive since it's the unthrottled mousemove listener and can fire thousands of times per second. any work that can be deferred in it, must be. i'll need to test how much of a perf impact all the extra code has. some stuff might need to move into updateCursor() (which is throttled to 60/s).

[EmiPhil]

Would it be possible/reasonable to use the src/sync mechanics in updateCursor?

[EmiPhil]

Actually I could put some of the logic into the mouseMove function in another raf or set the raf up to call two functions, the caching functionality and the updateCursor functionality.

I think that the scale functions are definitely a requirement, not so much as part of this pr but more an oversight from #221 that became obvious during the development of this one. I think it makes sense that if you have two charts in sync you would want them both to be using the same x/y zoom at the same time regardless of the uni breakpoints.

I'm not a performance expert but I think the caching math should be cheap: scaleDistance is just the abs of max - min and the rest is just division. I can see why we would want to move it elsewhere even if performance isn't an issue though...cacheMouse mostly deals with _x and _y (eg....caching the mouse) whereas this new code deals with caching the relative scale between this chart and its source chart

[EmiPhil]

See new code where I pulled out the caching logics into their own function. I also only call it in mousemove but now that it is a function we could call it wherever makes sense

[EmiPhil]

I think this implementation is the cleanest. By adding a second param for src in update cursor we can do the work where we need to without introducing globals. Also I made it so that the uni of the two charts doesn't have to be same - if we are getting our drag command from a src we use the src's uni value and scale our position to that. So you can have a small uni threshold if you want to x/y zoom on the ranger and a much larger one on the main chart. Otherwise they had to be the same so at a uni of, say, 40 on the ranger it was effectively infinity (with this change you could purposefully set the ranger one to infinity if you only wanted x or y).

[EmiPhil]

This push makes it so that we only do drag work is the cursor is in bounds on the chart. It can happen that the cursor isn't in bounds if you are syncing by absolute value of x/y but the scales are zoomed different. This also allows the cursor to lock when it is out of bounds, fixing bug where cursor lock wouldn't work when the cursor was out of bounds on one of the sync charts

[EmiPhil]

Next up to fix, if you click outside of the zoom range on the ranger it seems to reset the zoom of the main chart. I think this has something to do with the out of bounds logic as well.

[EmiPhil]

:facepalm: :facepalm: yeppp and that fixes my "next up to fix"

[EmiPhil]

Okay, now to break it you have to click within at the veeeeery very start or veeeeery very end of the y/x axis (on the sync-cursor demo).

I think that you have run into this before based on the logic here in cacheMouse:

        if (snap) {
            if (_x <= 1 || _x >= plotWidCss - 1)
                _x = incrRound(_x, plotWidCss);

            if (_y <= 1 || _y >= plotHgtCss - 1)
                _y = incrRound(_y, plotHgtCss);
        }

[EmiPhil]

The simple fix is to get rid of the check for snap and just force round it in those two cases.