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

Updated egui + wgpu + winit proof-of-concept integration #763

Merged
merged 2 commits into from
Jul 7, 2022

Conversation

follower
Copy link
Contributor

@follower follower commented Jul 1, 2022

Here's the updated egui + wgpu + winit proof-of-concept integration for Fornjot, since I figured out & fixed what was causing the issue I ran into when I was working on it a month ago.

GH seems to think it's merge-able with current main.

Feel free to use/modify as you see fit--no idea when I'll be through next but I did at least want to follow through on my intention this far & hope it's helpful to Fornjot's progress in some way.

@hannobraun
Copy link
Owner

Thank you, @follower, this is great work! I really appreciate that you followed through and didn't leave me hanging with a tantalizing but unmergable pull request 😄

I've taken a quick look at the code, and aside from the minor Clippy/rustfmt stuff, the only real problem I see is that it re-adds a dependency on winit to fj-viewer. fj-window was extracted out of fj-viewer with the specific intent to break that dependency, and enable use of fj-viewer with other windowing solutions. To be clear, this is not a criticism of your work. I understand that this is a proof of concept, and such a limitation is perfectly acceptable. I'm just explaining the context to you, or anyone else who comes across this pull request and might be interested.

I'm actually planning to publish a new Fornjot release, and I'll start looking into that on Monday (might take me a bit to actually do it; there are some problems with the release automation). Right after that, I want to get this merged. My first impression is, that it shouldn't be too hard to reshuffle things a bit. I might either just do that, or merge it as-is and open an issue to track the unwanted dependency. Either way, we'll get a GUI that's much more capable than what we have now, and can iterate on that further.

Until this pull request is merged, I will hold off on merging any other pull requests that could cause conflicts with it.

follower added 2 commits July 7, 2022 18:26
This commit is authored by @follower, with some minor modifications by
@hannobraun (squashing a lot of commits into one,  running `cargo fmt`,
and auto-applying Clippy suggestions).

This commit re-adds `fj-viewer`'s dependency on winit, indirectly via
`egui`'s winit integration. This is not desirable, but it is acceptable
for now. Getting a working egui integration is worth more.

@hannobraun will open an issue after merging this to track this problem.
@hannobraun hannobraun force-pushed the egui-wgpu-winit-integration branch from cf319d6 to 0415830 Compare July 7, 2022 16:37
@hannobraun hannobraun marked this pull request as ready for review July 7, 2022 16:37
@hannobraun hannobraun self-requested a review as a code owner July 7, 2022 16:37
@hannobraun
Copy link
Owner

Okay, I've squashed the commits to make everything a bit more manageable, and made some small (and automated) changes to the bigger commit that adds all the code, as described in the commit message.

CI should pass now, and I'm happy with merging it in this state. I'll open follow-up issues once I have merged it.

Thanks again, @follower! This should provide a nice base for future improvements.

@hannobraun hannobraun enabled auto-merge July 7, 2022 16:41
@hannobraun hannobraun merged commit d6df4af into hannobraun:main Jul 7, 2022
This was referenced Jul 7, 2022
@hannobraun
Copy link
Owner

I've opened these follow-up issues:

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