Provide JSON coordinate options

[skieffer]

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).

[skieffer]

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]

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.

[soerendomroes]

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.

[skieffer]

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]

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]

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:

elk/test/org.eclipse.elk.alg.test/src/org/eclipse/elk/alg/test/PlainJavaInitialization.java

Line 15 in ad04950

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

and here:

elk/test/org.eclipse.elk.shared.test/src/org/eclipse/elk/shared/test/ElkReflectTest.java

Line 17 in ad04950

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

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]

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.

[skieffer]

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]

As for the Layout algorithm 'layered' not found for Root Node error, I have to revoke my comment about it being perhaps related to execution inside a Docker container, as I am now getting that error even on my machine, when I omit the PlainJavaInitialization fix.

So perhaps its nonappearance was just some artifact of my broken testing setup.

[skieffer]

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

Okay, I'll add that.

[skieffer]

If we support ROOT for edges, we might also have to do this for nodes, ports, and node labels too. If implementing the ROOT option 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.

I think it should be easy to provide ROOT coords for everything, and it could be included in this PR. I'll aim to work on this soon (hopefully this week).

[soerendomroes]

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.

I don't know, whether you are familiar with Oomph setups for Eclipse. If this is not the case and your initial try fails, please do not refrain from asking questions and depending on our schedule (and mine) it might be helpful to have a short meeting regarding this since version used during the setup change frequently and one can easily spend an hour achieving nothing.

I am currently using an "Eclipse IDE for Java and DSL development" with product version 2023-12 and a Java 17 as a base and use the KIELER Semantics Oomph setup an the "ELK" stream with target platform set to None.

[skieffer]

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]

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?).

[skieffer]

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]

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

[skieffer]

Sorry to hear that! Take your time.

[soerendomroes]

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]

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]

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]

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]

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]

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]

@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]

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!