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

Proof of concept egui integration. #642

Closed
wants to merge 34 commits into from

Conversation

follower
Copy link
Contributor

[TODO: Add some context. ]

follower added 30 commits May 26, 2022 08:39
This is sufficient for compilation to succeed but doesn't enable
any visible functionality yet.
Note: Doesn't yet create or render anything visible.

      Doesn't yet process any input events.

Also includes some notes on the process/references used
for the implementation.
This should now be visible but won't respond to input.

The existing UI should still respond to input (e.g. keypress, mouse-).
(Originally removed while troubleshooting unintentional
managed texture 0 dropping.)
Used by future commit.

Encapsulation? Coupling? Never heard of 'em :D
(To match the existing UI toggles.)
Code in formatting function is copied from `config_ui.rs`.
...I mean, what else are GUIs for, right?
(Since we're currently passing `window` for other reasons, we might
as well make use of it here too.)
This displays arbitrary text alongside the file/line reference
of the macro call.

It's particularly useful for determining layout-related values without
spamming stdout or logs every frame.
Works better than default wrapping.
Is a floating window preferable for any reason?
This handles changing label text/widths better IMO.
The new name is of a length that avoids code automated reformatting.
@hannobraun
Copy link
Owner

Thank you, @follower, this is awesome!

Do you think we can start merging this soon? I haven't properly reviewed this yet, but the changes look really reasonable. And the GUI itself looks great. A bit overloaded maybe, with all the egui debug stuff, but a great base to build upon.

I think the main problem might be, that this is based on a pretty old (~2 months) version of the main branch. I don't think the graphics code itself has changed much since then, but it has moved into another crate (fj-viewer), and everything winit-related has been extracted from that recently (into fj-window). So this will require some shuffling around, before it can be merged.

@hannobraun hannobraun mentioned this pull request Jun 2, 2022
@follower
Copy link
Contributor Author

*sigh* (That's an existential sigh, not one directed at you...)

TL;DR: Yeah, I was aware of it being an outdated version, but as the computer I'm using is not ever likely to be mistaken for "what peak performance looks like", I wanted to avoid a re-compilation stage of unknown duration in order to update at that point.

Anyway, I had an updated (to May 26th) almost working version about a month ago but ran into a couple of issues & life.

But tonight I managed to fix/workaround the issues and got egui @ v0.18.0ish and main as of c85c096 working/hacked together:

Screenshot from 2022-06-30 21-07-52

Screenshot from 2022-06-30 21-08-09

And, yeah, that specific UI was mostly included for pedagogical reasons.

Architectural question

Also, while I have something working I also realised that in order to know how to proceed it would be good to clarify the architecture you're wanting because currently the "integration" of egui with the existing fj-app/viewer is somewhat "reinventing the wheel" when we could use eframe instead & just worry about the application UX & geometry functionality.

Essentially the three options are:

  • An "integration" of egui into fj-app so fj-app is the base of it all & egui is essentially only UI.
  • Use the eframe framework to handle I/O etc but still only layer the GUI over a window that fj-app controls/draws.
  • Lean fully into the egui world & just have Fornjot render onto a graphics layer when egui provides one in a callback.

Assuming you still want to continue down this road, what would be your preference?

@hannobraun
Copy link
Owner

Anyway, I had an updated (to May 26th) almost working version about a month ago but ran into a couple of issues & life.

But tonight I managed to fix/workaround the issues and got egui @ v0.18.0ish and main as of c85c096 working/hacked together:

That's great, @follower! Thank you for your continued work on this!

There haven't been many changes to fj-window/fj-viewer lately, so c85c096 should be plenty current. I expect minimal to no changes going from there to main.

Could you push your latest branch? I'd love to take a look and get it merged as soon as possible.

Also, while I have something working I also realised that in order to know how to proceed it would be good to clarify the architecture you're wanting because currently the "integration" of egui with the existing fj-app/viewer is somewhat "reinventing the wheel" when we could use eframe instead & just worry about the application UX & geometry functionality.

At this point, I have no problem with reinventing the wheel. You have a working egui integration, so let's get that merged quickly, so it doesn't get out of date again. Whether it's using the right architecture or not, I'm sure it's a better basis for further iteration than not having any egui integration at all.

Essentially the three options are:

  • An "integration" of egui into fj-app so fj-app is the base of it all & egui is essentially only UI.
  • Use the eframe framework to handle I/O etc but still only layer the GUI over a window that fj-app controls/draws.
  • Lean fully into the egui world & just have Fornjot render onto a graphics layer when egui provides one in a callback.

Assuming you still want to continue down this road, what would be your preference?

I have no idea what the best long-term solution is, but right now option 1 seems like the lowest risk to me (with the caveat that it shouldn't get integrated into fj-app, but fj-viewer/fj-window; but that probably was just a typo).

Since I have no experience with either egui or eframe, I can't judge whether it's a good idea to bind ourselves more closely to it. It might turn out well, but it sounds to me like getting married at your first date. Since the "reinvent the wheel" implementation is what you already have, we should just merge that and use it to gather experiences. We'll be in a better position to make a less temporary decision some time down the line.

There are also other considerations, like I'd like to replace our custom rendering code with an external library, if that is practical (#657). I don't know how that will affect our options.

@follower
Copy link
Contributor Author

follower commented Jul 1, 2022

Closing in favour: of #763

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.

2 participants