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.

Sign up for GitHub

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

Fix #1015 start gui independent of model #1095

Merged
merged 5 commits into from
Sep 16, 2022
+69 −56
Merged

Fix #1015 start gui independent of model #1095

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ffcdb63
start gui completely black, not giving the model
payload Sep 15, 2022
88621d2
rendering of gui now independent of shape, only dependent on camera
payload Sep 15, 2022
284f7d7
camera not optional anymore
payload Sep 15, 2022
ddbf0a3
refactored export only mode code path
payload Sep 15, 2022
a49e7df
fix loading model, no model, export mode
payload Sep 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 28 additions & 16 deletions crates/fj-app/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,34 +46,46 @@ fn main() -> anyhow::Result<()> {
let args = Args::parse();
let config = Config::load()?;

let mut path = config.default_path.unwrap_or_else(|| PathBuf::from(""));
let model = args.model.or(config.default_model).ok_or_else(|| {
anyhow!(
"No model specified, and no default model configured.\n\
Specify a model by passing `--model path/to/model`."
)
})?;
path.push(model);

let model = Model::from_path(path.clone())
.with_context(|| format!("Failed to load model: {}", path.display()))?;
let path = config.default_path.unwrap_or_else(|| PathBuf::from(""));
let parameters = args.parameters.unwrap_or_else(Parameters::empty);

let shape_processor = ShapeProcessor {
tolerance: args.tolerance,
};

if let Some(path) = args.export {
let model = if let Some(model) = args.model.or(config.default_model) {
let mut model_path = path;
model_path.push(model);
Some(Model::from_path(model_path.clone()).with_context(|| {
format!("Failed to load model: {}", model_path.display())
})?)
} else {
None
};

if let Some(export_path) = args.export {
// export only mode. just load model, process, export and exit

let model = model.ok_or_else(|| {
anyhow!(
"No model specified, and no default model configured.\n\
Specify a model by passing `--model path/to/model`."
)
})?;

let shape = model.load_once(&parameters, &mut status)?;
let shape = shape_processor.process(&shape)?;

export(&shape.mesh, &path)?;
export(&shape.mesh, &export_path)?;

return Ok(());
}

let watcher = model.load_and_watch(parameters)?;
run(watcher, shape_processor, status)?;
if let Some(model) = model {
let watcher = model.load_and_watch(parameters)?;
run(Some(watcher), shape_processor, status)?;
} else {
run(None, shape_processor, status)?;
}
Comment on lines +83 to +88
Copy link
Owner

Choose a reason for hiding this comment

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

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.


Ok(())
}
81 changes: 41 additions & 40 deletions crates/fj-window/src/run.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use crate::window::{self, Window};

/// Initializes a model viewer for a given model and enters its process loop.
pub fn run(
watcher: Watcher,
watcher: Option<Watcher>,
shape_processor: ShapeProcessor,
mut status: StatusReport,
) -> Result<(), Error> {
Expand All @@ -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());
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
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.

let mut camera_update_once = watcher.is_some();

event_loop.run(move |event, _, control_flow| {
trace!("Handling event: {:?}", event);

if let Some(new_shape) = watcher.receive(&mut status) {
match shape_processor.process(&new_shape) {
Ok(new_shape) => {
renderer.update_geometry(
(&new_shape.mesh).into(),
(&new_shape.debug_info).into(),
new_shape.aabb,
);
if let Some(watcher) = &watcher {
if let Some(new_shape) = watcher.receive(&mut status) {
match shape_processor.process(&new_shape) {
Ok(new_shape) => {
renderer.update_geometry(
(&new_shape.mesh).into(),
(&new_shape.debug_info).into(),
new_shape.aabb,
);

if camera.is_none() {
camera = Some(Camera::new(&new_shape.aabb));
}
if camera_update_once {
camera_update_once = false;
camera = Camera::new(&new_shape.aabb);
}
Comment on lines +65 to +68
Copy link
Owner

Choose a reason for hiding this comment

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

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).


shape = Some(new_shape);
}
Err(err) => {
// Can be cleaned up, once `Report` is stable:
// https://doc.rust-lang.org/std/error/struct.Report.html
shape = Some(new_shape);
}
Err(err) => {
// Can be cleaned up, once `Report` is stable:
// https://doc.rust-lang.org/std/error/struct.Report.html

println!("Shape processing error: {}", err);
println!("Shape processing error: {}", err);

let mut current_err = &err as &dyn error::Error;
while let Some(err) = current_err.source() {
println!();
println!("Caused by:");
println!(" {}", err);
let mut current_err = &err as &dyn error::Error;
while let Some(err) = current_err.source() {
println!();
println!("Caused by:");
println!(" {}", err);

current_err = err;
current_err = err;
}
}
}
}
Expand Down Expand Up @@ -173,17 +177,17 @@ pub fn run(
window.window().request_redraw();
}
Event::RedrawRequested(_) => {
if let (Some(shape), Some(camera)) = (&shape, &mut camera) {
if let Some(shape) = &shape {
camera.update_planes(&shape.aabb);
}

if let Err(err) = renderer.draw(
camera,
&mut draw_config,
window.window(),
&mut status,
) {
warn!("Draw error: {}", err);
}
if let Err(err) = renderer.draw(
&camera,
&mut draw_config,
window.window(),
&mut status,
) {
warn!("Draw error: {}", err);
}
}
_ => {}
Expand All @@ -192,8 +196,7 @@ pub fn run(
// fj-viewer input events
// These can fire multiple times per frame

if let (Some(shape), Some(camera), Some(should_focus)) =
(&shape, &camera, focus_event(&event))
if let (Some(shape), Some(should_focus)) = (&shape, focus_event(&event))
{
if should_focus {
// Don't unnecessarily recalculate focus point
Expand All @@ -212,10 +215,8 @@ pub fn run(
&held_mouse_button,
&mut previous_cursor,
);
if let (Some(input_event), Some(fp), Some(camera)) =
(input_event, focus_point, &mut camera)
{
input_handler.handle_event(input_event, fp, camera);
if let (Some(input_event), Some(fp)) = (input_event, focus_point) {
input_handler.handle_event(input_event, fp, &mut camera);
}
});
}
Expand Down