Provide JSON coordinate options #1071

skieffer · 2024-08-25T18:50:07Z

Resolves #901
Resolves #1012

We provide two new properties, which control what type of coordinates you get when you load a graph from JSON, and then transfer the layout back into the same JSON.

First new property

org.eclipse.elk.json.edgeCoords

Controls coordinates for edge route points and edge labels.

Possible values:

INHERIT: inherit value from parent node
CONTAINER: coords are relative to the edge's true container node (same as current behavior)
PARENT: coords are relative to the node where the edge is defined in the given JSON
ROOT: coords are "relative to root" i.e. global or absolute

Defaults to CONTAINER at root level, otherwise to INHERIT.

Second new property

org.eclipse.elk.json.shapeCoords

Controls coordinates for nodes, ports, and labels of nodes and ports.

Possible values:

INHERIT: inherit value from parent node
PARENT: coords are relative to the parent element in the given JSON
ROOT: coords are "relative to root" i.e. global or absolute

Defaults to PARENT at root level, otherwise to INHERIT.

Summary

Setting ROOT in both properties is a solution to #1012.

Setting PARENT for edgeCoords should make a majority of those people happy who have had a problem with #901, i.e. who expect the edge coords to simply be interpreted relative to the node where the edge was defined.

Questions / Tasks for this PR

Do we want to maintain the rule stated here, that edge labels are in the same coordinate system as their edges? This PR does not currently do that, which is why I'm opening it as a draft.
Should we also support a org.eclipse.elk.json.nodeCoords option? I think it would have three values: INHERIT, PARENT, and ROOT. Or should it be org.eclipse.elk.json.shapeCoords to also affect ports and labels? We need something like this for Layout option to force global container: root coordinates for all children and edges #1012 .
Docs should be updated, including page on coordinate system.

Tasks for another PR (in another repo)

elk-live should be updated to force use of PARENT coords for edges. Currently, elk-live is still broken on this issue (example).

* Record the original parent of each edge, before the call to `updateContainment()`. * Split the layout transfer into two passes: Pass 1 does everything except edges; Pass 2 does edges. This allows us to compute the global coords of each node, before working on the edges. * In this commit, we add some diagnostic output to the final JSON, which is probably not ultimately wanted. (Global coords, and original parents.)

For now, we don't do anything with it except note the enum ordinal in the JSON for each edge.

skieffer · 2024-08-25T19:04:41Z

The unit tests I added are passing on my machine. I believe I'm using Java 11. Looks like they failed in CI with Java 17. I don't know why that would make a difference.

soerendomroes · 2024-08-26T08:25:17Z

Thank you very much for starting this PR, I will try to support you if I can.

Do we want to maintain the rule stated here, that edge labels are in the same coordinate system as their edges? This PR does not currently do that, which is why I'm opening it as a draft.

I think the edge labels should be in the same coordinate system as the edge itself.

Should we also support a org.eclipse.elk.json.nodeCoords option?

