Incorrect access to node indices inside LayoutEngine::setupHierarchicalLayout (?)

[softwareCobbler]

I think I found a small bug inside vis/lib/network/modules/LayoutEngine.js. In the file in the master branch, LayoutEngine.js line 713 reads:

for (nodeId in this.body.nodes) {

But this.body.nodes contains nodes and edges (see breakpoint screencap).

This bug becomes apparent when nodes have user-assigned levels, and the configurator toggles back and forth between hierarchical layout and "normal" layout (which calls setupHierarchicalLayout) . When nodes have levels, the edges in this.body.nodes most certainly do not (why would they?), and the program complains that To use the hierarchical layout, nodes require either no predefined levels or levels have to be defined for all nodes.

The fix I have minimally tested is a small change to line 713 to read:

for (nodeId of this.body.nodeIndices) {

which ensures we lookup nodes by only node keys.

I think it is rare for a user to toggle back and forth between hierarchical and "normal" layouts, but with the configurator it is possible. See pull request #4183 for (as of this writing) a broken version of toggling back and forth between a hierarchical layout and a normal layout, once levels have been assigned to nodes.

Any thoughts?

[softwareCobbler]

Trying to understand this further: Vis will bug out when there are nodes both with and without user-defined levels in body.nodes. A user may add nodes that all have explicitly set levels, but Vis may still complain, because in some cases it will add its own nodes to the body.nodes array.

I wondered why Vis was adding its own nodes (with ids of the form "edgeId:" || id) to the body.nodes array. It is doing this to add hidden control nodes for Bezier curves, see BezierEdgeDynamic::setupSupportNode. This may occur elsewhere in the program but it occurs at least here.

So the nodes that the code is currently iterating over are correctly present within body.nodes, but the canonical way to iterate over user-supplied nodes is to use the list of user-supplied node ids, from body.nodeIndices, as the complete set of keys for body.nodes.

I could have sworn I've seen a comment like this in the codebase but I can't find it now.