skieffer commented 3 months ago

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

[x] 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.
[x] 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 #1012 .
[x] Docs should be updated, including page on coordinate system.

Tasks for another PR (in another repo)

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

skieffer commented 3 months ago

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 commented 3 months ago

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 theROOToption for everything seems too complicated, I think that it is also reasonable to only implementPARENT` for edges as a first PR that could be merged.

soerendomroes commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

Okay, I'll add that.

skieffer commented 3 months ago

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 commented 3 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

skieffer commented 2 months ago

Sorry to hear that! Take your time.

soerendomroes commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

How can I verify this?

I've opened https://github.com/kieler/elkjs/pull/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 commented 2 months ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 2 days ago

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 commented 2 days ago

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

soerendomroes commented 2 days ago

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 commented 2 days ago

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:

Screenshot 2024-11-24 at 16 44 00

but as soon as we add the

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

setting, then it looks good:

Screenshot 2024-11-24 at 16 44 15

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

skieffer commented 2 days ago

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 commented 2 days ago

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

eclipse / elk

Provide JSON coordinate options #1071

First new property

Second new property

Summary

Questions / Tasks for this PR

Tasks for another PR (in another repo)

Debug elkjs locally