Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide JSON coordinate options #1071

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

Conversation

skieffer
Copy link
Contributor

@skieffer skieffer commented Aug 25, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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 and others added 2 commits August 26, 2024 12:03
(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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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:

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

and here:

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
Copy link
Contributor

soerendomroes commented Aug 27, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

Okay, I'll add that.

@skieffer
Copy link
Contributor Author

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
Copy link
Contributor

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 skieffer changed the title Provide edge coordinate options Provide JSON coordinate options Aug 29, 2024
@skieffer skieffer marked this pull request as ready for review August 29, 2024 13:53
@skieffer
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

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

@skieffer
Copy link
Contributor Author

skieffer commented Sep 2, 2024

Sorry to hear that! Take your time.

@soerendomroes
Copy link
Contributor

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
Copy link
Contributor

soerendomroes commented Sep 6, 2024

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
Copy link
Contributor Author

skieffer commented Sep 6, 2024

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.

Copy link
Contributor

@soerendomroes soerendomroes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

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
Copy link
Contributor Author

skieffer commented Sep 6, 2024

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
Copy link
Contributor Author

skieffer commented Sep 6, 2024

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
Copy link
Contributor

@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
Copy link
Contributor Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants