Skip to content

Diagram editor #99

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

Open
wants to merge 174 commits into
base: main
Choose a base branch
from
Open

Diagram editor #99

wants to merge 174 commits into from

Conversation

koonpeng
Copy link
Collaborator

@koonpeng koonpeng commented Aug 8, 2025

New feature implementation

Implemented feature

This still needs more testing before I would consider it stable, but I thought this is a good time to merge to main.

Even though the change is huge, most of the changes is in diagram-editor so there shouldn't be much conflicts.

GenAI Use

We follow OSRA's policy on GenAI tools

  • I used a GenAI tool in this PR.
  • I did not use GenAI

Generated-by: Gemini

koonpeng added 30 commits May 16, 2025 06:04
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
@koonpeng koonpeng force-pushed the koonpeng/diagram-editor branch 4 times, most recently from 2ba3061 to 911c29f Compare August 12, 2025 02:13
Signed-off-by: Teo Koon Peng <[email protected]>
@koonpeng koonpeng force-pushed the koonpeng/diagram-editor branch from 911c29f to 9397cf3 Compare August 12, 2025 02:41
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Copy link
Contributor

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

I'm very excited for bevy impulse to have a functional visual editor! It looks like ReactFlow was a good choice, and I look forward to seeing how far we can go with it.

I don't think we have to be very strict on the first draft of the editor, but I'm seeing a few bugs in the UI that I think are likely to cause significant confusion for users in its current state. I think we should get those sorted out before merging this to main where users may feel invited to play around with it.

I've described the bugs below, and included a video for most of them. The last section is some remarks on non-critical improvements that I think we should follow up on if we don't resolve them within this PR.

Discrepancies between the displayed state and the actual state

For nodes, I see strange things happen with the config field. In the following video I have two nodes that use the mul builder. I assign one of them a config of 10 and the other a config of 100. Clicking back and forth between them, eventually they both show a config of 10, even though I never changed the one that was supposed to be 100:

mul_bug.mp4

I've seen other cases where I change the config of a node and then immediately re-click the node to find that its config did not seem to change at all.

I see similar problems with connection settings. When trying to set up a split operation, the UI doesn't respond to changes in what I select. Clicking away and re-selecting the edge allows it to update somewhat, but you can see that the sequence value never seems to update in the edit box, even though it shows correctly as 1 on the board:

split_bug.mp4

Confusing Join input

I made a test case that uses the Join operation and found that it was giving me the wrong result. It wasn't an error message; instead an incorrect number was being calculated. After inspecting the generated diagram json I noticed that only one buffer was being connected into the join even though the UI showed two connections into it. The problem is both connections were both set to Index 0, so one of them was being quietly discarded.

I understand if it's too tricky to automatically assign new connections to a unique index/key, but I would recommend these two changes to help users deal with this:

  • Have the connection line display the index/key of the buffer inside the join, similar to how connections coming out of Split display their sequence number.
  • Before allowing the workflow to be run or the json to be generated, check that there are no duplicate entries for Join and pop out an error if there are. We should do the same for Fork Result.

Unable to choose Key for Join connection

The UI for setting a string key for Join doesn't work. Without that we won't be able to join messages into structs; we'll only be able to join into arrays and tuples.

join_key_bug.mp4

Non-urgent quality of life improvements

There are few things that I think could improve the quality of life, although I don't consider these urgent. We can open follow-up tickets for these like #102.

  • For operations like Split and Fork Clone that support multiple outputs, use a different icon for the output slot, maybe something like a small cluster of 3 or 4 circles instead of one. Fork Result could have exactly two output circles since it only supports two outputs.
  • I like that buffer "outputs" and join/listen "inputs" have the same color to indicate that they can be connected to each other. I think we should do the same for ordinary inputs and outputs. I don't think it makes sense that ordinary outputs are white and ordinary inputs are orange when they can connect to each other. If we want to distinguish inputs from outputs then maybe output icons can be little downward pointing triangles instead of circles.
  • The icons for Fork Result and Split should be flipped upside-down. I would also recommend using a question mark in some way for Fork Result as a reference to its similarity to the Rust question mark operator.

Install the dependencies:

```bash
pnpm install
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's include the instructions for setting up pnpm like what we have for rmf-web. Our target user base is the ROS community, who often have no experience using web tools like pnpm.

mxgrey and others added 10 commits August 17, 2025 17:50
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
@koonpeng
Copy link
Collaborator Author

Discrepancies between the displayed state and the actual state

Turns out this is due to reactflow only returning the new values on the next render, fixed by avoiding use of reactflow's state.

Confusing Join input

Added labels to buffer edges.

Unable to choose Key for Join connection

fixed

The icons for Fork Result and Split should be flipped upside-down. I would also recommend using a question mark in some way for Fork Result as a reference to its similarity to the Rust question mark operator.

updated the icons.

I like that buffer "outputs" and join/listen "inputs" have the same color to indicate that they can be connected to each other. I think we should do the same for ordinary inputs and outputs. I don't think it makes sense that ordinary outputs are white and ordinary inputs are orange when they can connect to each other. If we want to distinguish inputs from outputs then maybe output icons can be little downward pointing triangles instead of circles.

Which operation did you see the inputs to be orange? Both ordinary inputs and outputs should be white-ish. Orange is for stream outputs.

@mxgrey
Copy link
Contributor

mxgrey commented Aug 18, 2025

Which operation did you see the inputs to be orange? Both ordinary inputs and outputs should be white-ish. Orange is for stream outputs.

In the videos you can see that mul has an orange output icon, but it doesn't produce any streams.

Would ReactFlow be able to support having streams come out of the side (maybe right side) of the box instead of the bottom?

Signed-off-by: Teo Koon Peng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

2 participants