Refactor fj-viewer into multiple crates. #584
Conversation
|
Thank you for the pull request, @freylint! I'll take a look on Monday. |
|
Update: I don't think I will get to this today. Tomorrow should work though. |
There was a problem hiding this comment.
Thanks again for the pull request, @freylint!
I've left a bunch of comments where I noticed problems or had suggestions. In addition, I have some big-picture/architectural concerns with the way the new crates are set up:
- All of the new group of crates depend on
fj-gui, meaning they all still depend on winit. My understanding is that the primary motivation for this change would be to allow for non-winit windowing libraries to be used, and this goal is not achieved. fj-guionly contains the window abstraction, which is a bit thin. I don't think separating this from the event-handling code left infj-viewermakes sense. When would you want to use one, but not the other?- It's a bit weird that
fj-inputdepends onfj-gfx, which has all the graphics and UI code. Not sure if splittingCamerainto its own crate would be any better though. I don't know, it's probably fine for now.
We definitely need something like this, to make fj-viewer more modular and allow for more flexible use, but I don't think this pull request is it.
While in general, I'd be happy to merge changes that take things in the right direction and iterate on them later, the situation here is a bit different. I'd like to avoid releasing crates that we are going to rename or abandon afterwards, where possible. So merging this as-is would essentially block any release for the time being, which I also don't want.
So this will have to be a bit more solid, before it's ready to be merged. Maybe a more focused PR that only extracts one new crate (as I suggested in #572) would be a more suitable first step.
| "crates/fj-interop", | ||
| "crates/fj-kernel", |
There was a problem hiding this comment.
Could you modify the commit to not remove this? I realize you're adding it back in a later commit, but having it removed for a few commits could mess up any git bisect over that commit range.
| @@ -8,6 +8,7 @@ members = [ | |||
| "crates/fj-host", | |||
| "crates/fj-input", | |||
| "crates/fj-interop", | |||
| "crates/fj-kernel", | |||
There was a problem hiding this comment.
The new crate is not being added to the workspace.
| [features] | ||
| default = [] | ||
| gfx = [] |
There was a problem hiding this comment.
What are those features about? I don't think they're used anywhere.
| gfx = [] | ||
|
|
||
| [dependencies] | ||
| winit = "0.26.1" |
There was a problem hiding this comment.
I think fj-input needs to eventually be independent from winit, for the separation to make sense, but this is fine for a start.
| @@ -0,0 +1,22 @@ | |||
| [package] | |||
| name = "fj-gfx" | |||
There was a problem hiding this comment.
I think I'd prefer the name fj-graphics. It's still short enough, and it doesn't have the connotation of being related to gfx-rs, except that it happens to use their library, which should be an internal implementation detail.
Happy to merge it like this, so no change is necessary, but I might adjust this before the next release.
| @@ -5,8 +5,6 @@ use fj_interop::mesh::Mesh; | |||
| use fj_math::{Aabb, Point, Scalar, Transform, Triangle, Vector}; | |||
| use winit::dpi::PhysicalPosition; | |||
|
|
|||
| use crate::window::Window; | |||
There was a problem hiding this comment.
Won't this lead to a compile error, as Window is still used below? I realize you add it back later, but it would be much nicer if every single commit compiled (again, due to any git bisect that might be necessary later).
Might not be worth changing it though, as I think that would require a whole different order of extracting things.
| @@ -7,7 +7,7 @@ use wgpu::util::DeviceExt as _; | |||
| use wgpu_glyph::ab_glyph::InvalidFont; | |||
| use winit::dpi::PhysicalSize; | |||
|
|
|||
| use crate::{camera::Camera, window::Window}; | |||
There was a problem hiding this comment.
Same as above. Seems to introduce a build error.
| @@ -5,6 +5,7 @@ | |||
|
|
|||
| use std::time::Instant; | |||
|
|
|||
| use fj_gfx::{self as grahpics, DrawConfig, Renderer}; | |||
There was a problem hiding this comment.
Typo: fj_grahpics. I realize you're fixing it in a later commit, but again, the comment that it would be nicer if each commits compiles applies.
| @@ -15,11 +16,7 @@ use winit::{ | |||
| event_loop::{ControlFlow, EventLoop}, | |||
| }; | |||
|
|
|||
| use crate::{ | |||
| camera::Camera, | |||
There was a problem hiding this comment.
Camera is disappearing here, and added back in a later commit, which means this commit fails to compile. See other comments regarding that.
In addition, this kind of thing makes it slightly harder to review the pull request, as I always need to cross-reference against later commits, if I see something like this.
| @@ -0,0 +1,9 @@ | |||
| [package] | |||
| name = "fj-gui" | |||
There was a problem hiding this comment.
I think this is a misnomer. The crate only contains the winit-based window abstraction, which is related to the topic of GUI, but isn't really a GUI. The only real GUI code is within fj-gfx. Maybe fj-window would be a more appropriate name?
|
Closing in favor of #640. |
This is an intermediate artifact of experimenting with building a vscode plugin / split view editor for fornjot editing. It's a kind of more complete version of #497.
This splits the library components of
fj-viewerinto three seperate crates:fj-gui,fj-gfx, andfj-input. This was done to allow additional client implementations in rust to share code withfj-viewercleanly.Note: While not in this PR. I'm experimenting with a stretch2 based UI due to its association with the future ambitions of the
bevyproject, making it the most likely option to be maintained in my short analysis.