tgdwyer / WebCola

Javascript constraint-based graph layout
http://marvl.infotech.monash.edu/webcola/
MIT License
2.01k stars 257 forks source link

`cola.d3adaptor` has different requirements on links to `d3.layout.force` #14

Closed Muon closed 10 years ago

Muon commented 10 years ago

d3.layout.force.links() requires the source and target attributes to point to elements of the nodes array, not their indices. This causes attempts to use constraints to throw exceptions if the links are specified in D3's style.

tgdwyer commented 10 years ago

Actually, we follow d3.layout.force fairly exactly even though I think it is a bit weird. d3.layout.force expects source and target to initially be indices but then converts them to pointers to elements of the nodes array inside the start method. For better or worse, we do the same thing.

Can you be more specific about how the cola.d3adaptor differed from your expectations and from d3.layout.force? Maybe give a code example? Thanks!

Muon commented 10 years ago

Oh, oops. Hm, ok. Well, I tried using flowLayout() but it blew up in stronglyConnectedComponents() when it tried to use the links' source and target properties directly as keys instead of accessing their index property. If I specify constraints manually, it blows up elsewhere. I'll take a closer look in a few hours.

tgdwyer commented 10 years ago

I see. Probably, it should be able to take either indices or node references in the links array. If indices are passed in they should be converted to direct node references as D3 does. For most use cases I believe this works correctly. However, as you have found the cola.generateDirectedEdgeConstraints is actually expecting an array of indices. So I would say you have found a bug!

Hopefully you can work around this in the short term by passing in edges as an array of indices. I'll leave this issue open until I handle it explicitly.

Thanks!

Muon commented 10 years ago

I wrote up a fix which Works For Me(tm). Probably not the best way to do it though.

EDIT 1: Eating own words now. Other bugs popped up (hooray). EDIT 2: That's not other bugs. That's the lack of @Craxic's fix for the WebCola bug found in snapapps/edgy#120 Argh.

Craxic commented 10 years ago

I recall there being a problem I had to take a look at in snapapps/edgy#152... What was it specifically you needed? Sorry...

Muon commented 10 years ago

@Craxic snapapps/edgy#152 is a different WebCola bug (I probably should have reported it here as well, though, hmm). If you could push the fix you did in snapapps/edgy@fea919c59a4488133fba8f6a9bd58acf983f4474 somewhere I can get it, that would be great.

Craxic commented 10 years ago

Oooh, you want the source? OK, I'll see if I can find it somewhere!

stevenbird commented 10 years ago

Hi @craxic, can we please have your fix?

Craxic commented 10 years ago

I've uploaded the most recent version I have to https://github.com/Craxic/WebCola. For some reason, however, the minified version is not the same as snapapps/edgy@fea919c. Let me know if it doesn't work.

tgdwyer commented 10 years ago

Sorry I've been deeply immersed in paper writing the last few days. I'll be back on deck in a day or so. Please send me another message if there's an urgent problem I can help with.

On 31 March 2014 17:04, Craxic notifications@github.com wrote:

I've uploaded the most recent version I have to https://github.com/Craxic/WebCola. For some reason, however, the minified version is not the same as snapapps/edgy@fea919chttps://github.com/snapapps/edgy/commit/fea919c. Let me know if it doesn't work.

Reply to this email directly or view it on GitHubhttps://github.com/tgdwyer/WebCola/issues/14#issuecomment-39056242 .

Dr Tim Dwyer BSc BCS(Hons) PhD Senior Lecturer and Larkins Fellow Caulfield School of Information Technology Mobile: 0481 240 767 (from overseas: +61 481 240 767)

tgdwyer commented 10 years ago

Hi All, I'm back on deck now. Could someone please send me a pull request for this fix and I'll check it and all being well merge it.

Thanks!

tgdwyer commented 10 years ago

I just checked in a pretty big change, inspired by Muon's pull request that should resolve this problem.