[KED-2365] Simplify layout algorithm, improve layout quality and performance #398
Conversation
src/utils/graph/layout.js
Outdated
| @@ -16,7 +16,7 @@ import { | |||
| * @param {array} params.nodes The input nodes | |||
| * @param {array} params.edges The input edges | |||
| * @param {object=} params.layers The node layers if specified | |||
| * @param {number} params.basisX The basis relating diagram width in X | |||
| * @param {number} params.spreadX The amount to adapt spacing in X | |||
There was a problem hiding this comment.
I wonder what is the main motivation behind adapting spacing instead of the relation to the width of the diagram, and how does it improve the layout engine? It would be worth mentioning which goal this change relates to in the PR decription.
There was a problem hiding this comment.
Ah I've updated the comment here to make it a bit clearer what that parameter does, though it's not really expected that this value will change from the included default constant which is pretty implementation relative and was found by trial and error.
If you're really curious though it's worth following the parameter through the code and playing with the value while watching in the browser.
| } | ||
|
|
||
| // Update separation constraints but ensure spacing is exact | ||
| updateSeparationConstraints(separationConstraints, rows, spaceX, true); | ||
| // Constraints to maintain a minimum horizontal node spacing |
There was a problem hiding this comment.
This change is very interesting! Similar to my comment above, it would be great to document the reasoning / motivation behind this change in the PR description.
There was a problem hiding this comment.
Ah this part was just a refactoring and simplification so I've not much to add on that.
There was a problem hiding this comment.
Thanks for introducing these improvements @bru5 , I haven't extensively tested the new layout yet, I'm sure it's good stuff!
Just some initial comments:
- I notice in the PR decription on the list of improvements, but seems to be missing the 'how' - I notice the various changes that was introduced in this PR, such as change in constraints, etc, and it would be great if you could include the explanation of the relating changes for each improvement so whoever reviewing this understand and relate the changes with the motivations behind. Currently it's a bit of a guessing game in understanding and relating the how to the list of improvements.
- There are a lot of calculations for the constraints - these needs unit tests.
I will have a more thorough read and further poke around and let you know if I see anything!
|
+1 to @studioswong's suggestion. I would love to know how these improve the current engine for future reference. |
There was a problem hiding this comment.
I've just tested out the graph, the new layout and improvements looks good! I notice that these new constraints and improvement only apply to the new graph - I persume these improvements does not apply to the old Dagre graph layout?
Following my previous comment, the constraints itself now contains quite a bit of calculations - tests would need to set up for these calculations within the constraints. I notice that there is currently no test file set up for constraints.js - perhaps it's time we set one up to ensure the constraints are correctly tested?
Thank you!
|
Thanks for the comments all, I've now added more specific tests for each constraint. I realise there is still some way to go in explaining the algorithm at a higher level than the implementation. I'll work on a higher level supporting document for that, since this is not particularly straightforward and is very abstract. Often I do find that code comments are not the best format for discussing things at a high level, for now they are describing the specific implementation and the API but I can definitely add cross references once complete. |
…ormance (#398) * [KED-2365] Simplify graph algorithm, improve graph layout quality and performance * [KED-2528] Refactor and split graph solve functions to further simplify * [KED-2528] Resolve crossing constraints symmetrically * [KED-2528] Add more specific tests for layout constraints * [KED-2528] Improve docs on graph layout functions
Description
Simplify layout algorithm, improve layout quality and performance.
Development notes
Performance and algorithmic improvements:
Example performance change for 868 nodes + 1555 edges test case
Output changes and quality improvements:
QA notes
Check various pipelines, selections and compare results with main branch.
Load time should be significantly reduced and larger pipelines should show some visual improvement described above.
Results may look somewhat different than previous renderings but in general should be as good or better than before.
Checklist
RELEASE.mdfileLegal notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":
I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.
I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.