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

GUI #10

Merged
merged 32 commits into from
Nov 6, 2024
Merged

GUI #10

merged 32 commits into from
Nov 6, 2024

Conversation

ll-nick
Copy link
Collaborator

@ll-nick ll-nick commented May 27, 2024

(Actual draft PRs are a paid feature so the title will have to do)

Adds @orzechow fancy GUI web app to the repo and the demo while replacing the ROS based communication with something more generic, probably web sockets.

@ll-nick ll-nick self-assigned this May 27, 2024
@orzechow
Copy link
Member

The C++ lib I was using for an HTTP Rest API was cpp-httplib

What I like: It has a small footprint and lean C++ interface.
Server-side events might provide a similar experience to WebSockets, though I have no experience on these.

Ubuntu 22.04 provides an (of course a bit outdated) apt package: libcpp-httplib-dev

@orzechow orzechow changed the base branch from pacman-demo to entt_interface June 3, 2024 14:40
Base automatically changed from entt_interface to pacman-demo June 4, 2024 09:11
@orzechow orzechow marked this pull request as draft October 4, 2024 13:49
@orzechow orzechow changed the title Draft: GUI GUI Oct 4, 2024
@orzechow orzechow mentioned this pull request Oct 14, 2024
Base automatically changed from pacman-demo to main October 31, 2024 10:17
@orzechow orzechow force-pushed the gui branch 5 times, most recently from fb77cc9 to 1f8f7c8 Compare November 4, 2024 22:39
@orzechow orzechow changed the base branch from main to modernize_cmake November 4, 2024 23:24
@orzechow
Copy link
Member

orzechow commented Nov 4, 2024

@ll-nick Alright, the GUI is ready for review 🎉

I moved all CMake cleanup stuff into #68 to separate concerns a bit.

pacman_arbitrators_vp9.webm

@orzechow orzechow marked this pull request as ready for review November 4, 2024 23:32
Base automatically changed from modernize_cmake to main November 5, 2024 10:14
Copy link
Collaborator Author

@ll-nick ll-nick left a comment

Choose a reason for hiding this comment

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

Very, very nice, thanks for all the hard work!

This is a very nice and important feature and it's integrated really well. Most complaints I have are minor (and since I'm neither a JS guy nor the CMake expert between the two of us you probably know better anyway ;-) ).
The one thing I'd wish for if it is feasible without too much overhead would be the ability to configure the port of the vue app.

Also, you should update this line in the current README:

We will shortly add an [arbitration graph GUI](https://github.com/KIT-MRT/arbitration_graphs/pull/10) and a [tutorial](https://github.com/KIT-MRT/arbitration_graphs/pull/51) based on this demo – stay tuned! 

I opened the PR, so you'll have to approve it yourself.

Comment on lines +112 to +123
optionRx: function (options) {
if (options == null || options === undefined || options.length == 0) {
// This component is a behavior option → rounded edges
return 10;
} else {
// This component is an arbitrator → square edges
return 0;
}
},
optionRy: function () {
return this.optionRx();
},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As a Firefox user, I appreciate you making it Firefox compatible 🔥 🦊

However, the colors still seem pretty off on Firefox, it looks much better on Chromium.
gui_colors

Copy link
Member

Choose a reason for hiding this comment

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

Weird, which Firefox version do you use? 🤔
The video above has been recorded from my Firefox 131.0.3

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm, weird. I use 132.0.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, after updating to 132.0.1 I still don't see this issue…

orzechow and others added 7 commits November 5, 2024 13:42

Verified

This commit was signed with the committer’s verified signature.
Wauplin Lucain

Verified

This commit was signed with the committer’s verified signature.
Wauplin Lucain
@orzechow
Copy link
Member

orzechow commented Nov 5, 2024

Rebased onto main

@orzechow
Copy link
Member

orzechow commented Nov 5, 2024

Alright – now, that we don't compile the static files path into an object file, but pass it as pre-compiled header (5bc50c9),
I gave it a shot to turn the GUI into a header-only lib as well: 3f1fa33 🎉

@ll-nick Please test this thoroughly on your machine (remember to disable or even prune docker caches)

Also, I figured that the demo tests didn't compile anymore: 8ed4e40

As soon as you're happy with these changes, we can merge 😄

@ll-nick
Copy link
Collaborator Author

ll-nick commented Nov 6, 2024

Alrighty, I tested everything I believe

  • Building the library docker
  • Building the devel docker and run the unit tests
  • Build and run the demo
  • Build the tutorial and run the unit tests

Everything works flawlessly except for the colors of the GUI being off for me. If it works for you, I'd merge anyway and look into this in a separate issue or something. Maybe I also just need to delete some browser cache or similar.

Copy link
Member

@orzechow orzechow left a comment

Choose a reason for hiding this comment

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

Approving my own stuff feels weird 🙈

@orzechow orzechow merged commit a923d0a into main Nov 6, 2024
@orzechow orzechow deleted the gui branch November 6, 2024 10:09
@ll-nick
Copy link
Collaborator Author

ll-nick commented Nov 7, 2024

Approving my own stuff feels weird 🙈

image

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

Successfully merging this pull request may close these issues.

None yet

2 participants