d3 / d3-force

Force-directed graph layout using velocity Verlet integration.
https://d3js.org/d3-force
ISC License
1.81k stars 376 forks source link

link.js: "nodeById" uses array position instead of "index" property #190

Closed runeksvendsen closed 3 years ago

runeksvendsen commented 3 years ago

Running initialize with a nodes array of

[{"name":"Two","index":2}, {"name":"One","index":1}, {"name":"Zero","index":0}]

results in a nodeById map which maps 0 to the string "Two". This is because the index property of a node is ignored, and instead its position in the nodes array is used as the index.

As a result, the order of the objects inside the nodes array is what determines the source and target of links, and not -- as one would expect -- the index property of the node in question.

Fil commented 3 years ago

the node's index is set (in this case, overwritten) in https://github.com/d3/d3-force/blob/master/src/simulation.js#L65

runeksvendsen commented 3 years ago

@Fil If I have the following data:

nodes = [{"name":"Two","index":2}, {"name":"One","index":1}, {"name":"Zero","index":0}]
links = [{"source": 2, "target": 1}]

where in the code is the source and target properties of links updated to reflect the new node indices?

In the above case the new node index for {"name":"Two","index":2} would be 0, so the link inside links should have its source property changed to reflect this. Otherwise the link will start at {"name":"Zero","index":0} instead of {"name":"Two","index":2}.

Fil commented 3 years ago

The change in links happens at https://github.com/d3/d3-force/blob/master/src/link.js#L60 ; this is after the nodes have been set and "indexed".

When source and target are given as indices (ie not objects), then the nodes are taken from the nodes list, and the "index" set in https://github.com/d3/d3-force/blob/master/src/simulation.js#L65 is used.

See also https://observablehq.com/d/c9eb672a85fa81b1

runeksvendsen commented 3 years ago

I believe I've reproduced the bug here: https://observablehq.com/@runeksvendsen/force-directed-graph

This draws a link between "One" and "Zero" when using a nodes array of

[ {"name":"Three", "index":3}, 
  {"name":"Two",   "index":2}, 
  {"name":"One",   "index":1}, 
  {"name":"Zero",  "index":0} ]

and a links array of [{"source": 2, "target": 3}].

Fil commented 3 years ago

Exactly, that's what happens: all the nodes' indexes are applied on initialization based on their position in the nodes array (and overwriting any "index" property they might already have); then links are computed based on that index, ie the order of the nodes.

In other words any "index" property on nodes is not taken into account, and even discarded/replaced by the array index.

It is unfortunate in your case (but you can fix by either sorting nodes to have a correct index, or assign source and target as nodes yourself, rather than as indices). The behavior is properly documented:

Each node must be an object. The following properties are assigned by the simulation:

  • index - the node’s zero-based index into nodes
runeksvendsen commented 3 years ago

Thank you for your time! I did not realize this.

The reason I was confused is because the Vega "force directed graph"-example (https://vega.github.io/vega/examples/force-directed-layout/) uses node data which contains an index property (https://vega.github.io/editor/data/miserables.json).

I will submit a PR to the Vega repo suggesting to correct this.