Feature/graph layout #20
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
May 5, 2017 18:19
#9 Backbone model are created from single attiributes/options instance. In order to support instantiation method, i.e. clone, we have to follow this guidelines. Therefore, required step parameter was moved to opts. Additionally, the inheritance of VisualGroup from VisualStep was added according to model's Group->Step inheritance.
New technique creates new temporary graph with all links moved to highest rank child of each adjacent cell. This approach handles the limitation covered in: dagrejs/dagre-d3#261.
daniilsavchuk
approved these changes
May 5, 2017
This commit is aimed to support embedded cells during the layout process. A complete graph instead of just cells array is required to make this happen.
In our trick with compound graphs layout links between parent and child is reassigned to some child - child link which will only confuse the layout algorithm. Therefore, such links should be removed from aux graph.
|
@voidest , could you please provide a full test sample for this fix? i.e. link to a full WDL script and expected results of it's usage. |
1. Dagre resizes nodes with embeds and fitEmbeds, that follows the layout, makes nodes overlap. 2. Layout algorithm does not take into account port captions. Thus, thay may overlap after sorting too. Therefore, we assign the bbox size to our aux graph nodes. These changes required additional constants tweaks.
|
@sidoruka, this pr does not actually fix any visible issue. It just enables the layouting of some specific compound graphs, which we avoided to use earlier due to dagre limitation. In particular, with these changes we can use group calls (e.g. scatter) with their own ports and connect them to child steps (of course, after proper changes are made in the Model and parser). |
|
@voidest , ok thanks for clarification. |
Open
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
General idea
This pull request provides an opportunity to layout compound graphs via jointjs. At the moment the layouting is limited - links to nodes that have embeds were prohibited. Such behavour blocks our features like group steps with their own inputs (e.g. scatter, conditional calls) mentioned in #9. Provided changes allow us to layout diverse compound graphs.
Changes
Refactored Visual Step, Group and Link to fit the Backbone model. This refactoring enabled the usage of standard clone methods and allowed the creation of temporary graph that has modified links. When that temporary graph is arranged, the layout is applied to the elements of the current graph.
NOTE - please let me know before PR will be merged. I have to select suitable package.json version for npm registry.