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

Node Opacity #554

Merged
merged 35 commits into from
Mar 31, 2020
Merged

Node Opacity #554

merged 35 commits into from
Mar 31, 2020

Conversation

Tyler-Maclachlan
Copy link
Contributor

Hey I added a node option for overall node opacity (it will set border, shadows and images).

I also added ordering for hovered nodes in the renderer for: #226

@Tyler-Maclachlan
Copy link
Contributor Author

Hey @Thomaash @mojoaxel

Can you guys give this a look

Copy link
Member

@Thomaash Thomaash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you provide an example for this?

lib/network/modules/components/Node.js Outdated Show resolved Hide resolved
lib/network/modules/components/nodes/shapes/Image.js Outdated Show resolved Hide resolved
@Tyler-Maclachlan
Copy link
Contributor Author

Hey @Thomaash

I've added an example and fixed up the things.

Copy link
Member

@Thomaash Thomaash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the example and some of the other changes are also great. However it doesn't seem to work. When I set opacity for one node it affects some other too, sometimes all of them. If I set it on a group it just behaves weirdly (it's either ignored or it affects even some nodes that don't belong into the group). Also opacity zero behaves weirdly (I guess because it's falsy and you somewhere test it for truthiness instead of nullness). Also this should be mentioned in the docs (also in the Korean docs, TODO text instead of Korean is okay if you don't speak Korean).

Tyler Maclachlan added 2 commits March 20, 2020 22:43
KR Docs todo
@Tyler-Maclachlan
Copy link
Contributor Author

Hey @Thomaash

Fixed the issues, they were all because of a falseness check.

@Tyler-Maclachlan
Copy link
Contributor Author

Oops, clicked close and comment by accident

Copy link
Member

@Thomaash Thomaash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems much better but there are still some things that need polishing:

  • NaN, infinity etc. should be ignored and reported (console.error). Now it misbehaves with them.
  • Values outside of <0, 1> should also be ignored and reported.
  • Explicitly set opacity on a node seems to be ignored when the node is selected.
  • The example should say something more than “Display nodes as images”.
  • Your branch is out of date. It's necessary to merge changes from the master before merging into the master.

Tyler-Maclachlan and others added 2 commits March 22, 2020 21:07
Fix problems with values invalid for opacity
@Tyler-Maclachlan
Copy link
Contributor Author

  • NaN, infinity etc. should be ignored and reported (console.error). Now it misbehaves with them. - done

  • Values outside of <0, 1> should also be ignored and reported. - done

  • Explicitly set opacity on a node seems to be ignored when the node is selected. - I have one in the example and it's working as it should, can you provide a codepen?

  • The example should say something more than “Display nodes as images”. - done

  • Your branch is out of date. It's necessary to merge changes from the master before merging into the master. - done

Copy link
Member

@Thomaash Thomaash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try setting the Main node's opacity to 7, Number.POSITIVE_INFINITY, Number.NaN etc. and see what happens. There are no errors in the console, the node has initially opacity 1 but if you click it, it becomes transparent.

Also don't use isNaN and isFinite. They're quite funky in their behavior and often return unexpected results (due to type coercion). Use Number.isNaN and Number.isFinite instead.

Copy link
Member

@Thomaash Thomaash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to work now, there are just some minor code quality issues before it can be shipped.

lib/network/modules/NodesHandler.js Outdated Show resolved Hide resolved
lib/network/modules/components/Node.js Outdated Show resolved Hide resolved
lib/network/modules/components/Node.js Outdated Show resolved Hide resolved
Copy link
Member

@Thomaash Thomaash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still seems that you're leaking the options between nodes somehow.

Check the following example (the main node has opacity 1 and there's no opacity set for all). The main node has opacity 1 and no group but it's transparent unless selected.

<!doctype html>
<html>
<head>
  <title>Vis Network | Node Styles | Images with Opacity</title>

  <style type="text/css">
    #mynetwork {
      width: 600px;
      height: 600px;
      border: 1px solid lightgray;
    }
  </style>

  <script type="text/javascript" src="../../../standalone/umd/vis-network.min.js"></script>

  <script type="text/javascript">
    var nodes = null;
    var edges = null;
    var network = null;

    var DIR = '../img/refresh-cl/';
    var EDGE_LENGTH_MAIN = 150;
    var EDGE_LENGTH_SUB = 50;

    // Called when the Visualization API is loaded.
    function draw() {
      // Create a data table with nodes.
      nodes = [];

      // Create a data table with links.
      edges = [];

      nodes.push({id: 1, label: 'Main', image: DIR + 'Network-Pipe-icon.png', shape: 'image', opacity: .7});
      nodes.push({id: 2, label: 'Office', image: DIR + 'Network-Pipe-icon.png', shape: 'image'});
      nodes.push({id: 3, label: 'Wireless', image: DIR + 'Network-Pipe-icon.png', shape: 'image'});
      nodes.push({id: 22, label: 'Normal', opacity: 1});
      edges.push({from: 1, to: 2, length: EDGE_LENGTH_MAIN});
      edges.push({from: 1, to: 3, length: EDGE_LENGTH_MAIN});
      edges.push({from: 1, to: 22, length: EDGE_LENGTH_MAIN});

      for (var i = 4; i <= 7; i++) {
        nodes.push({id: i, label: 'Computer', image: DIR + 'Hardware-My-Computer-3-icon.png', shape: 'image', group: 'computer'});
        edges.push({from: 2, to: i, length: EDGE_LENGTH_SUB});
      }

      nodes.push({id: 101, label: 'Printer', image: DIR + 'Hardware-Printer-Blue-icon.png', shape: 'image'});
      edges.push({from: 2, to: 101, length: EDGE_LENGTH_SUB});

      nodes.push({id: 102, label: 'Laptop', image: DIR + 'Hardware-Laptop-1-icon.png', shape: 'image'});
      edges.push({from: 3, to: 102, length: EDGE_LENGTH_SUB});

      nodes.push({id: 103, label: 'network drive', image: DIR + 'Network-Drive-icon.png', shape: 'image'});
      edges.push({from: 1, to: 103, length: EDGE_LENGTH_SUB});

      nodes.push({id: 104, label: 'Internet', image: DIR + 'System-Firewall-2-icon.png', shape: 'image'});
      edges.push({from: 1, to: 104, length: EDGE_LENGTH_SUB});

      for (var i = 200; i <= 201; i++ ) {
        nodes.push({id: i, label: 'Smartphone', image: DIR + 'Hardware-My-PDA-02-icon.png', shape: 'image'});
        edges.push({from: 3, to: i, length: EDGE_LENGTH_SUB});
      }

      // create a network
      var container = document.getElementById('mynetwork');
      var data = {
        nodes: nodes,
        edges: edges
      };
      var options = {
        nodes: {},
        groups: {
          computer: {
            opacity: .3
          }
        }
      };
      network = new vis.Network(container, data, options);
    }
  </script>
  

<body onload="draw()">

<p>
  Display nodes with global opacity, individual opacity, and opacity in a group.
</p>
<div id="mynetwork"></div>

</body>
</html>

@Tyler-Maclachlan
Copy link
Contributor Author

This wasn't a leaking of options between nodes. It was the fact that the context was being changed without being restored.

Copy link
Member

@Thomaash Thomaash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yet another issue: Group opacity overrides node opacity. It should be the other way around.

<!doctype html>
<html>
<head>
  <title>Vis Network | Node Styles | Images with Opacity</title>

  <style type="text/css">
    #mynetwork {
      width: 600px;
      height: 600px;
      border: 1px solid lightgray;
    }
  </style>

  <script type="text/javascript" src="../../../standalone/umd/vis-network.min.js"></script>

  <script type="text/javascript">
    var nodes = null;
    var edges = null;
    var network = null;

    var DIR = '../img/refresh-cl/';
    var EDGE_LENGTH_MAIN = 150;
    var EDGE_LENGTH_SUB = 50;

    // Called when the Visualization API is loaded.
    function draw() {
      // Create a data table with nodes.
      nodes = [];

      // Create a data table with links.
      edges = [];

      nodes.push({id: 1, label: 'Main', image: DIR + 'Network-Pipe-icon.png', shape: 'image', opacity: .7});
      nodes.push({id: 2, label: 'Office', image: DIR + 'Network-Pipe-icon.png', shape: 'image'});
      nodes.push({id: 3, label: 'Wireless', image: DIR + 'Network-Pipe-icon.png', shape: 'image'});
      nodes.push({id: 22, label: 'Normal', opacity: 1})
      edges.push({from: 1, to: 2, length: EDGE_LENGTH_MAIN});
      edges.push({from: 1, to: 3, length: EDGE_LENGTH_MAIN});
      edges.push({from: 1, to: 22, length: EDGE_LENGTH_MAIN});

      for (var i = 4; i <= 7; i++) {
        nodes.push({id: i, label: 'Computer', image: DIR + 'Hardware-My-Computer-3-icon.png', shape: 'image', group: 'computer'});
        edges.push({from: 2, to: i, length: EDGE_LENGTH_SUB, opacity: 0.1});
      }

      nodes.push({id: 101, label: 'Printer', image: DIR + 'Hardware-Printer-Blue-icon.png', shape: 'image'});
      edges.push({from: 2, to: 101, length: EDGE_LENGTH_SUB});

      nodes.push({id: 102, label: 'Laptop', image: DIR + 'Hardware-Laptop-1-icon.png', shape: 'image'});
      edges.push({from: 3, to: 102, length: EDGE_LENGTH_SUB});

      nodes.push({id: 103, label: 'network drive', image: DIR + 'Network-Drive-icon.png', shape: 'image'});
      edges.push({from: 1, to: 103, length: EDGE_LENGTH_SUB});

      nodes.push({id: 104, label: 'Internet', image: DIR + 'System-Firewall-2-icon.png', shape: 'image'});
      edges.push({from: 1, to: 104, length: EDGE_LENGTH_SUB});

      for (var i = 200; i <= 201; i++ ) {
        nodes.push({id: i, label: 'Smartphone', image: DIR + 'Hardware-My-PDA-02-icon.png', shape: 'image'});
        edges.push({from: 3, to: i, length: EDGE_LENGTH_SUB});
      }

      // create a network
      var container = document.getElementById('mynetwork');
      var data = {
        nodes: nodes,
        edges: edges
      };
      var options = {
        nodes: {
          opacity: .5
        },
        groups: {
          computer: {
            opacity: 0.9
          }
        }
      };
      network = new vis.Network(container, data, options);
    }
  </script>
  

<body onload="draw()">

<p>
  Display nodes with global opacity, individual opacity, and opacity in a group.
</p>
<div id="mynetwork"></div>

</body>
</html>

@Tyler-Maclachlan
Copy link
Contributor Author

Tyler-Maclachlan commented Mar 30, 2020

@Thomaash
I fixed it, but too many functions take options and overwrite them.

Node.parseOptions should in my opinion only check that options are valid, not set any options.

Check out how chartjs does theirs.

@Thomaash
Copy link
Member

@Tyler-Maclachlan I'd like to rework the options (in fact I already did a lot of work on this), but the problem is that the options are so complex and I don't have the time to finish it. You can see my work at visjs/vis-util#71, just in case your interested.

In the unlikely case you would have the time to do a complete overhaul of this, that would be incredibly awesome.

Copy link
Member

@Thomaash Thomaash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to finally work okay. Thank you very much.

@Thomaash Thomaash merged commit b9ff848 into visjs:master Mar 31, 2020
@vis-bot
Copy link
Collaborator

vis-bot commented Mar 31, 2020

🎉 This PR is included in version 7.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants