Skip to content

Conversation

@tobiasholenstein
Copy link
Member

@tobiasholenstein tobiasholenstein commented Nov 28, 2024

This PR depends on #22402 . To check out this PR locally:
git fetch https://git.openjdk.org/jdk.git pull/22430/head:pull/22430
git checkout pull/22430

This pull request enhances the Ideal Graph Visualizer (IGV) by introducing an interactive feature that allows users to move nodes within the hierarchical layout by dragging them to new positions. This manual adjustment helps users better understand and explore the graph structure by customizing the layout according to their needs.

Overview

Previously, the hierarchical layout in IGV was static, and users could not adjust node positions manually. This limitation made it challenging to reorganize the graph for improved readability or to focus on specific areas of interest. With this enhancement is new:

  • Interactive Node Movement: Users can now click and drag nodes to new positions within the graph.
  • Dynamic Edge Adjustment: When nodes are moved, connected edges adjust dynamically to maintain the graph's structure.
  • Layer Management: Nodes can be moved within the same layer or across different layers, with the layout updating accordingly.
  • Persistent Positions: Moved nodes remain in their new positions until the layout is reset or the nodes are moved again.

Limitations

  • Interactive Node Moving only works in Sea of nodes view with cut long edges off. (The standard option for IGV)
  • Currently, no empty layers are allowed in the layout, so users cannot introduce a horizontal gap between nodes that only contains edges.
  • To move long straight edges, it's best to drag the top part of the edges around.
    When the graph changes - for example, when nodes are removed or hidden, or layers are applied - the rearrangements are lost since the graph gets re-laid out. To preserve rearrangements, support for a stable incremental layout algorithm would be needed.

Main Changes

LayoutMover Interface

Created a new interface LayoutMover with methods moveVertex, moveVertices, and moveLink.
HierarchicalLayoutManager now implements LayoutMover, providing concrete implementations for these methods.

Enhancements to HierarchicalLayoutManager

Improved the HierarchicalLayoutManager so it can handle moving nodes interactively. Added methods to move single nodes or multiple nodes, and to adjust links. Nodes can now be moved within the same layer or to different layers while keeping the graph consistent. Also added a writeBack method to apply these changes.

Modifications to LayoutGraph

Updated LayoutGraph to support changes when nodes are moved. Added methods to manage layers, like creating and deleting layers, inserting new ones when needed, and removing empty layers. Also added ways to handle nodes and edges, like removing them or adding edges with dummy nodes if necessary. Improved how edges are handled to reduce crossings and straighten them, and made better algorithms to position nodes and minimize edge crossings.

User Interface Enhancements in DiagramScene

Enhanced the user interface to let users move nodes and edges by dragging them. Added visual feedback like shadows and pointers when moving nodes. We updated the actions for FigureWidget to include move actions, making nodes draggable. We also adjusted the methods that rebuild the layout to handle these dynamic changes.

FigureWidget and LineWidget

Made changes so that when nodes and edges are moved, the UI components update accordingly. In FigureWidget, added an updatePosition method to update its position when the node moves. In LineWidget, we made the start and end points editable so edges can adjust when nodes move, and added methods to support moving edges interactively.

Examples

1. Moving a node to a different layer

1

2. Moving a node within the layer to a different position

2

3. Moving the position where an edge crosses a layer

3

Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8343705: IGV: Interactive Node Moving in Hierarchical Layout (Enhancement - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22430/head:pull/22430
$ git checkout pull/22430

Update a local copy of the PR:
$ git checkout pull/22430
$ git pull https://git.openjdk.org/jdk.git pull/22430/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22430

View PR using the GUI difftool:
$ git pr show -t 22430

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22430.diff

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 28, 2024

👋 Welcome back tholenstein! A progress list of the required criteria for merging this PR into pr/22402 will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Nov 28, 2024

@tobiasholenstein This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8343705: IGV: Interactive Node Moving in Hierarchical Layout

Reviewed-by: chagedorn, thartmann, rcastanedalo

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk
Copy link

openjdk bot commented Nov 28, 2024

@tobiasholenstein The following label will be automatically applied to this pull request:

  • hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Nov 28, 2024
@openjdk
Copy link

openjdk bot commented Nov 28, 2024

@tobiasholenstein this pull request can not be integrated into pr/22402 due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8343705
git fetch https://git.openjdk.org/jdk.git pr/22402
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge pr/22402"
git push

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Nov 28, 2024
@tobiasholenstein tobiasholenstein marked this pull request as ready for review November 28, 2024 12:40
@openjdk openjdk bot added rfr Pull request is ready for review and removed merge-conflict Pull request has merge conflict with target branch labels Nov 28, 2024
@mlbridge
Copy link

mlbridge bot commented Nov 28, 2024

Webrevs

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Choose a reason for hiding this comment

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

I thoroughly tested this with a few complex graphs and it works great. This is an awesome enhancement!

Copy link
Contributor

@robcasloz robcasloz left a comment

Choose a reason for hiding this comment

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

I tested and benchmarked this change using the same methodology as in JDK-8314512. Both testing and performance results are OK. The change seems to introduce a slight overhead of around 10% w.r.t. JDK-8314512 but still speeds up the baseline (current IGV) by around 1.4x thanks to disabling assertions in JDK-8314512. See full results here: results.ods. I will start to review the actual code changes.

Copy link
Contributor

@robcasloz robcasloz left a comment

Choose a reason for hiding this comment

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

I re-tested this change after merging the latest changes from #22402 and did not find any issue.

@openjdk-notifier openjdk-notifier bot changed the base branch from pr/22402 to master November 29, 2024 13:56
@openjdk-notifier
Copy link

The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork:

git checkout JDK-8343705
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk
Copy link

openjdk bot commented Nov 29, 2024

⚠️ @tobiasholenstein This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Nov 29, 2024
@openjdk openjdk bot removed merge-conflict Pull request has merge conflict with target branch rfr Pull request is ready for review labels Nov 29, 2024
@openjdk openjdk bot added the rfr Pull request is ready for review label Nov 29, 2024
Copy link
Member

@chhagedorn chhagedorn left a comment

Choose a reason for hiding this comment

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

Great work! This is a really nice feature. I've tested it on Linux and it works as expected.

…a/com/sun/hotspot/igv/hierarchicallayout/LayoutGraph.java

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>
@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 29, 2024
@tobiasholenstein
Copy link
Member Author

Thanks @robcasloz , @TobiHartmann and @chhagedorn for the reviews!

/integrate

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Nov 29, 2024
@openjdk
Copy link

openjdk bot commented Nov 29, 2024

@tobiasholenstein This pull request has not yet been marked as ready for integration.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 29, 2024
@tobiasholenstein
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Nov 29, 2024

Going to push as commit 28b0f3e.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 29, 2024
@openjdk openjdk bot closed this Nov 29, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 29, 2024
@openjdk
Copy link

openjdk bot commented Nov 29, 2024

@tobiasholenstein Pushed as commit 28b0f3e.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated

Development

Successfully merging this pull request may close these issues.

4 participants