Force nodes to stay on the svg

[JackWilb]

Depends on #121.

This change forces the nodes to stay on the SVG by setting the x and y attributes to values that are on the SVG.

I found that updating node.x and node.y and then doing translate(node.x, node.y) didn't work. I think that's because the simulation updates node.x and node.y before the node is drawn, so there's a bit of a race conditions. My solution was to set the translate to the forced values and assign those values back to node.x and node.y. That seems to keep everything settled.

[waxlamp]

I found that updating node.x and node.y and then doing translate(node.x, node.y) didn't work. I think that's because the simulation updates node.x and node.y before the node is drawn, so there's a bit of a race conditions. My solution was to set the translate to the forced values and assign those values back to node.x and node.y. That seems to keep everything settled.

I think you should file an issue to fix this in a followup. In general, the simulation code should stop nodes from going out of bounds (that is, all logic for setting node positions should be in that simulation callback), and the translation that you bind in the DOM should just be a simple function that manufactures the correct string from the node positions.

Hopefully that made sense. Basically, you have the simulation setting unbounded node positions, and rendering code snapping those back in bounds, and you're sort of expressing that snapping part twice in order to override what you think are timing considerations.

Let me know if you'd like my help in digging into this.

[JackWilb]

I found that updating node.x and node.y and then doing translate(node.x, node.y) didn't work. I think that's because the simulation updates node.x and node.y before the node is drawn, so there's a bit of a race conditions. My solution was to set the translate to the forced values and assign those values back to node.x and node.y. That seems to keep everything settled.

I think you should file an issue to fix this in a followup. In general, the simulation code should stop nodes from going out of bounds (that is, all logic for setting node positions should be in that simulation callback), and the translation that you bind in the DOM should just be a simple function that manufactures the correct string from the node positions.

Hopefully that made sense. Basically, you have the simulation setting unbounded node positions, and rendering code snapping those back in bounds, and you're sort of expressing that snapping part twice in order to override what you think are timing considerations.

I like your suggestion Roni, but I think it will cause some issue if we code it up as you envisioned it. The main issue is that the simulation is not running at all times, it stops after the nodes settle. By only having the movement code inside the simulation callback, there could be an issue where you can't click and drag the nodes around. Or if you can, that they could be dragged off the screen.

I do like your idea for not "snapping [the nodes] back in bounds".

Maybe the better solution is to have one function that sets the limits of the x and y positions and to use that in both the simulation callback, and in the nodeDrag function? Let me know if you have any other ideas.

[waxlamp]

I found that updating node.x and node.y and then doing translate(node.x, node.y) didn't work. I think that's because the simulation updates node.x and node.y before the node is drawn, so there's a bit of a race conditions. My solution was to set the translate to the forced values and assign those values back to node.x and node.y. That seems to keep everything settled.

I think you should file an issue to fix this in a followup. In general, the simulation code should stop nodes from going out of bounds (that is, all logic for setting node positions should be in that simulation callback), and the translation that you bind in the DOM should just be a simple function that manufactures the correct string from the node positions.
Hopefully that made sense. Basically, you have the simulation setting unbounded node positions, and rendering code snapping those back in bounds, and you're sort of expressing that snapping part twice in order to override what you think are timing considerations.

I like your suggestion Roni, but I think it will cause some issue if we code it up as you envisioned it. The main issue is that the simulation is not running at all times, it stops after the nodes settle. By only having the movement code inside the simulation callback, there could be an issue where you can't click and drag the nodes around. Or if you can, that they could be dragged off the screen.

Click-and-drag of course will have its own "movement" code, which will essentially move the node (and one end of any links it's attached to) to the mouse position, including a "clamping" operation that will stop the node from going out of bounds. In the "mouseup" callback that finishes the drag motion, you would restart the simulation with a low temperature.

Besides that, once the simulation settles down and stops, there's presumably no more need to clamp node positions.

I don't know if this design will work in Multilink specifically, but this is the general design for interactive force-based graph layouts, so if there's some issue in Multilink, I'd suspect it's a problem that could use some attention.

I do like your idea for not "snapping [the nodes] back in bounds".

Maybe the better solution is to have one function that sets the limits of the x and y positions and to use that in both the simulation callback, and in the nodeDrag function? Let me know if you have any other ideas.

The problem I have here is the setting of the node positions twice, not the lack of a refactored function that can handle the logic. I don't think that will cause any immediate issues for us, but it's sort of like a powder keg in the middle of our match factory.

For now I would file an issue to look at this and then move on with the current PR.

[JackWilb]

For now I would file an issue to look at this and then move on with the current PR.

Opened #136