If we support ROOT for edges, we might also have to do this for nodes, ports, and node labels too. If implementing the ROOToption for everything seems too complicated, I think that it is also reasonable to only implement PARENT` for edges as a first PR that could be merged.

plugins/org.eclipse.elk.graph.json/src/org/eclipse/elk/graph/json/JsonImporter.xtend

soerendomroes · 2024-08-26T09:18:25Z

One issue with the tests is the NPE pointed out above.

Another one is that the algorithm is not properly registered in the test.
The Layout algorithm 'layered' not found for Root Node seems to be a separate issue that typically occurs if one does not register the algorithm correctly.
I guess the tests has to add LayoutMetaDataService.getInstance().registerLayoutMetaDataProviders(new LayeredMetaDataProvider()); somewhere, as seen here.

test/org.eclipse.elk.graph.json.test/src/org/eclipse/elk/graph/json/test/EdgeCoordsTest.xtend

plugins/org.eclipse.elk.graph.json/src/org/eclipse/elk/graph/json/JsonImporter.xtend

(Applying suggestions from code review) Co-authored-by: Sören Domrös <[email protected]>

(applying suggestion from code review) Co-authored-by: Sören Domrös <[email protected]>

skieffer · 2024-08-26T16:07:37Z

One issue with the tests is the NPE pointed out above.

Thanks for the help in interpreting the test failures. I was only running individual tests on my local machine, since I was unable to get maven working here, and I missed the ones that were failing.

I've now got maven working locally, so I should be able to test more thoroughly now.

skieffer · 2024-08-26T16:13:43Z

QUESTION: Can you tell me how to run maven on just a single test class?

By studying the CI code for this repo I extracted this:

$ mvn -V -e --fail-at-end --no-transfer-progress \
    --define tests.paths.elk-repo=/Users/skieffer/elk-master/git/elk \
    --define tests.paths.models-repo=/Users/skieffer/elk-master/git/elk-models \
    verify

which allows me to run all the tests.

Following this guide I tried adding
-Dtest=org.eclipse.elk.graph.json.test.EdgeCoordsTest, as in

mvn -V -e --fail-at-end --no-transfer-progress \
    --define tests.paths.elk-repo=/Users/skieffer/elk-master/git/elk \
    --define tests.paths.models-repo=/Users/skieffer/elk-master/git/elk-models \
    -Dtest=test.org.eclipse.elk.graph.json.test.EdgeCoordsTest verify

but then it skips all the tests, including the one I want, and says, "No tests were executed!".

skieffer · 2024-08-26T17:46:14Z

Another one is that the algorithm is not properly registered in the test.
The Layout algorithm 'layered' not found for Root Node seems to be a separate issue that typically occurs if one does not register the algorithm correctly.
I guess the tests has to add LayoutMetaDataService.getInstance().registerLayoutMetaDataProviders(new LayeredMetaDataProvider()); somewhere, as seen here.

This one's going to be tough for me to work on, since I'm not getting the error at all, locally.
I have now run the entire test suite on my machine, under both Java 11 and Java 17, and in both cases all tests pass.

This does seem similar to what's discussed in the issue you linked (#859), where the problem arose only inside a Docker container. It seems that here the problem only occurs in the CI test runner container.

As a shot in the dark, I tried adding the line

LayoutMetaDataService.getInstance().registerLayoutMetaDataProviders(new LayeredMetaDataProvider());

inside the one new test method that I've defined in this PR, but unfortunately I can't even seem to import LayeredMetaDataProvider. I find two existing imports among the tests, here:

https://github.com/eclipse/elk/blob/ad04950f83f8aee9421f5b0666d06d6c23c66983/test/org.eclipse.elk.alg.test/src/org/eclipse/elk/alg/test/PlainJavaInitialization.java#L15

and here:

https://github.com/eclipse/elk/blob/ad04950f83f8aee9421f5b0666d06d6c23c66983/test/org.eclipse.elk.shared.test/src/org/eclipse/elk/shared/test/ElkReflectTest.java#L17

but when I put

import org.eclipse.elk.alg.layered.options.LayeredMetaDataProvider

into my test file (EdgeCoordsTest.xtend) Eclipse complains that the import can't be resolved.

Is it because I am in an .xtend file, whereas the two existing cases are in .java files? (Sorry, I hardly ever work with Java, so I am grasping at straws here!)

soerendomroes · 2024-08-27T08:41:50Z

I noticed a typo in -Dtest=~~test.~~org.eclipse.elk.graph.json.test.EdgeCoordsTest this could be one problem.
The main issue, however, is that all plugins that have no tests fail.
Adding -DfailIfNoTests=false works for me.

The following command works for me, if you do the fixes above.

mvn --define tests.paths.elk-repo="<path-to-git-repos>/elk (copy)"  \
--define tests.paths.models-repo=<path-to-git-repos>/elk-models/  \
-Dtest=org.eclipse.elk.graph.json.test.EdgeCoordsTest \
-Dmaven.repo.local=./mvnrepo clean verify --fail-at-end -DfailIfNoTests=false

The local maven repo is optional, but I use this to not compromise my user local nvm repository. Clean might also be optional. Additionally, I tend to copy the elk repository to avoid unnecessary artifacts

I am usually developing ELK by using the Eclipse Setup of our semantics repository using the ELK stream to install ELK, Klighd, and our SCCharts repositories in an Eclipse such that I can just run Unit tests in Eclipse.

test/org.eclipse.elk.graph.json.test/src/org/eclipse/elk/graph/json/test/EdgeCoordsTest.xtend

Co-authored-by: Sören Domrös <[email protected]>

skieffer · 2024-08-27T13:41:32Z

Thank you for all the help. The test. typo was a variation, after my initial attempt without it failed. ( Thus, more grasping at straws. :) ) It's now working, with -DfailIfNoTests=false.

However, I'm finding that maven seems to rebuild everything on each run, even things having no changes (e.g. the layout algorithms), and it does this even if I omit clean. This is quite slow, so I may try to follow the setup guide you linked, so that I can run the tests inside of Eclipse. Thanks for the advice.

skieffer · 2024-08-29T13:54:48Z

I think this is ready for review, but I'd like to see how the updated docs pages will look. Is there an easy way to build the docs locally using maven?

skieffer · 2024-08-29T14:02:18Z

I don't know, whether you are familiar with Oomph setups for Eclipse.

Not really, but I followed this one to get the setup I'm using now. It seems similar to the one for the semantics repo (but maybe not as complete?).

Also, mention this in the docs.

skieffer · 2024-08-29T22:03:29Z

Okay, I added one more commit:

The container property is now written into edges only when using CONTAINER coordinate mode.
I think this is better, because in PARENT or ROOT mode it is just useless clutter.
The container property is now mentioned in the docs.

soerendomroes · 2024-09-02T06:25:40Z

I will try to review it till the end of the week, but I am currently not feeling well.

skieffer · 2024-09-02T11:24:02Z

Sorry to hear that! Take your time.

soerendomroes · 2024-09-06T06:30:17Z

I think this is ready for review, but I'd like to see how the updated docs pages will look. Is there an easy way to build the docs locally using maven?

The documentation for this can be found here.
hugo && hugo server in the docs folder after a normal maven build should do the trick.
The gitpod configuration of ELK should also directly build the website and start it.

soerendomroes · 2024-09-06T06:32:12Z

I don't know, whether you are familiar with Oomph setups for Eclipse.

Not really, but I followed this one to get the setup I'm using now. It seems similar to the one for the semantics repo (but maybe not as complete?).

Yes, this one "only" installs ELK, which is enough to develop. The one in the semantics repository will also install tooling for the elkt and elkj languages and actual programming languages together with stuff to start a language server or an Eclipse RCA.

skieffer · 2024-09-06T11:40:44Z

The documentation for this can be found here.
hugo && hugo server in the docs folder after a normal maven build should do the trick.

Thanks, that worked perfectly. Don't know how I missed that page before.

After reviewing the built docs, I think they basically look good, but of course I'll be interested to hear your feedback.

One question: I used italics for page links following the style of the link to "layout options" at the top of this page, but I don't know if that's actually the desired style for all internal links or not.

soerendomroes

This looks good. How can I verify this?

I am trying to build elk-live such that I can verify that the new a new locally build elkjs version does indeed solves the issue.

soerendomroes · 2024-09-06T13:15:47Z

Btw, I debug elk-live locally using the following setup:

Debug elkjs locally

elk, elkjs, elk-live in same directory
Build elkjs from elk, as described here

cd elkjs && npm install && npm run build

Add local elkjs in elk-live in

package.json "elkjs-local": "../../elkjs",

webpack.config.js const elkWorkerPathLocal = '../../elkjs/lib/elk-worker.min.js';

,
            new CopyWebpackPlugin([{
                from: elkWorkerPathLocal,
                to: 'elk-local'
            }])

json_template.html <option value="local">local</option>

Build and run elk-live (requires node 18).

cd elk-live/client && yarn install && yarn run build && cd ../server && ./gradlew jettyRun

localhost:8080 for running server

Select the local option for elkjs.

skieffer · 2024-09-06T14:59:46Z

How can I verify this?

I've opened kieler/elkjs#298, to share the drawing code I used to verify it during development.
Even if that PR is not accepted, you can download the files from it.

That one supports drawing in all of the new proposed JSON coordinate modes.

Another way to verify would be to try a graph in elk-live which has bad edges (I paste one below), and then check that, once you set

    "org.eclipse.elk.json.edgeCoords": "PARENT"

then the edges look good. Actually I have not tried this yet myself, but I think it should work, and we should check it before merging this.

{
    "id": "root",
    "properties": {
      "algorithm": "layered",
      "org.eclipse.elk.hierarchyHandling": "INCLUDE_CHILDREN"
    },
    "children": [
      { "id": "A",
        "children": [
          { "id": "x", "width": 50, "height": 90 },
          { "id": "B",
            "labels": [ { "text": "B", "width": 12, "height": 20 } ],
            "ports": [
              { "id": "p", "width": 10, "height": 10,
                "labels": [ { "text": "p", "width": 12, "height": 20 } ]
              }
            ],
            "children": [
              { "id": "y", "width": 50, "height": 90 },
              { "id": "z", "width": 50, "height": 90 }
            ],
            "edges": [
              { "id": "e1", "sources": [ "y" ], "targets": [ "z" ] },
              { "id": "e2", "sources": [ "x" ], "targets": [ "z" ],
                "labels": [ { "text": "e2", "width": 22, "height": 20 } ]
              },
              { "id": "e3", "sources": [ "x" ], "targets": [ "p" ] },
              { "id": "e4", "sources": [ "p" ], "targets": [ "y" ] }
            ]
          }
        ]
      }
    ]
  }

skieffer · 2024-09-06T15:01:43Z

Btw, I debug elk-live locally using the following setup:

Thanks. I plan to give this a shot. I tried building elk-live locally before, but ran into some errors and gave up. I'll let you know if I have problems again.

soerendomroes · 2024-10-17T07:23:09Z

@skieffer Do you need any pointers to elk-live? I guess it would be best to add a switch to the transformation from the elk json graph to sprotty?

skieffer · 2024-10-18T19:44:48Z

Do you need any pointers to elk-live?

Hey @soerendomroes , thanks for checking in. I'm out of office this week, but hope to look at this again next week. I'll let you know how it goes with elk-live!

skieffer · 2024-11-24T19:39:44Z

Hi @soerendomroes , well it's over a month later, but I finally found some time to work on this.
I followed your instructions for building elk-live with a "local" elkjs option, and I got through the yarn build just fine.
However, the ./gradlew run failed, as follows. What do I need to do?

$ ./gradlew jettyRun
Starting a Gradle Daemon (subsequent builds will be faster)
Registering ELK version 0.9.1.
Registering ELK version 0.9.0.
Registering ELK version 0.8.1.
Registering ELK version 0.8.0.
Registering ELK version 0.7.1.
Registering ELK version 0.7.0.
Registering ELK version 0.6.1.
Registering ELK version 0.6.0.
Registering ELK version 0.5.0.
Registering ELK version 0.4.1.
Registering ELK version 0.4.0.
Registering ELK version 0.3.0.

FAILURE: Build failed with an exception.

* Where:
Build file '/Users/skieffer/elk-master/git/elk-live/server/elk-layout-version/build.gradle' line: 47

* What went wrong:
A problem occurred evaluating project ':elk-layout-version'.
> Could not resolve all files for configuration ':elk-layout-version:runtimeClasspath'.
   > Could not find org.eclipse.elk:org.eclipse.elk.core:0.9.0-SNAPSHOT.
     Searched in the following locations:
       - https://repo.maven.apache.org/maven2/org/eclipse/elk/org.eclipse.elk.core/0.9.0-SNAPSHOT/maven-metadata.xml
       - https://repo.maven.apache.org/maven2/org/eclipse/elk/org.eclipse.elk.core/0.9.0-SNAPSHOT/org.eclipse.elk.core-0.9.0-SNAPSHOT.pom
       - https://repo1.maven.org/maven2/org/eclipse/elk/org.eclipse.elk.core/0.9.0-SNAPSHOT/maven-metadata.xml
       - https://repo1.maven.org/maven2/org/eclipse/elk/org.eclipse.elk.core/0.9.0-SNAPSHOT/org.eclipse.elk.core-0.9.0-SNAPSHOT.pom
       - https://oss.sonatype.org/content/repositories/snapshots/org/eclipse/elk/org.eclipse.elk.core/0.9.0-SNAPSHOT/maven-metadata.xml
       - https://oss.sonatype.org/content/repositories/snapshots/org/eclipse/elk/org.eclipse.elk.core/0.9.0-SNAPSHOT/org.eclipse.elk.core-0.9.0-SNAPSHOT.pom
     Required by:
         project :elk-layout-version

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 12s

soerendomroes · 2024-11-24T19:44:11Z

@skieffer This looks like something we fixed on the main branch. Can you merge main?

soerendomroes · 2024-11-24T19:46:48Z

The issue is that snapshot 0.9.0 no longer exists since we released elk 0.9.0. Once you merge the main branch the build should use the 0.10.0 snapshot.

skieffer · 2024-11-24T21:51:32Z

Thanks! Merging solved it.

Okay, I have tried out the example graph from above, and the results are looking good.

If we allow the default CONTAINER edge coordinates to be used, then the drawing looks as bad as expected:

but as soon as we add the

"org.eclipse.elk.json.edgeCoords": "PARENT"

setting, then it looks good:

So that's a nice verification that this PR is working as intended.

skieffer · 2024-11-24T21:54:05Z

So I think this is about ready, but I'd like to first prepare a PR over on the elk-live repo to integrate this, before this one is merged. That way if I discover any usability issues in the process I can still fix them.

I hope to be able to work on that this week (time permitting).

skieffer · 2024-11-24T23:03:57Z

Okay, maybe it was easier than I thought. I've opened kieler/elk-live#94 .

skieffer added 12 commits August 24, 2024 10:08

Target platform changes?

915fa24

Update .gitignore

d07e112

Supply missing map clear

d34db90

Add comments

3e63089

Add org.eclipse.elk.json.edgeCoords property

7208ee3

For now, we don't do anything with it except note the enum ordinal in the JSON for each edge.

Adjust edge coordinates according to mode

4d49d8b

Use extension methods

4f76fa0

Add unit tests

3683032

Use do-while loop

0300a97

Remove diagnostic output

f72a435

Revert incidental changes

6d152c9