eclipse / elk

Eclipse Layout Kernel - Automatic layout for Java applications.
https://www.eclipse.org/elk/
Other
243 stars 82 forks source link

Algorithm-specific layout option defaults #104

Open uruuru opened 7 years ago

uruuru commented 7 years ago

There's a conceptual problem with the layout option defaults. Each layouter gets its own *Options class to access individual layout options. Using that class, a proper default value is returned as specified in the melk file. However, this does not work whenever an option must be retrieved without knowing which layout algorithm is currently used. This also applies to the configuration classes for graphviz and ogdf, where every layouter can have a different default value but during configuration CoreOptions is used to avoid code duplication.

spoenemann commented 7 years ago

Where does this cause problems?

uruuru commented 7 years ago

An artificial example: dot has a default node node spacing (nns) of 15 and neato has one of 12. The LayoutDotExporter retrieves nss from a graph using the CoreOptions class. There, a default nss of 10 is specified. Thus, for a graph without any explicit nss set, both dot and neato end up having nss of 10. Additionally, after the first #getProperty(nss) call, the graph will actually have a specified nss: 10.

le-cds commented 7 years ago

Another example is the node and label size calculation. We factored that code out of ELK Layered to be generally usable. Regardless of which layout algorithm calls the code, it always uses the CoreOptions constants to access the properties, which may have different defaults than the algorithm calling the code.

le-cds commented 7 years ago

We thought about this problem a bit. Here are the "results"...

There are three ways to invoke automatic layout: the DiagramLayoutEngine, the RecursiveGraphLayoutEngine, and by invoking layout providers directly. The problem can thus not be solved solely by adding a bit of magic to the first two classes; the solution should work in the latter case as well. It follows that the solution will have to be implemented somewhere in IPropertyHolder.getProperty(...) or one of the methods called therein, in particular: IProperty.getDefault(...). The main problem is that those methods lack context as to which algorithm is used to layout the property holder they are called on.

If such context was provided to these methods, we could make them ask the LayoutMetaDataService to retrieve the proper default value for the given combination of layout option and layout algorithm. Of course, those defaults would have to be registered with the LayoutMetaDataService in the first place.

A problem is to find the currently active layout algorithm, which does not even have to be configured through properties if layout is invoked directly through a layout provider.

We will continue to think about this.

le-cds commented 7 years ago

One of the most important aspects of this is code independent of particular layout algorithms, such as the node label placement and size calculation code, which can be called by other layout algorithms. I came up with one possible solution, and I'd be interested in your opinions before (possibly) implementing it.

The core problem here is that default property values are determined by which property constant is used to retrieve a property value. The node label placement code simply uses the constants defined in CoreOptions, which is a problem. I propose to change the code such that it does not access property holders directly, but through some kind of a "proxy" object, just like the one I introduced in commit df1a539f940bd:

// Old:
node.getProperty(CoreOptions.SPACINGS_NODE_NODE);
// New:
delegator.getProperty(node, CoreOptions.SPACINGS_NODE_NODE);
// New will actually be equivalent to, for example:
node.getProperty(LayeredOptions.SPACINGS_NODE_NODE);

The PropertyConstantsDelegator would be used to access properties and could be configured to use specific property constants when accessing a given property instead of the property constant passed to its getProperty(...) method.

The question then is, who configures the delegator? Well, why not have ELK's meta data compiler generate a method into the layout option classes generated for algorithms that provides a delegator configured to use the algorithm's own property constants? That would be reasonably easy to do. Another way would be to allow delegators to be created for LayoutAlgorithmData objects, but that wouldn't work for the plain Java layer, which the former solution would.

On another note, while we're at it, I would like to change the meta data service such that instead of remembering which LayoutOptionDatas an algorithm supports with which default values, it would simply register and remember the property constants generated for an algorithm.

Anyway, this solution does not solve the problem at its core, but solves one of its symptoms (algorithm-independent code not using the correct default values). Still, what do you think, @uruuru and @spoenemann ?

spoenemann commented 7 years ago

While thinking about your proposal I got another idea: Let's introduce a "resolve" phase that is applied to an input graph to resolve the actual layout algorithm to apply to each parent node. The result of the layout algorithm resolution would be attached to the graph using a dedicated property. The phase itself could be implemented as a graph element visitor and be applied after validation.

The layout algorithm resolution results could be used in at least two cases:

Of course it would be a problem if someone invokes a layout algorithm directly instead of the DiagramLayoutEngine, since the layout algorithm resolution would not be available. That is easy to solve:

spoenemann commented 6 years ago

I implemented my idea about a layout algorithm resolution phase in #314. LayoutAlgorithmData instances are now attached to the graph with the option CoreOptions.RESOLVED_ALGORITHM. The PropertyConstantsDelegator could now pick up that information from the graph and derive default values from it.