Fix #1015 start gui independent of model #1095
Conversation
|
This PR is a first attempt. I am happy to polish it a little bit upon some discussion. |
payload
changed the title
There was a problem hiding this comment.
Thank you, @payload, this looks great!
I've left some comments with suggestions, but this is all minor stuff, as you didn't give me anything to actually complain about.
If there is no model, run the GUI without watcher. This is kind of useless now until we can select and load a model from the GUI.
This could be confusing to users, but that's acceptable for now. Just one more thing to take care of in #917.
Merging this now. Feel free to address any of the comments in follow-up PRs, or not, as you prefer.
| @@ -46,39 +46,43 @@ pub fn run( | |||
| let mut draw_config = DrawConfig::default(); | |||
|
|
|||
| let mut shape = None; | |||
| let mut camera = None; | |||
| let mut camera = Camera::new(&Default::default()); | |||
There was a problem hiding this comment.
| let mut camera = Camera::new(&Default::default()); | |
| let mut camera = Camera::new(&Aabb::default()); |
This would make it a bit more obvious what is being passed.
| if camera_update_once { | ||
| camera_update_once = false; | ||
| camera = Camera::new(&new_shape.aabb); | ||
| } |
There was a problem hiding this comment.
I'm not sure I like camera_update_once. It seems like the kind of thing that could possibly turn into a bug, as things get shuffled around over time. But it is nicer, than having if let Some(camera) everywhere.
I think maybe Camera is no longer well-suited to this new situation. We could make the constructor parameter-less, just creating reasonable defaults there, and have some kind of internal flag that makes sure that update_planes does what Camera::new currently does when first called.
Then we could just create the camera using Camera::new above and do an unconditional update_planes here, instead of down there right before the rendering. But that would be work for a follow-up pull request (and if you're inclined to do it, I'd be happy to merge it).
| if let Some(model) = model { | ||
| let watcher = model.load_and_watch(parameters)?; | ||
| run(Some(watcher), shape_processor, status)?; | ||
| } else { | ||
| run(None, shape_processor, status)?; | ||
| } |
There was a problem hiding this comment.
This could be made a bit more compact (but possible less clear) using Option::map. Not necessary to change anything, and I don't know if it would even be better. Just noting the possibility, because it stuck out to me.
Fixes #1015
Rendering the gui depends on a camera being there but the camera was only initialized when the model watcher loads something from the model.
Now there is always a camera and in case there is a model watcher the camera is reset on load once.
In main loading a model was always assumed. Since a model is now optional upon start of the app, we have following cases.
If there is a model parameter or config, create a Model::from this model parameter. If this fails already, exit.
If there is an export parameter, check that there is a model. If not exit with "no model specified". Else load once, process and export. Done.
If there is a model, create a watcher and load. Run the GUI.
If there is no model, run the GUI without watcher. This is kind of useless now until we can select and load a model from the GUI.