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

ScatterPlot pan and zoom on both axes #1590

Open
icoert opened this issue Oct 2, 2019 · 21 comments
Open

ScatterPlot pan and zoom on both axes #1590

icoert opened this issue Oct 2, 2019 · 21 comments

Comments

@icoert
Copy link

icoert commented Oct 2, 2019

I am looking forward to diving into scatterPlot.js to make possible pan and zoom on both axes. Now it's working only for one axis, x. I would also want to stop filtering when zoom is applied, but not necessarily (it could involve a lot of work in coordinate-grid-mixin.js I guess). Any hints or some indications?

@icoert icoert changed the title ScatterPlot pan and zoom ScatterPlot pan and zoom on both axes Oct 2, 2019
@gordonwoodhull
Copy link
Contributor

You are right, you would have to dig into coordinate grid mixin and find out! To my knowledge no one has done this with dc.js yet.

@icoert
Copy link
Author

icoert commented Dec 30, 2019

You are right, you would have to dig into coordinate grid mixin and find out! To my knowledge no one has done this with dc.js yet.

I followed the way that zoom is applied on X axis and I did the same to make possible zooming on Y axis, but it's not working as I expected.

_zoom.transform(_chart.root(), domainXToZoomTransform(_chart.x().domain(), _xOriginalDomain, _origX));
_zoom.transform(_chart.root(), domainYToZoomTransform(_chart.y().domain(), _yOriginalDomain, _origY));

There is a problem with _yOriginalDomain:

dc.js:4595 Uncaught TypeError: Cannot read property '1' of undefined
    at domainYToZoomTransform (dc.js:4595)
    at updateD3zoomTransform (dc.js:4605)
    at Object._chart._enableMouseZoom (dc.js:4518)
    at configureMouseZoom (dc.js:4491)
    at Object._chart._doRender (dc.js:4443)
    at Object._chart.render (dc.js:2092)
    at Object.complete (index.js:380)
    at l.parseChunk (papaparse.min.js:7)
    at l._chunkLoaded (papaparse.min.js:7)
    at XMLHttpRequest.<anonymous> (papaparse.min.js:7)

https://jsfiddle.net/silviuuv/Ldkptja0/95/

I changed it a little like this and something is happening.

_zoom.transform(_chart.root(), domainXToZoomTransform(_chart.x().domain(), _xOriginalDomain, _origX));
_zoom.transform(_chart.root(), domainYToZoomTransform(_chart.y().domain(), _chart.y().domain(), _origY));

@gordonwoodhull
Copy link
Contributor

I wasn’t able to load your fiddle - perhaps too much data. So I don’t know if the “something” that is happening is a good thing. I’ll try again later.

Yes there is no _yOriginalDomain. Might need to add one in order to stop zooming out too far or to reset the zoom.

@icoert
Copy link
Author

icoert commented Dec 30, 2019

I wasn’t able to load your fiddle - perhaps too much data. So I don’t know if the “something” that is happening is a good thing. I’ll try again later.

Yes, I added the entire edited dc.js there.

Yes there is no _yOriginalDomain. Might need to add one in order to stop zooming out too far or to reset the zoom.

I added one, but it's not recognized as _xOriginalDomain is..

@gordonwoodhull
Copy link
Contributor

Right, you’d have to find the 2-3 other places where _xOriginalDomain is used, and do something similar.

(But not necessarily in the same places, since the X and Y axes are often processed separately.)

@icoert
Copy link
Author

icoert commented Jan 6, 2020

Here is what I did
The behaviour of zooming/panning and filtering based on zooming is not the one I expected. (I am close anyway)

I duplicated the class CoordinateGridMixin as CoordinateGridMixinForScatter and used it with the scatter.

Please have a look. A fiddle where I copied the entire dc.js file

@gordonwoodhull
Copy link
Contributor

Thanks for the demo - I was just about to ask for that.

This is neat!

Could you spell out what you expect and the actual behavior? Is the problem that you want it to zoom around the mouse cursor and instead it seems to be snapped/locked to the minimum Y coordinate?

@icoert
Copy link
Author

icoert commented Jan 6, 2020

Yes, exactly! The zoom and pan are still somehow limited to X axis. I would like to achieve something similar with this example.

