-
Notifications
You must be signed in to change notification settings - Fork 70
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
Add force-directed graph #25
base: master
Are you sure you want to change the base?
Conversation
Exciting! Will review soon, thanks :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @tofuness. This is really exciting!
I added some general comments here. And I wonder if you have any new tweaks to the jittery problem that we discussed.
|
||
# Hacky Edge implementation | ||
class Edge | ||
constructor: (source, target) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a new class with source
and target
, I wonder if this can be just a Pair
or Line
eg: var edge = new Line( source ).to( target )
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Edge
implementation holds a reference to the source
and target
. The Line
/Point
implementation currently just copies the coordinates from source
/target
.
Two solutions I can currently think of:
- We change
Graph.addEdge(Edge edge)
toGraph.addEdge(Point, Point)
orGraph.addEdge([Point, Point])
- Let
Pair
also accept references toPoint
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Then I think either Graph.addEdge(P, P)
or using an Edge
class will make sense. Changing Pair to accept references seems a bit more dangerous :)
|
||
@property 'p1', | ||
get: -> | ||
return { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be simplified as @target.clone()
return new Edge(@source.clone(), @target.clone()) | ||
|
||
|
||
class Vertex extends Vector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be a Pair
, or do you prefer the more semantic name displacement
instead of p1
?
@height = height | ||
@frames = [] | ||
|
||
_fa: (x, k) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May want to move _fa
, _fa
, and _cool
inside generate
, since they are only used inside it.
return @ | ||
|
||
|
||
class ForceDirectedGraph extends Graph |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think if we simply make this as Graph
, and then rename the generate()
to forceDirected()
?
class ForceDirectedGraph extends Graph | ||
# Fruchterman & Reingold algorithm: http://citeseer.ist.psu.edu/viewdoc/download?doi=10.1.1.13.8444&rep=rep1&type=pdf | ||
# Walshaw's algorithm: http://jgaa.info/accepted/2003/Walshaw2003.7.3.pdf | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reading all the papers! 👍
|
||
v.displacement.add( | ||
delta.$divide(deltaMag) | ||
.multiply(@_fr(deltaMag, k)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer moving this to a single line: $divide(...).multiply(...)
|
||
if (v.x < 0) | ||
v.x = 0 | ||
if (v.x > @width) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use else if
since v.x cannot be < 0 and > width.
Alternatively, use the Point's max and min functions: v.max( 0, 0 ).min( @width, @height)
super() | ||
@width = width | ||
@height = height | ||
@frames = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frames
is not used, right?
@@ -53,7 +53,8 @@ var coreFiles = coreElems.map(function(n) { return path.src.core+n+".coffee"; } | |||
|
|||
var extendElems = [ | |||
"Easing", "GridCascade", "ParticleEmitter", "ParticleField", "QuadTree", | |||
"SamplePoints", "StripeBound", "UI", "Noise", "Delaunay", "Shaping" | |||
"SamplePoints", "StripeBound", "UI", "Noise", "Delaunay", "Shaping", | |||
"ForceDirectedGraph", "Vertex", "Edge" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we should add Vertex
and Edge
as separate classes right now, unless they are useful enough on their own.
Sorry for keeping this PR stale 🙏 I've been thinking about the implementation and I think I need to revisit this algorithm. Fruchterman & Reingold (current implementation) runs on O(n^2) and is horribly slow. I lost my (half-working) implementation of this 😭 I've also looked into FADE (that builds on the Barnes-Hut simulation): I am not sure what I should do at the moment. Maybe try and take a stab at FADE? |
@tofuness Wow these papers are intense. I would suggest you pick an implementation since you probably know way more about this than I do. I appreciate your contribution, even if it's O(n^2). It's a start! |
Let's get this party started 🎊
demo/index.html
file and thegulpfile.js
to make this easier to review. Just rungulp extend
and check out the demo to get started 😄 .Problems:
ForceDirectedGraph
file, I have multiple classes implemented.Graph
to maybe extendPointSet
Edge
to maybe extendPair
Edge
implementation is bad and looks like that because I wanted it to be compatible withForm.lines
.Let me know your thoughts. Please take a look at this when you have time and sorry for not splitting it into multiple commits 💦 ..