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

Stop successive edges from linking up #77

Open
JakeArkinstall opened this issue Apr 13, 2016 · 3 comments
Open

Stop successive edges from linking up #77

JakeArkinstall opened this issue Apr 13, 2016 · 3 comments

Comments

@JakeArkinstall
Copy link

I'm trying this library out, and I have to admit that it is very useful.

One issue that I'm having right now is that drawing two independent edges ends up with a third edge being drawn. Simple example:

graph = G.graph({
    antialias: true,
    bgColor: "black",
    edgeWidth: 1,
    nodeSize: 10
});
G.node([0, 0, 0], {id: 0}).addTo(graph);
G.node([0, 1, 0], {id: 1}).addTo(graph);
G.node([1, 0, 0], {id: 2}).addTo(graph);
G.node([1, 1, 0], {id: 3}).addTo(graph);

G.edge([0, 1]).addTo(graph);
G.edge([2, 3]).addTo(graph);

graph.renderIn('frame');

Expected result:

 O     O
 |     |
 |     |
 |     |
 |     |
 O     O

Result with the current version of graphosaurus:

 O     O
 |\    |
 | \   |
 |  \  |
 |   \ |
 O     O

This is problematic for me. If I'm doing something wrong here, I can't see what it is.

I have tried looking around the code and finding where this happens, but I chose this library specifically to avoid the learning code of three.js - so I don't know where this error is creeping in.

@JakeArkinstall
Copy link
Author

So it looks like the entire graph has one line formed of segments, and adding an edge simply adds two more vertices to this segmented line.

So I presume this is the intended behaviour? Is there a simple way of getting around that? (Though I must admit, I'd be confused about this being intended behaviour - it would seem to place a huge restriction on the types of graphs that this can be used for :( ).

@JakeArkinstall
Copy link
Author

I have a proposal below. It works, but I doubt that is the most efficient approach. In fact, I REALLY hope that it isn't the most efficient approach, because it impacts performance. Fortunately, it only has an impact on three methods that are already grouped together, so I'll just post those.

Frame.prototype._normalizePositions = function () {
    if (this.nodesHaveBeenNormalized || this._numNodes() < 2) {
      return;
    }

    this.nodesHaveBeenNormalized = true;

    this.scale = 1 / this.points.boundingSphere.radius;
    var positions = this.points.attributes.position.array;

    for (var i = 0; i < positions.length; i++) {
        positions[i] *= this.scale;
    }

    for (var i = 0; i < this.edges.length; i++){
        positions = this.edges[i].attributes.position.array;
        positions[0] *= this.scale;
        positions[1] *= this.scale;
    }
};

Frame.prototype._initEdges = function () {
    this.edges = [];
    this.lines = [];
};

Frame.prototype._syncEdgeDataFromGraph = function () {
    var edges = this.graph.edges();

    var material = new THREE.LineBasicMaterial({
        vertexColors: THREE.VertexColors,
        linewidth: this.graph._edgeWidth,
        opacity: this.graph._edgeOpacity,
        transparent: this.graph._edgeOpacity < 1,
    });

    for (var i = 0; i < edges.length; i++) {
        var positions = new THREE.BufferAttribute(new Float32Array(6), 3);
        var colors = new THREE.BufferAttribute(new Float32Array(6), 3);
        var edge = edges[i];
        var nodes = edge.nodes();

        positions.setXYZ(
            0,
            this.scale * nodes[0]._pos.x,
            this.scale * nodes[0]._pos.y,
            this.scale * nodes[0]._pos.z);

        positions.setXYZ(
            1,
            this.scale * nodes[1]._pos.x,
            this.scale * nodes[1]._pos.y,
            this.scale * nodes[1]._pos.z);

        colors.setXYZ(
            0,
            edge._color.r,
            edge._color.g,
            edge._color.b);

        colors.setXYZ(
            1,
            edge._color.r,
            edge._color.g,
            edge._color.b);

        var edgeGeometry = new THREE.BufferGeometry();
        edgeGeometry.addAttribute('position', positions);
        edgeGeometry.addAttribute('color', colors);
        this.edges.push(edgeGeometry);

        var line = new THREE.Line(edgeGeometry, material, THREE.LineSegments);
        this.lines.push(line);

        this.scene.add(line);
    }
};

@frewsxcv
Copy link
Owner

The line here might be problematic for your scenario. The distinction here is between THREE.Line and THREE.LineSegments. I originally used THREE.Line, but was encountering performance issues, and (if I recall correctly) I switched to THREE.LineSegments (which used to be called THREE.LinePieces) because it only has to draw one line and performance was better, though that might have been the wrong solution. I wasn't considering the scenario where there a user might want to draw two disjoint graphs.

So, all in all, I would recommend try changing that line from LineSegments to Line and see if that solves your problem. If it does let me know, I'm curious. We can maybe include it as an option somewhere.

On a side note, I just noticed a possibly different bug: #78

It works, but I doubt that is the most efficient approach. In fact, I REALLY hope that it isn't the most efficient approach, because it impacts performance.

I would be extremely surprised if any of my code is anywhere near ideal, performance wise. This was my first project related to graphics, and the code reflects that. If you have any ideas about how to improve any of it, please let me know!

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

No branches or pull requests

2 participants