almende / vis

⚠️ This project is not maintained anymore! Please go to https://github.com/visjs
7.85k stars 1.48k forks source link

Overlapping node *labels* in hierarchical layout #1964

Open FalFire opened 8 years ago

FalFire commented 8 years ago

I am using the hierarchical layout using the following settings:

{
    layout: {
        hierarchical: {
            direction: "LR",
            sortMethod: "directed",
            levelSeparation: 120
        }
    },
    nodes: {
        color: "#ffffff",
        shape: 'circularImage',
        size: 11,
        borderWidth: 0,
        borderWidthSelected: 0,
    },
    edges: {
        arrows: {
            middle: {
                scaleFactor: 0.4
            }
        },
        color: {
            color: '#888888',
            opacity: 0.6,
        },
        dashes: false,
        width: 0.75
    },
    interaction: {
        selectConnectedEdges: false
    }
};

Additionally, I set the image URL on each node separately. When the graph is drawn, long node labels are drawn underneath/on top of eachother. Is there a way to prevent this from happening? I tried giving nodes with longer text labels (as in number of characters) a higher mass, but this didn't have any visible impact even when setting the mass very high. Is there another setting I can use to try to prevent this, or is there a way to take into account the bounding box of nodes including their labels in the hierarchical solver?

mojoaxel commented 8 years ago

When the graph is drawn, long node labels are drawn underneath/on top of eachother.

@FalFire Could you maybe provide a screenshot to demonstrate the problem.

FalFire commented 8 years ago

See attached screenshot. The problem mainly/only occurs when the graph contains paths of consecutive nodes that have the same y coordinate. In my use-case this happens a lot.

example

mojoaxel commented 8 years ago

@FalFire Thanks for the screenshot! Looks like a bug to me.

mojoaxel commented 8 years ago

I created a jsbin to fiddle around with this issue.

mojoaxel commented 8 years ago

@AlexDM0 Maybe you want to label this as [Bug] and [Network]!?

AlexDM0 commented 8 years ago

Hi,

This is not a bug, truth be told there's not much we can do about this. I'd suggest to put linebreaks in the labels. Size of the label is not taken into account for any of the positioning code.

Regards

mojoaxel commented 8 years ago

This is not a bug

I understand, but it feels like one to the user.

Size of the label is not taken into account for any of the positioning code.

Maybe it should?

Maybe the best way would be to create a Feature-Request issue for "Overlapping Network Labels" and communicate that this is an issue at the moment.

@FalFire Line-Breaks could be a solution for you in the meantime!?

AlexDM0 commented 8 years ago

Hi,

Maybe it should be taken into account, sure. Similarly, the drawn size of the node is not taken into account. This is for performance reasons really. It would give very different results from the expectation if we let the physics depend on drawn size.

I'd prefer not to label things as bugs unless it really IS a bug: a mistake in the code. A feature request could be a solution here, though it's a tricky one. I'm not sure what the best way for this would be. Probably automatic linebreaking but it'd be computationally expensive for a bit of a cornercase.

Regards

FalFire commented 8 years ago

Hi, thanks for the discussion so far. What would solve the problem at least partially I think would be to let the levelSeparation in the hierarchical layout depend on the bounding boxes of the nodes in the two adjacent levels. This would not solve it for the other layouts but it would already help in my case. To tackle this more generally maybe allowing a user-defined function to determine the levelSeparation would be an option?

techBeck03 commented 7 years ago

I'm not programmer but this seemed to fix the issue for me.....haven't tested extensively though:

value: function _placeNodesByHierarchy(distribution) {
  var nodeId = undefined,
      node = undefined;
  this.positionedNodes = {};
  // start placing all the level 0 nodes first. Then recursively position their branches.
  for (var level in distribution) {
    if (distribution.hasOwnProperty(level)) {
      for (nodeId in distribution[level].nodes) {
        if (distribution[level].nodes.hasOwnProperty(nodeId)) {
          node = distribution[level].nodes[nodeId];
          if (this.options.hierarchical.direction === 'UD' || this.options.hierarchical.direction === 'DU') {
            if (node.options.label.length > 12){
              var labelPadding = (2 * node.options.label.length) + 20;
            }
            else{
              var labelPadding = 20;
            }
            if (node.x === undefined) {
              node.x = distribution[level].distance + labelPadding;
            }
            distribution[level].distance = node.x + this.nodeSpacing + labelPadding;
          } else {
            if (node.y === undefined) {
              node.y = distribution[level].distance;
            }
            distribution[level].distance = node.y + this.nodeSpacing;
          }
          this.positionedNodes[nodeId] = true;
          this._placeBranchNodes(node.edges, node.id, distribution, level);
        }
      }
    }
  }
dockstreet commented 7 years ago

I think this would be a helpful feature / fix. In a wrapper I'm using in R https://github.com/datastorm-open/visNetwork For a hierarchy I'm having to count the nodes for a level and then if it's over a certain amount do a modulus computation to put the other nodes on another level.

It does get around the problem but if the JavaScript that techBeck03 wrote helps , then why not include it? If it doesn't then I do think overlapping labels do make the network more challenging for the user to read by having to move the nodes.

I think it should remain open and I would be glad to test if migrated into the R visjs wrapper / htmlwidget

wimrijnders commented 7 years ago

OK, noted.

I will honestly proclaim that I do not regard this as a serious issue; but I'm not the final word on setting priority. If even one user thinks it is worthwhile, it will remain open.

wimrijnders commented 7 years ago

@dockstreet The solution in code as presented above by @techBeck03 is based on old code. The code has evolved in the meantime, so the applied change are unsuitable.

I'm investigating how to rewrite this, and also if this is a relevant fix in the first place.

wimrijnders commented 7 years ago

@FalFire:

What would solve the problem at least partially I think would be to let the levelSeparation in the hierarchical layout depend on the bounding boxes of the nodes in the two adjacent levels.

I am agreeing that something like this may be the simplest and most efficient solution. I add for completeness that nodeSpacing may also need to be adjusted for UD/DU hierarchical networks.

I am currently inclined to add a new option to layout.hierarchical for this behavior. But my viewpoint may change, still investigating.

wimrijnders commented 7 years ago

I would like to point out that the max width of the label can be set with option nodes.widthConstraint.maximum. If you select a value < nodeSpacing and < levelSeparation, the labels should not overlap. This can be used as a workaround until this issue can be completely fixed.

There are known issues with label handling, see #3171 and #3185. Fixes for these are pending.