Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Dragging on force layouts should update x and y as well as px, py #919

Open
ezyang opened this Issue · 2 comments

2 participants

@ezyang

Currently, when you use the built in force layout drag functionality, the following code is called:

  function dragmove(d) {
    d.px = d3.event.x;
    d.py = d3.event.y;
    force.resume(); // restart annealing
  }

The reason this works is that the Verlet integration is special-cased to treat dragged nodes (which are marked "fixed") specially:

    // position verlet integration
    i = -1; while (++i < n) {
      o = nodes[i];
      if (o.fixed) {
        o.x = o.px;
        o.y = o.py;
      } else {
        o.x -= (o.px - (o.px = o.x)) * friction;
        o.y -= (o.py - (o.py = o.y)) * friction;
      }
    }

This seems a bit odd to me: wouldn't it make more sense to set x and y along side px and py? That is to say, all of the force calculations should be using the present position of the node, as opposed to the previous position.

Now, this is all a bit academic with the current implementation, since the positional offsets are never too different. However, let's consider now a hypothetical variant of force directed layout which implements pauses. (OK, maybe not so hypothetical: I've implemented it.) That is to say, when the layout is paused, calls to alpha are buffered (including resume), and not invoked until the layout is unpaused. What happens when you drag a node when the layout is paused? There are two possibilities: (1) the node doesn't move, since all positions are frozen, or (2) the node moves, but no forces are applied.

Here's what actually happens: the node doesn't redraw (remember, only px, py) are changing, but once the layout is unpaused, the node flies off in the opposite direction it was paused. This is because the drag already finished, so the fixed flag is no longer set! Eek!

Fortunately, the fix is simple:

  function dragmove(d) {
    d.x = d.px = d3.event.x;
    d.y = d.py = d3.event.y;
    force.resume(); // restart annealing
  }

But I have to convince you that it's a good idea, right?

@mbostock
Owner

I don't see any harm in setting x and y on dragmove.

It just hasn't been necessary, so far, because when a node is fixed, it's position is locked to px and py. The reason the position is locked to px and py, and not x and y, is that this eliminates the need for each force to check if a node is fixed before applying. By separating the concern, the code is simplified and branching is reduced, potentially making things a tiny bit faster. This extends to custom forces, as well, implemented by listening for "tick" events.

But setting x and y in addition to px and py on dragmove wouldn't change this, so it seems safe, if slightly redundant.

I'm not sure I understand how your pausing force layout, though. Why not use force.stop() and force.resume() to pause, and avoid this issue?

@ezyang

For example, with the default drag behavior, when you drag a node the graph reheats. So if you want to drag a node without causing force layout, you need to reimplement all of the drag functionality yourself to have it not call resume. And in general, you may have lots of functions which are calling stop/resume. So the extra layer of indirection helps out a bit.

Here's an example implementation:

/**
 * Pausable force layouts.
 *
 * When debugging force-directed layouts, it's useful to be able to pause
 * the layout algorithm, make some modifications, and then start it up
 * again.  Why might you not want to use stop/resume/start manually?
 * Well, you might have some code which tweaks a parameter in the layout
 * and then invokes stop/resume/start; however, if the layout is paused,
 * you'd really like that change to be buffered for until the layout is
 * unpaused.
 *
 * This adds a new accessor, 'paused', which allows you to pause and
 * unpause the layout.  Otherwise, the interface is the same.
 *
 * Note for draggers: if you have installed a drag handler, strange
 * things may happen since the (x,y) coordinates were not updated.
 * See https://github.com/mbostock/d3/issues/919
 *
 * Usage:
 *
 *    var force = d3.layout.force();
 *    d3_layout_force_pausable(force);
 *
 * (Alas, force does not support 'call').
 */

function d3_layout_force_pausable(force) {
  var paused = false,
      pendingAlpha,
      pendingStart;

  force.paused = function(v) {
    if (!arguments.length) return paused;
    if (v != paused) { // only trigger on change
      if (v) {
        pendingAlpha = force.alpha(); // careful about updating me
        pendingStart = false;
        force.stop();
        paused = true;
      } else {
        paused = false; // must be done before
        if (pendingStart) force.start();
        force.alpha(pendingAlpha);
      }
    }
    return force;
  }

  function replace(orig, h) {
    return function() {
      if (paused) {
        return h.apply(this, arguments);
      } else {
        return orig.apply(this, arguments);
      }
    }
  }

  force.alpha = replace(force.alpha, function(v) {
    if (!arguments.length) return pendingAlpha;
    pendingAlpha = parseFloat(v);
    return force;
  });
  force.start = replace(force.start, function() {
    pendingStart = true;
    pendingAlpha = 0.1;
    return force;
  });

  return force;
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.