@gordonwoodhull
Copy link
Contributor

Yep, it looks like it's working correctly except for some reason it's locking the minimum Y value.

Did you modify dc.js directly or do you have a fork I could look at? It's kind of tricky trying to diff dc.js itself since it is an artifact of the build. Would be much easier to diff just the coordinate grid mixin to see what you've done.

@icoert
Copy link
Author

icoert commented Jan 6, 2020

I edited directly dc.js :( here it is --> CoordinateGridMixinForScatter

@gordonwoodhull
Copy link
Contributor

It looks like the relevant code in _domainToZoomTransform is getting called in error - that is only supposed to be hit when the zoom is set due to a call to .focus().

It looks like this was changed as part of transitioning to d3.zoom 4.0, in 3653b4e.

If you change your _updateD3zoomTransform to

    _updateD3zoomTransform () {
        if (this._zoom && d3.event.sourceEvent.type !== 'wheel') {
            this._zoom.transform(this.root(), this._domainToZoomTransform(this.x().domain(), this._xOriginalDomain, this._origX, this.y().domain(), this._yOriginalDomain, this._origY));
        }
    }

then you will get the default behavior from D3, which is the right one:

https://jsfiddle.net/gordonwoodhull/16c0fosh/18/

It would be really nice to add this feature to dc.js, if you feel like creating a pull request. Otherwise, at least the code is preserved here in the fiddle.

@icoert
Copy link
Author

icoert commented Jan 7, 2020

It really does the thing, but it's not perfect yet. It seems that panning is not working.

I don't know why in my case when I edit dc.js, because of 'd3.event.sourceEvent.type' it's saying that d3 is not defined.

And for 'd3Selection.event.sourceEvent.type' it's saying that 'Cannot read property 'sourceEvent' of null'.

Edit: I fixed editing focus function:
if(d3Selection.event.sourceEvent.type !== 'wheel')
this._updateD3zoomTransform();

Panning is still the problem.

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Jan 7, 2020

Good point about the symbol name for D3 - as of v4 we are importing the different D3 modules separately, so you need to use the name of the specific module now.

Panning works in the above fiddle, but dc.js limits panning to the original domain, so you can only pan to the boundaries that were specified (or, once #1623 is fixed, the boundaries that were inferred by elastic X/Y).

You can set zoomOutRestrict to false to override this behavior.

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Jan 7, 2020

It's also necessary to ignore mousemove events for panning to work correctly, so:

    _updateD3zoomTransform () {
        if (this._zoom && d3Selection.event.sourceEvent.type !== 'wheel' && d3Selection.event.sourceEvent.type !== 'mousemove') {
            this._zoom.transform(this.root(), this._domainToZoomTransform(this.x().domain(), this._xOriginalDomain, this._origX, this.y().domain(), this._yOriginalDomain, this._origY));
        }
    }

https://jsfiddle.net/gordonwoodhull/16c0fosh/41/

This still isn't completely correct, as it throws an error

Uncaught TypeError: Cannot read property 'sourceEvent' of null

on loading the chart. But it seems to work correctly. My attempts to fix this caused other problems, and I've run out of time for the moment.

Probably the right thing to do is to fix the math in _domainToZoomTransform by looking at what d3.zoom is doing. It looks like the Y transform should be based on the mouse position as well as the extent, but I'm not clear why this is necessary for Y and not for X.

I also see that grid lines are getting lost when you zoom in and then out again. I don't think this is a bug in the base dc.js, since examples like this one work fine.

@icoert
Copy link
Author

icoert commented Jan 7, 2020

Thanks, I really apreciate your effort.

This is how I fixed the error:

 _updateD3zoomTransform () {
          if(d3Selection.event != null)   
            if (this._zoom && d3Selection.event.sourceEvent.type !== 'wheel' && d3Selection.event.sourceEvent.type !== 'mousemove') {
                this._zoom.transform(this.root(), this._domainToZoomTransform(this.x().domain(), this._xOriginalDomain, this._origX, this.y().domain(), this._yOriginalDomain, this._origY));
            }
      }

@icoert
Copy link
Author

icoert commented Jan 8, 2020

One more thing which seems to not working well. If I call the function focus() or refocusAll() the scatter gets the initial state, but when I zoom in it starts zooming in from the last session of zooming. It's pretty weird.

It looks like the function _updateD3zoomTransform () doesn't do anything.

I fixed it:

      _domainToZoomTransform (xnewDomain, xorigDomain, xScale, ynewDomain, yorigDomain, yScale) {
          const xk = (xorigDomain[1] - xorigDomain[0]) / (xnewDomain[1] - xnewDomain[0]);
          const xt = -1 * xScale(xnewDomain[0]);
  
          const yk = (yorigDomain[1] - yorigDomain[0]) / (ynewDomain[1] - ynewDomain[0]);
          const yt = 1 * yScale(ynewDomain[1]);
          return d3Zoom.zoomIdentity.translate(xt, yt);
      }
  
      _updateD3zoomTransform () {
        if(d3Selection.event != null)
            if (this._zoom && d3Selection.event.sourceEvent.type !== 'wheel' && d3Selection.event.sourceEvent.type !== 'mousemove') {
                this._zoom.transform(this.root(), this._domainToZoomTransform(this.x().domain(), this._xOriginalDomain, this._origX, this.y().domain(), this._yOriginalDomain, this._origY));
            }
        if(d3Selection.event == null)
            this._zoom.transform(this.root(), this._domainToZoomTransform(this.x().domain(), this._xOriginalDomain, this._origX, this.y().domain(), this._yOriginalDomain, this._origY));
      }

@gordonwoodhull
Copy link
Contributor

Yes, the cases that were throwing errors are definitely needed sometimes.

Impressed with all of your work here.

Since _domainToZoomTransform was called in all cases before, I think it could be fixed to return the same results as d3-zoom. If anyone wants to work on getting this into core, I think that would be the way to go.

Yesterday, before I ran out of time, I was able to turn your work into a clean patch. It just had a weird issue where the dots faded to grey once you zoomed, which I couldn't figure out. I should fix that up and submit as a PR so that this is available to others.

Ideally the functionality should be merged into CoordinateGridMixin instead, but the new modular structure of DCv4 should also allow people to replace individual components (here, the mixin and scatter plot), without having to patch the source.

@icoert
Copy link
Author

icoert commented Jan 8, 2020

I avoided that by eliminating the filtering feature while you zoom. I tried to make a clean path using a two dimensional filter, because once you zoom on both axes you need to filter by two dimensions, but I wasn't able to achieve satisfactory results. The one responsible for this is _zoomHandler. I sincerely like zooming without filtering.
Check this demo

@gordonwoodhull
Copy link
Contributor

Ah, that makes sense, thanks! The chart should not be filtering itself but it's possible for bugs to arise where it does. Your setup code is correct but something may have gone wrong in your fork of CoordinateGridMixin, or there may have been a latent, undiscovered bug in the original.

Completely agree that ideally zooming and filtering should be decoupled. dc.js grew out of basically a demo and "cookbook", so sometimes features are tied together in surprising ways. I'd support adding a flag to disable filtering on zoom, although there are workarounds (which escape my mind atm).

@icoert
Copy link
Author

icoert commented Jan 8, 2020

I didn't use my fork. You can find the dc.js attached in my drive. I am not so skilled with pull requests, most because I have a lack of experience. 😃

It would be nice to have a version with this feature.

@gordonwoodhull
Copy link
Contributor

Thanks, yes, I was able to extract your changes from the generated file yesterday.

I was using the word "fork" informally, not in the Git sense. You copied the contents of CoordinateGridMixin in order to create CoordinateGridMixinForScatter.

Maybe it's easy for me to say, since Git made my brain hurt when I learned it a few years ago. But really you just have to click the fork button, clone the resulting repo, make and your changes to src/, build with grunt, commit the changes, and push them back to your fork. GitHub will guide you to the PR from there.

Don't worry about it.

I promise I will make your changes available in a branch of the repo. I am not promising to do all the work that would be necessary to bring the feature into mainline:

  • debugging
  • writing new tests, if possible
  • merging back into the original CoordinateGridMixin

Hopefully someone else can pick that up.

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

No branches or pull requests

2 participants