From 71fa8abd8bd2bfdf2f1befd8a26adaf913a99bdf Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 13:40:35 +0200 Subject: [PATCH 01/33] Clean up imports --- crates/fj-host/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/fj-host/src/lib.rs b/crates/fj-host/src/lib.rs index 50c50e822..92c3e8a57 100644 --- a/crates/fj-host/src/lib.rs +++ b/crates/fj-host/src/lib.rs @@ -17,7 +17,6 @@ mod platform; -use fj_interop::status_report::StatusReport; use std::{ collections::{HashMap, HashSet}, ffi::OsStr, @@ -31,6 +30,7 @@ use std::{ }; use fj::{abi, version::RawVersion}; +use fj_interop::status_report::StatusReport; use notify::Watcher as _; use thiserror::Error; From 04fc3d384cbe72835c4cea3d1b0ad742181eec3e Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 13:45:50 +0200 Subject: [PATCH 02/33] Pass `Parameters` into `Model` constructor This limits the flexibility regarding parameters, but doesn't make a difference right now, as that flexibility isn't used anywhere. It's going to make things simpler though for the time being, which aids in what I'm currently doing. --- crates/fj-app/src/main.rs | 6 +++--- crates/fj-host/src/lib.rs | 20 +++++++++----------- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/crates/fj-app/src/main.rs b/crates/fj-app/src/main.rs index 283efecc7..bfc58b06b 100644 --- a/crates/fj-app/src/main.rs +++ b/crates/fj-app/src/main.rs @@ -58,7 +58,7 @@ fn main() -> anyhow::Result<()> { { let mut model_path = path; model_path.push(model); - Model::from_path(model_path.clone()).with_context(|| { + Model::from_path(model_path.clone(), parameters).with_context(|| { if path_of_model.as_os_str().is_empty() { format!( "Model is not defined, can't find model defined inside the default-model also, add model like \n cargo run -- -m {}", model.display() @@ -80,7 +80,7 @@ fn main() -> anyhow::Result<()> { if let Some(export_path) = args.export { // export only mode. just load model, process, export and exit - let shape = model.load_once(¶meters, &mut status)?; + let shape = model.load_once(&mut status)?; let shape = shape_processor.process(&shape)?; export(&shape.mesh, &export_path)?; @@ -90,7 +90,7 @@ fn main() -> anyhow::Result<()> { let invert_zoom = config.invert_zoom.unwrap_or(false); - let watcher = model.load_and_watch(parameters)?; + let watcher = model.load_and_watch()?; run(watcher, shape_processor, status, invert_zoom)?; Ok(()) diff --git a/crates/fj-host/src/lib.rs b/crates/fj-host/src/lib.rs index 92c3e8a57..06c871d39 100644 --- a/crates/fj-host/src/lib.rs +++ b/crates/fj-host/src/lib.rs @@ -41,6 +41,7 @@ pub struct Model { src_path: PathBuf, lib_path: PathBuf, manifest_path: PathBuf, + parameters: Parameters, } impl Model { @@ -48,7 +49,10 @@ impl Model { /// /// The path expected here is the root directory of the model's Cargo /// package, that is the folder containing `Cargo.toml`. - pub fn from_path(path: PathBuf) -> Result { + pub fn from_path( + path: PathBuf, + parameters: Parameters, + ) -> Result { let crate_dir = path.canonicalize()?; let metadata = cargo_metadata::MetadataCommand::new() @@ -70,6 +74,7 @@ impl Model { src_path, lib_path, manifest_path: pkg.manifest_path.as_std_path().to_path_buf(), + parameters, }) } @@ -82,7 +87,6 @@ impl Model { /// model for changes, reloading it continually. pub fn load_once( &self, - arguments: &Parameters, status: &mut StatusReport, ) -> Result { let manifest_path = self.manifest_path.display().to_string(); @@ -164,7 +168,7 @@ impl Model { lib.get(abi::INIT_FUNCTION_NAME.as_bytes())?; let mut host = Host { - args: arguments, + args: &self.parameters, model: None, }; @@ -189,10 +193,7 @@ impl Model { /// /// Consumes this instance of `Model` and returns a [`Watcher`], which can /// be queried for changes to the model. - pub fn load_and_watch( - self, - parameters: Parameters, - ) -> Result { + pub fn load_and_watch(self) -> Result { let (tx, rx) = mpsc::sync_channel(0); let tx2 = tx.clone(); @@ -262,7 +263,6 @@ impl Model { _watcher: Box::new(watcher), channel: rx, model: self, - parameters, }) } } @@ -316,7 +316,6 @@ pub struct Watcher { _watcher: Box, channel: mpsc::Receiver<()>, model: Model, - parameters: Parameters, } impl Watcher { @@ -330,8 +329,7 @@ impl Watcher { ) -> Result, Error> { match self.channel.try_recv() { Ok(()) => { - let shape = match self.model.load_once(&self.parameters, status) - { + let shape = match self.model.load_once(status) { Ok(shape) => shape, Err(Error::Compile) => { // An error is being displayed to the user via the From a966458c029817a7840ee5fe01c5aaaa22e395fc Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 13:48:22 +0200 Subject: [PATCH 03/33] Add `Watcher::watch_model` --- crates/fj-app/src/main.rs | 4 ++-- crates/fj-host/src/lib.rs | 7 ++++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/crates/fj-app/src/main.rs b/crates/fj-app/src/main.rs index bfc58b06b..c81b62d3f 100644 --- a/crates/fj-app/src/main.rs +++ b/crates/fj-app/src/main.rs @@ -19,7 +19,7 @@ use std::path::PathBuf; use anyhow::{anyhow, Context as _}; use fj_export::export; -use fj_host::{Model, Parameters}; +use fj_host::{Model, Parameters, Watcher}; use fj_interop::status_report::StatusReport; use fj_operations::shape_processor::ShapeProcessor; use fj_window::run::run; @@ -90,7 +90,7 @@ fn main() -> anyhow::Result<()> { let invert_zoom = config.invert_zoom.unwrap_or(false); - let watcher = model.load_and_watch()?; + let watcher = Watcher::watch_model(model)?; run(watcher, shape_processor, status, invert_zoom)?; Ok(()) diff --git a/crates/fj-host/src/lib.rs b/crates/fj-host/src/lib.rs index 06c871d39..efdaed37f 100644 --- a/crates/fj-host/src/lib.rs +++ b/crates/fj-host/src/lib.rs @@ -193,7 +193,7 @@ impl Model { /// /// Consumes this instance of `Model` and returns a [`Watcher`], which can /// be queried for changes to the model. - pub fn load_and_watch(self) -> Result { + fn load_and_watch(self) -> Result { let (tx, rx) = mpsc::sync_channel(0); let tx2 = tx.clone(); @@ -319,6 +319,11 @@ pub struct Watcher { } impl Watcher { + /// Watch the provided model for changes + pub fn watch_model(model: Model) -> Result { + model.load_and_watch() + } + /// Receive an updated shape that the reloaded model created /// /// Returns `None`, if the model has not changed since the last time this From dc2aeeda357f3fb1839e40b234e7e737d9d5e6fe Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 13:49:26 +0200 Subject: [PATCH 04/33] Remove `Model::load_and_watch` It's been made redundant by `Watcher::wwatch_model`. --- crates/fj-host/src/lib.rs | 129 +++++++++++++++++--------------------- 1 file changed, 58 insertions(+), 71 deletions(-) diff --git a/crates/fj-host/src/lib.rs b/crates/fj-host/src/lib.rs index efdaed37f..755bc8ab4 100644 --- a/crates/fj-host/src/lib.rs +++ b/crates/fj-host/src/lib.rs @@ -82,9 +82,6 @@ impl Model { /// /// The passed arguments are provided to the model. Returns the shape that /// the model returns. - /// - /// Please refer to [`Model::load_and_watch`], if you want to watch the - /// model for changes, reloading it continually. pub fn load_once( &self, status: &mut StatusReport, @@ -186,18 +183,66 @@ impl Model { Ok(shape) } +} - /// Load the model, then watch it for changes - /// - /// Whenever a change is detected, the model is being reloaded. - /// - /// Consumes this instance of `Model` and returns a [`Watcher`], which can - /// be queried for changes to the model. - fn load_and_watch(self) -> Result { +fn package_associated_with_directory<'m>( + metadata: &'m cargo_metadata::Metadata, + dir: &Path, +) -> Result<&'m cargo_metadata::Package, Error> { + for pkg in metadata.workspace_packages() { + let crate_dir = pkg + .manifest_path + .parent() + .and_then(|p| p.canonicalize().ok()); + + if crate_dir.as_deref() == Some(dir) { + return Ok(pkg); + } + } + + Err(ambiguous_path_error(metadata, dir)) +} + +fn ambiguous_path_error( + metadata: &cargo_metadata::Metadata, + dir: &Path, +) -> Error { + let mut possible_paths = Vec::new(); + + for id in &metadata.workspace_members { + let cargo_toml = &metadata[id].manifest_path; + let crate_dir = cargo_toml + .parent() + .expect("A Cargo.toml always has a parent"); + // Try to make the path relative to the workspace root so error messages + // aren't super long. + let simplified_path = crate_dir + .strip_prefix(&metadata.workspace_root) + .unwrap_or(crate_dir); + + possible_paths.push(simplified_path.into()); + } + + Error::AmbiguousPath { + dir: dir.to_path_buf(), + possible_paths, + } +} + +/// Watches a model for changes, reloading it continually +pub struct Watcher { + _watcher: Box, + channel: mpsc::Receiver<()>, + model: Model, +} + +impl Watcher { + /// Watch the provided model for changes + pub fn watch_model(model: Model) -> Result { let (tx, rx) = mpsc::sync_channel(0); let tx2 = tx.clone(); - let watch_path = self.src_path.clone(); + let watch_path = model.src_path.clone(); let mut watcher = notify::recommended_watcher( move |event: notify::Result| { @@ -259,70 +304,12 @@ impl Model { // about that, if it happened. thread::spawn(move || tx2.send(()).expect("Channel is disconnected")); - Ok(Watcher { + Ok(Self { _watcher: Box::new(watcher), channel: rx, - model: self, + model, }) } -} - -fn package_associated_with_directory<'m>( - metadata: &'m cargo_metadata::Metadata, - dir: &Path, -) -> Result<&'m cargo_metadata::Package, Error> { - for pkg in metadata.workspace_packages() { - let crate_dir = pkg - .manifest_path - .parent() - .and_then(|p| p.canonicalize().ok()); - - if crate_dir.as_deref() == Some(dir) { - return Ok(pkg); - } - } - - Err(ambiguous_path_error(metadata, dir)) -} - -fn ambiguous_path_error( - metadata: &cargo_metadata::Metadata, - dir: &Path, -) -> Error { - let mut possible_paths = Vec::new(); - - for id in &metadata.workspace_members { - let cargo_toml = &metadata[id].manifest_path; - let crate_dir = cargo_toml - .parent() - .expect("A Cargo.toml always has a parent"); - // Try to make the path relative to the workspace root so error messages - // aren't super long. - let simplified_path = crate_dir - .strip_prefix(&metadata.workspace_root) - .unwrap_or(crate_dir); - - possible_paths.push(simplified_path.into()); - } - - Error::AmbiguousPath { - dir: dir.to_path_buf(), - possible_paths, - } -} - -/// Watches a model for changes, reloading it continually -pub struct Watcher { - _watcher: Box, - channel: mpsc::Receiver<()>, - model: Model, -} - -impl Watcher { - /// Watch the provided model for changes - pub fn watch_model(model: Model) -> Result { - model.load_and_watch() - } /// Receive an updated shape that the reloaded model created /// From a09d89c2cbb12c0e1253834a59c4b3dd868057d1 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 13:50:30 +0200 Subject: [PATCH 05/33] Rename `Model::from_path` The old name has become too specific. --- crates/fj-app/src/main.rs | 2 +- crates/fj-host/src/lib.rs | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/crates/fj-app/src/main.rs b/crates/fj-app/src/main.rs index c81b62d3f..0722c8474 100644 --- a/crates/fj-app/src/main.rs +++ b/crates/fj-app/src/main.rs @@ -58,7 +58,7 @@ fn main() -> anyhow::Result<()> { { let mut model_path = path; model_path.push(model); - Model::from_path(model_path.clone(), parameters).with_context(|| { + Model::new(model_path.clone(), parameters).with_context(|| { if path_of_model.as_os_str().is_empty() { format!( "Model is not defined, can't find model defined inside the default-model also, add model like \n cargo run -- -m {}", model.display() diff --git a/crates/fj-host/src/lib.rs b/crates/fj-host/src/lib.rs index 755bc8ab4..37a9780b7 100644 --- a/crates/fj-host/src/lib.rs +++ b/crates/fj-host/src/lib.rs @@ -49,10 +49,7 @@ impl Model { /// /// The path expected here is the root directory of the model's Cargo /// package, that is the folder containing `Cargo.toml`. - pub fn from_path( - path: PathBuf, - parameters: Parameters, - ) -> Result { + pub fn new(path: PathBuf, parameters: Parameters) -> Result { let crate_dir = path.canonicalize()?; let metadata = cargo_metadata::MetadataCommand::new() From 9f1387fc5bd289f4208f427a6f0dbcd94f294a03 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 13:54:30 +0200 Subject: [PATCH 06/33] Add `Model::src_path` --- crates/fj-host/src/lib.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/fj-host/src/lib.rs b/crates/fj-host/src/lib.rs index 37a9780b7..310ba6491 100644 --- a/crates/fj-host/src/lib.rs +++ b/crates/fj-host/src/lib.rs @@ -75,6 +75,10 @@ impl Model { }) } + pub(crate) fn src_path(&self) -> PathBuf { + self.src_path.clone() + } + /// Load the model once /// /// The passed arguments are provided to the model. Returns the shape that @@ -239,7 +243,7 @@ impl Watcher { let (tx, rx) = mpsc::sync_channel(0); let tx2 = tx.clone(); - let watch_path = model.src_path.clone(); + let watch_path = model.src_path(); let mut watcher = notify::recommended_watcher( move |event: notify::Result| { From ef7b73895c17424b95c039fceded7c1c8678170b Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 13:56:37 +0200 Subject: [PATCH 07/33] Move `Watcher` to dedicated module --- crates/fj-host/src/lib.rs | 130 ++-------------------------------- crates/fj-host/src/watcher.rs | 127 +++++++++++++++++++++++++++++++++ 2 files changed, 131 insertions(+), 126 deletions(-) create mode 100644 crates/fj-host/src/watcher.rs diff --git a/crates/fj-host/src/lib.rs b/crates/fj-host/src/lib.rs index 310ba6491..9cbedac5e 100644 --- a/crates/fj-host/src/lib.rs +++ b/crates/fj-host/src/lib.rs @@ -16,22 +16,21 @@ #![warn(missing_docs)] mod platform; +mod watcher; + +pub use self::watcher::Watcher; use std::{ - collections::{HashMap, HashSet}, - ffi::OsStr, + collections::HashMap, io, ops::{Deref, DerefMut}, path::{Path, PathBuf}, process::Command, str, - sync::mpsc, - thread, }; use fj::{abi, version::RawVersion}; use fj_interop::status_report::StatusReport; -use notify::Watcher as _; use thiserror::Error; use self::platform::HostPlatform; @@ -230,127 +229,6 @@ fn ambiguous_path_error( } } -/// Watches a model for changes, reloading it continually -pub struct Watcher { - _watcher: Box, - channel: mpsc::Receiver<()>, - model: Model, -} - -impl Watcher { - /// Watch the provided model for changes - pub fn watch_model(model: Model) -> Result { - let (tx, rx) = mpsc::sync_channel(0); - let tx2 = tx.clone(); - - let watch_path = model.src_path(); - - let mut watcher = notify::recommended_watcher( - move |event: notify::Result| { - // Unfortunately the `notify` documentation doesn't say when - // this might happen, so no idea if it needs to be handled. - let event = event.expect("Error handling watch event"); - - // Various acceptable ModifyKind kinds. Varies across platforms - // (e.g. MacOs vs. Windows10) - if let notify::EventKind::Modify( - notify::event::ModifyKind::Any, - ) - | notify::EventKind::Modify( - notify::event::ModifyKind::Data( - notify::event::DataChange::Any, - ), - ) - | notify::EventKind::Modify( - notify::event::ModifyKind::Data( - notify::event::DataChange::Content, - ), - ) = event.kind - { - let file_ext = event - .paths - .get(0) - .expect("File path missing in watch event") - .extension(); - - let black_list = HashSet::from([ - OsStr::new("swp"), - OsStr::new("tmp"), - OsStr::new("swx"), - ]); - - if let Some(ext) = file_ext { - if black_list.contains(ext) { - return; - } - } - - // This will panic, if the other end is disconnected, which - // is probably the result of a panic on that thread, or the - // application is being shut down. - // - // Either way, not much we can do about it here. - tx.send(()).expect("Channel is disconnected"); - } - }, - )?; - - watcher.watch(&watch_path, notify::RecursiveMode::Recursive)?; - - // To prevent a race condition between the initial load and the start of - // watching, we'll trigger the initial load here, after having started - // watching. - // - // Will panic, if the receiving end has panicked. Not much we can do - // about that, if it happened. - thread::spawn(move || tx2.send(()).expect("Channel is disconnected")); - - Ok(Self { - _watcher: Box::new(watcher), - channel: rx, - model, - }) - } - - /// Receive an updated shape that the reloaded model created - /// - /// Returns `None`, if the model has not changed since the last time this - /// method was called. - pub fn receive_shape( - &self, - status: &mut StatusReport, - ) -> Result, Error> { - match self.channel.try_recv() { - Ok(()) => { - let shape = match self.model.load_once(status) { - Ok(shape) => shape, - Err(Error::Compile) => { - // An error is being displayed to the user via the - // `StatusReport that is passed to `load_once` above, so - // no need to do anything else here. - return Ok(None); - } - Err(err) => { - return Err(err); - } - }; - - Ok(Some(shape)) - } - Err(mpsc::TryRecvError::Empty) => { - // Nothing to receive from the channel. - Ok(None) - } - Err(mpsc::TryRecvError::Disconnected) => { - // The other end has disconnected. This is probably the result - // of a panic on the other thread, or a program shutdown in - // progress. In any case, not much we can do here. - panic!(); - } - } - } -} - /// Parameters that are passed to a model. #[derive(Debug, Clone, Eq, PartialEq)] pub struct Parameters(pub HashMap); diff --git a/crates/fj-host/src/watcher.rs b/crates/fj-host/src/watcher.rs new file mode 100644 index 000000000..cbb9daf23 --- /dev/null +++ b/crates/fj-host/src/watcher.rs @@ -0,0 +1,127 @@ +use std::{collections::HashSet, ffi::OsStr, sync::mpsc, thread}; + +use fj_interop::status_report::StatusReport; +use notify::Watcher as _; + +use crate::{Error, Model}; + +/// Watches a model for changes, reloading it continually +pub struct Watcher { + _watcher: Box, + channel: mpsc::Receiver<()>, + model: Model, +} + +impl Watcher { + /// Watch the provided model for changes + pub fn watch_model(model: Model) -> Result { + let (tx, rx) = mpsc::sync_channel(0); + let tx2 = tx.clone(); + + let watch_path = model.src_path(); + + let mut watcher = notify::recommended_watcher( + move |event: notify::Result| { + // Unfortunately the `notify` documentation doesn't say when + // this might happen, so no idea if it needs to be handled. + let event = event.expect("Error handling watch event"); + + // Various acceptable ModifyKind kinds. Varies across platforms + // (e.g. MacOs vs. Windows10) + if let notify::EventKind::Modify( + notify::event::ModifyKind::Any, + ) + | notify::EventKind::Modify( + notify::event::ModifyKind::Data( + notify::event::DataChange::Any, + ), + ) + | notify::EventKind::Modify( + notify::event::ModifyKind::Data( + notify::event::DataChange::Content, + ), + ) = event.kind + { + let file_ext = event + .paths + .get(0) + .expect("File path missing in watch event") + .extension(); + + let black_list = HashSet::from([ + OsStr::new("swp"), + OsStr::new("tmp"), + OsStr::new("swx"), + ]); + + if let Some(ext) = file_ext { + if black_list.contains(ext) { + return; + } + } + + // This will panic, if the other end is disconnected, which + // is probably the result of a panic on that thread, or the + // application is being shut down. + // + // Either way, not much we can do about it here. + tx.send(()).expect("Channel is disconnected"); + } + }, + )?; + + watcher.watch(&watch_path, notify::RecursiveMode::Recursive)?; + + // To prevent a race condition between the initial load and the start of + // watching, we'll trigger the initial load here, after having started + // watching. + // + // Will panic, if the receiving end has panicked. Not much we can do + // about that, if it happened. + thread::spawn(move || tx2.send(()).expect("Channel is disconnected")); + + Ok(Self { + _watcher: Box::new(watcher), + channel: rx, + model, + }) + } + + /// Receive an updated shape that the reloaded model created + /// + /// Returns `None`, if the model has not changed since the last time this + /// method was called. + pub fn receive_shape( + &self, + status: &mut StatusReport, + ) -> Result, Error> { + match self.channel.try_recv() { + Ok(()) => { + let shape = match self.model.load_once(status) { + Ok(shape) => shape, + Err(Error::Compile) => { + // An error is being displayed to the user via the + // `StatusReport that is passed to `load_once` above, so + // no need to do anything else here. + return Ok(None); + } + Err(err) => { + return Err(err); + } + }; + + Ok(Some(shape)) + } + Err(mpsc::TryRecvError::Empty) => { + // Nothing to receive from the channel. + Ok(None) + } + Err(mpsc::TryRecvError::Disconnected) => { + // The other end has disconnected. This is probably the result + // of a panic on the other thread, or a program shutdown in + // progress. In any case, not much we can do here. + panic!(); + } + } + } +} From fdd99314dbf0535b7ff209726476357f8426c6c8 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 14:02:58 +0200 Subject: [PATCH 08/33] Move `Parameters` to dedicated module --- crates/fj-host/src/lib.rs | 41 ++------------------------------ crates/fj-host/src/parameters.rs | 40 +++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 39 deletions(-) create mode 100644 crates/fj-host/src/parameters.rs diff --git a/crates/fj-host/src/lib.rs b/crates/fj-host/src/lib.rs index 9cbedac5e..471cac50a 100644 --- a/crates/fj-host/src/lib.rs +++ b/crates/fj-host/src/lib.rs @@ -15,15 +15,14 @@ #![warn(missing_docs)] +mod parameters; mod platform; mod watcher; -pub use self::watcher::Watcher; +pub use self::{parameters::Parameters, watcher::Watcher}; use std::{ - collections::HashMap, io, - ops::{Deref, DerefMut}, path::{Path, PathBuf}, process::Command, str, @@ -229,42 +228,6 @@ fn ambiguous_path_error( } } -/// Parameters that are passed to a model. -#[derive(Debug, Clone, Eq, PartialEq)] -pub struct Parameters(pub HashMap); - -impl Parameters { - /// Construct an empty instance of `Parameters` - pub fn empty() -> Self { - Self(HashMap::new()) - } - - /// Insert a value into the [`Parameters`] dictionary, implicitly converting - /// the arguments to strings and returning `&mut self` to enable chaining. - pub fn insert( - &mut self, - key: impl Into, - value: impl ToString, - ) -> &mut Self { - self.0.insert(key.into(), value.to_string()); - self - } -} - -impl Deref for Parameters { - type Target = HashMap; - - fn deref(&self) -> &Self::Target { - &self.0 - } -} - -impl DerefMut for Parameters { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.0 - } -} - /// An error that can occur when loading or reloading a model #[derive(Debug, Error)] pub enum Error { diff --git a/crates/fj-host/src/parameters.rs b/crates/fj-host/src/parameters.rs new file mode 100644 index 000000000..bdfd658a0 --- /dev/null +++ b/crates/fj-host/src/parameters.rs @@ -0,0 +1,40 @@ +use std::{ + collections::HashMap, + ops::{Deref, DerefMut}, +}; + +/// Parameters that are passed to a model. +#[derive(Debug, Clone, Eq, PartialEq)] +pub struct Parameters(pub HashMap); + +impl Parameters { + /// Construct an empty instance of `Parameters` + pub fn empty() -> Self { + Self(HashMap::new()) + } + + /// Insert a value into the [`Parameters`] dictionary, implicitly converting + /// the arguments to strings and returning `&mut self` to enable chaining. + pub fn insert( + &mut self, + key: impl Into, + value: impl ToString, + ) -> &mut Self { + self.0.insert(key.into(), value.to_string()); + self + } +} + +impl Deref for Parameters { + type Target = HashMap; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl DerefMut for Parameters { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } +} From 2c2276c13562e8ad4f94c0d4d75a4ccdebb4a1c3 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 14:05:36 +0200 Subject: [PATCH 09/33] Add `Host::take_model` --- crates/fj-host/src/lib.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/fj-host/src/lib.rs b/crates/fj-host/src/lib.rs index 471cac50a..226e54398 100644 --- a/crates/fj-host/src/lib.rs +++ b/crates/fj-host/src/lib.rs @@ -175,7 +175,7 @@ impl Model { } } - let model = host.model.take().ok_or(Error::NoModelRegistered)?; + let model = host.take_model().ok_or(Error::NoModelRegistered)?; model.shape(&host).map_err(Error::Shape)? }; @@ -298,6 +298,12 @@ struct Host<'a> { model: Option>, } +impl Host<'_> { + pub fn take_model(&mut self) -> Option> { + self.model.take() + } +} + impl<'a> fj::models::Host for Host<'a> { fn register_boxed_model(&mut self, model: Box) { self.model = Some(model); From 9cd84ca50142ec9b533089673d01358b12d7838b Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 14:07:39 +0200 Subject: [PATCH 10/33] Add `Host::new` --- crates/fj-host/src/lib.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/crates/fj-host/src/lib.rs b/crates/fj-host/src/lib.rs index 226e54398..293eb6cd7 100644 --- a/crates/fj-host/src/lib.rs +++ b/crates/fj-host/src/lib.rs @@ -163,10 +163,7 @@ impl Model { let init: libloading::Symbol = lib.get(abi::INIT_FUNCTION_NAME.as_bytes())?; - let mut host = Host { - args: &self.parameters, - model: None, - }; + let mut host = Host::new(&self.parameters); match init(&mut abi::Host::from(&mut host)) { abi::ffi_safe::Result::Ok(_metadata) => {} @@ -298,7 +295,14 @@ struct Host<'a> { model: Option>, } -impl Host<'_> { +impl<'a> Host<'a> { + pub fn new(parameters: &'a Parameters) -> Self { + Self { + args: parameters, + model: None, + } + } + pub fn take_model(&mut self) -> Option> { self.model.take() } From 09edebab5ea0225d4d5be6156184a176a045ad49 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 14:08:31 +0200 Subject: [PATCH 11/33] Move `Host` to dedicated directory --- crates/fj-host/src/host.rs | 31 +++++++++++++++++++++++++++++++ crates/fj-host/src/lib.rs | 32 ++------------------------------ 2 files changed, 33 insertions(+), 30 deletions(-) create mode 100644 crates/fj-host/src/host.rs diff --git a/crates/fj-host/src/host.rs b/crates/fj-host/src/host.rs new file mode 100644 index 000000000..e66a738dd --- /dev/null +++ b/crates/fj-host/src/host.rs @@ -0,0 +1,31 @@ +use crate::Parameters; + +pub struct Host<'a> { + args: &'a Parameters, + model: Option>, +} + +impl<'a> Host<'a> { + pub fn new(parameters: &'a Parameters) -> Self { + Self { + args: parameters, + model: None, + } + } + + pub fn take_model(&mut self) -> Option> { + self.model.take() + } +} + +impl<'a> fj::models::Host for Host<'a> { + fn register_boxed_model(&mut self, model: Box) { + self.model = Some(model); + } +} + +impl<'a> fj::models::Context for Host<'a> { + fn get_argument(&self, name: &str) -> Option<&str> { + self.args.get(name).map(|s| s.as_str()) + } +} diff --git a/crates/fj-host/src/lib.rs b/crates/fj-host/src/lib.rs index 293eb6cd7..eb9a2ab90 100644 --- a/crates/fj-host/src/lib.rs +++ b/crates/fj-host/src/lib.rs @@ -15,6 +15,7 @@ #![warn(missing_docs)] +mod host; mod parameters; mod platform; mod watcher; @@ -30,6 +31,7 @@ use std::{ use fj::{abi, version::RawVersion}; use fj_interop::status_report::StatusReport; +use host::Host; use thiserror::Error; use self::platform::HostPlatform; @@ -289,33 +291,3 @@ pub enum Error { possible_paths: Vec, }, } - -struct Host<'a> { - args: &'a Parameters, - model: Option>, -} - -impl<'a> Host<'a> { - pub fn new(parameters: &'a Parameters) -> Self { - Self { - args: parameters, - model: None, - } - } - - pub fn take_model(&mut self) -> Option> { - self.model.take() - } -} - -impl<'a> fj::models::Host for Host<'a> { - fn register_boxed_model(&mut self, model: Box) { - self.model = Some(model); - } -} - -impl<'a> fj::models::Context for Host<'a> { - fn get_argument(&self, name: &str) -> Option<&str> { - self.args.get(name).map(|s| s.as_str()) - } -} From a3cd2018825870df9428d4d68c513a505e3f3d10 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 14:11:50 +0200 Subject: [PATCH 12/33] Move `Model` to dedicated directory --- crates/fj-host/src/lib.rs | 275 +----------------------------------- crates/fj-host/src/model.rs | 267 ++++++++++++++++++++++++++++++++++ 2 files changed, 272 insertions(+), 270 deletions(-) create mode 100644 crates/fj-host/src/model.rs diff --git a/crates/fj-host/src/lib.rs b/crates/fj-host/src/lib.rs index eb9a2ab90..ec5dfc298 100644 --- a/crates/fj-host/src/lib.rs +++ b/crates/fj-host/src/lib.rs @@ -16,278 +16,13 @@ #![warn(missing_docs)] mod host; +mod model; mod parameters; mod platform; mod watcher; -pub use self::{parameters::Parameters, watcher::Watcher}; - -use std::{ - io, - path::{Path, PathBuf}, - process::Command, - str, +pub use self::{ + model::{Error, Model}, + parameters::Parameters, + watcher::Watcher, }; - -use fj::{abi, version::RawVersion}; -use fj_interop::status_report::StatusReport; -use host::Host; -use thiserror::Error; - -use self::platform::HostPlatform; - -/// Represents a Fornjot model -pub struct Model { - src_path: PathBuf, - lib_path: PathBuf, - manifest_path: PathBuf, - parameters: Parameters, -} - -impl Model { - /// Initialize the model using the path to its crate - /// - /// The path expected here is the root directory of the model's Cargo - /// package, that is the folder containing `Cargo.toml`. - pub fn new(path: PathBuf, parameters: Parameters) -> Result { - let crate_dir = path.canonicalize()?; - - let metadata = cargo_metadata::MetadataCommand::new() - .current_dir(&crate_dir) - .exec()?; - - let pkg = package_associated_with_directory(&metadata, &crate_dir)?; - let src_path = crate_dir.join("src"); - - let lib_path = { - let name = pkg.name.replace('-', "_"); - let file = HostPlatform::lib_file_name(&name); - let target_dir = - metadata.target_directory.clone().into_std_path_buf(); - target_dir.join("debug").join(file) - }; - - Ok(Self { - src_path, - lib_path, - manifest_path: pkg.manifest_path.as_std_path().to_path_buf(), - parameters, - }) - } - - pub(crate) fn src_path(&self) -> PathBuf { - self.src_path.clone() - } - - /// Load the model once - /// - /// The passed arguments are provided to the model. Returns the shape that - /// the model returns. - pub fn load_once( - &self, - status: &mut StatusReport, - ) -> Result { - let manifest_path = self.manifest_path.display().to_string(); - - let mut command_root = Command::new("cargo"); - - let command = command_root - .arg("rustc") - .args(["--manifest-path", &manifest_path]) - .args(["--crate-type", "cdylib"]); - - let cargo_output = command.output()?; - let exit_status = cargo_output.status; - - if exit_status.success() { - let seconds_taken = str::from_utf8(&cargo_output.stderr) - .unwrap() - .rsplit_once(' ') - .unwrap() - .1 - .trim(); - status.update_status( - format!("Model compiled successfully in {seconds_taken}!") - .as_str(), - ); - } else { - let output = match command.output() { - Ok(output) => { - String::from_utf8(output.stderr).unwrap_or_else(|_| { - String::from("Failed to fetch command output") - }) - } - Err(_) => String::from("Failed to fetch command output"), - }; - status.clear_status(); - status.update_status(&format!( - "Failed to compile model:\n{}", - output - )); - return Err(Error::Compile); - } - - // So, strictly speaking this is all unsound: - // - `Library::new` requires us to abide by the arbitrary requirements - // of any library initialization or termination routines. - // - `Library::get` requires us to specify the correct type for the - // model function. - // - The model function itself is `unsafe`, because it is a function - // from across an FFI interface. - // - // Typical models won't have initialization or termination routines (I - // think), should abide by the `ModelFn` signature, and might not do - // anything unsafe. But we have no way to know that the library the user - // told us to load actually does (I think). - // - // I don't know of a way to fix this. We should take this as motivation - // to switch to a better technique: - // https://github.com/hannobraun/Fornjot/issues/71 - let shape = unsafe { - let lib = libloading::Library::new(&self.lib_path)?; - - let version_pkg: libloading::Symbol RawVersion> = - lib.get(b"version_pkg")?; - - let version_pkg = version_pkg(); - if fj::version::VERSION_PKG != version_pkg.as_str() { - let host = String::from_utf8_lossy( - fj::version::VERSION_PKG.as_bytes(), - ) - .into_owned(); - let model = - String::from_utf8_lossy(version_pkg.as_str().as_bytes()) - .into_owned(); - - return Err(Error::VersionMismatch { host, model }); - } - - let init: libloading::Symbol = - lib.get(abi::INIT_FUNCTION_NAME.as_bytes())?; - - let mut host = Host::new(&self.parameters); - - match init(&mut abi::Host::from(&mut host)) { - abi::ffi_safe::Result::Ok(_metadata) => {} - abi::ffi_safe::Result::Err(e) => { - return Err(Error::InitializeModel(e.into())); - } - } - - let model = host.take_model().ok_or(Error::NoModelRegistered)?; - - model.shape(&host).map_err(Error::Shape)? - }; - - Ok(shape) - } -} - -fn package_associated_with_directory<'m>( - metadata: &'m cargo_metadata::Metadata, - dir: &Path, -) -> Result<&'m cargo_metadata::Package, Error> { - for pkg in metadata.workspace_packages() { - let crate_dir = pkg - .manifest_path - .parent() - .and_then(|p| p.canonicalize().ok()); - - if crate_dir.as_deref() == Some(dir) { - return Ok(pkg); - } - } - - Err(ambiguous_path_error(metadata, dir)) -} - -fn ambiguous_path_error( - metadata: &cargo_metadata::Metadata, - dir: &Path, -) -> Error { - let mut possible_paths = Vec::new(); - - for id in &metadata.workspace_members { - let cargo_toml = &metadata[id].manifest_path; - let crate_dir = cargo_toml - .parent() - .expect("A Cargo.toml always has a parent"); - // Try to make the path relative to the workspace root so error messages - // aren't super long. - let simplified_path = crate_dir - .strip_prefix(&metadata.workspace_root) - .unwrap_or(crate_dir); - - possible_paths.push(simplified_path.into()); - } - - Error::AmbiguousPath { - dir: dir.to_path_buf(), - possible_paths, - } -} - -/// An error that can occur when loading or reloading a model -#[derive(Debug, Error)] -pub enum Error { - /// Model failed to compile - #[error("Error compiling model")] - Compile, - - /// I/O error while loading the model - #[error("I/O error while loading model")] - Io(#[from] io::Error), - - /// Failed to load the model's dynamic library - #[error("Error loading model from dynamic library")] - LibLoading(#[from] libloading::Error), - - /// Host version and model version do not match - #[error("Host version ({host}) and model version ({model}) do not match")] - VersionMismatch { - /// The host version - host: String, - - /// The model version - model: String, - }, - - /// Initializing a model failed. - #[error("Unable to initialize the model")] - InitializeModel(#[source] fj::models::Error), - - /// The user forgot to register a model when calling - /// [`fj::register_model!()`]. - #[error("No model was registered")] - NoModelRegistered, - - /// An error was returned from [`fj::models::Model::shape()`]. - #[error("Unable to determine the model's geometry")] - Shape(#[source] fj::models::Error), - - /// Error while watching the model code for changes - #[error("Error watching model for changes")] - Notify(#[from] notify::Error), - - /// An error occurred while trying to use evaluate - /// [`cargo_metadata::MetadataCommand`]. - #[error("Unable to determine the crate's metadata")] - CargoMetadata(#[from] cargo_metadata::Error), - - /// The user pointed us to a directory, but it doesn't look like that was - /// a crate root (i.e. the folder containing `Cargo.toml`). - #[error( - "It doesn't look like \"{}\" is a crate directory. Did you mean one of {}?", - dir.display(), - possible_paths.iter().map(|p| p.display().to_string()) - .collect::>() - .join(", ") - )] - AmbiguousPath { - /// The model directory supplied by the user. - dir: PathBuf, - /// The directories for each crate in the workspace, relative to the - /// workspace root. - possible_paths: Vec, - }, -} diff --git a/crates/fj-host/src/model.rs b/crates/fj-host/src/model.rs new file mode 100644 index 000000000..3b83aca92 --- /dev/null +++ b/crates/fj-host/src/model.rs @@ -0,0 +1,267 @@ +use std::{ + io, + path::{Path, PathBuf}, + process::Command, + str, +}; + +use fj::{abi, version::RawVersion}; +use fj_interop::status_report::StatusReport; + +use crate::{host::Host, platform::HostPlatform, Parameters}; + +/// Represents a Fornjot model +pub struct Model { + src_path: PathBuf, + lib_path: PathBuf, + manifest_path: PathBuf, + parameters: Parameters, +} + +impl Model { + /// Initialize the model using the path to its crate + /// + /// The path expected here is the root directory of the model's Cargo + /// package, that is the folder containing `Cargo.toml`. + pub fn new(path: PathBuf, parameters: Parameters) -> Result { + let crate_dir = path.canonicalize()?; + + let metadata = cargo_metadata::MetadataCommand::new() + .current_dir(&crate_dir) + .exec()?; + + let pkg = package_associated_with_directory(&metadata, &crate_dir)?; + let src_path = crate_dir.join("src"); + + let lib_path = { + let name = pkg.name.replace('-', "_"); + let file = HostPlatform::lib_file_name(&name); + let target_dir = + metadata.target_directory.clone().into_std_path_buf(); + target_dir.join("debug").join(file) + }; + + Ok(Self { + src_path, + lib_path, + manifest_path: pkg.manifest_path.as_std_path().to_path_buf(), + parameters, + }) + } + + pub(crate) fn src_path(&self) -> PathBuf { + self.src_path.clone() + } + + /// Load the model once + /// + /// The passed arguments are provided to the model. Returns the shape that + /// the model returns. + pub fn load_once( + &self, + status: &mut StatusReport, + ) -> Result { + let manifest_path = self.manifest_path.display().to_string(); + + let mut command_root = Command::new("cargo"); + + let command = command_root + .arg("rustc") + .args(["--manifest-path", &manifest_path]) + .args(["--crate-type", "cdylib"]); + + let cargo_output = command.output()?; + let exit_status = cargo_output.status; + + if exit_status.success() { + let seconds_taken = str::from_utf8(&cargo_output.stderr) + .unwrap() + .rsplit_once(' ') + .unwrap() + .1 + .trim(); + status.update_status( + format!("Model compiled successfully in {seconds_taken}!") + .as_str(), + ); + } else { + let output = match command.output() { + Ok(output) => { + String::from_utf8(output.stderr).unwrap_or_else(|_| { + String::from("Failed to fetch command output") + }) + } + Err(_) => String::from("Failed to fetch command output"), + }; + status.clear_status(); + status.update_status(&format!( + "Failed to compile model:\n{}", + output + )); + return Err(Error::Compile); + } + + // So, strictly speaking this is all unsound: + // - `Library::new` requires us to abide by the arbitrary requirements + // of any library initialization or termination routines. + // - `Library::get` requires us to specify the correct type for the + // model function. + // - The model function itself is `unsafe`, because it is a function + // from across an FFI interface. + // + // Typical models won't have initialization or termination routines (I + // think), should abide by the `ModelFn` signature, and might not do + // anything unsafe. But we have no way to know that the library the user + // told us to load actually does (I think). + // + // I don't know of a way to fix this. We should take this as motivation + // to switch to a better technique: + // https://github.com/hannobraun/Fornjot/issues/71 + let shape = unsafe { + let lib = libloading::Library::new(&self.lib_path)?; + + let version_pkg: libloading::Symbol RawVersion> = + lib.get(b"version_pkg")?; + + let version_pkg = version_pkg(); + if fj::version::VERSION_PKG != version_pkg.as_str() { + let host = String::from_utf8_lossy( + fj::version::VERSION_PKG.as_bytes(), + ) + .into_owned(); + let model = + String::from_utf8_lossy(version_pkg.as_str().as_bytes()) + .into_owned(); + + return Err(Error::VersionMismatch { host, model }); + } + + let init: libloading::Symbol = + lib.get(abi::INIT_FUNCTION_NAME.as_bytes())?; + + let mut host = Host::new(&self.parameters); + + match init(&mut abi::Host::from(&mut host)) { + abi::ffi_safe::Result::Ok(_metadata) => {} + abi::ffi_safe::Result::Err(e) => { + return Err(Error::InitializeModel(e.into())); + } + } + + let model = host.take_model().ok_or(Error::NoModelRegistered)?; + + model.shape(&host).map_err(Error::Shape)? + }; + + Ok(shape) + } +} + +fn package_associated_with_directory<'m>( + metadata: &'m cargo_metadata::Metadata, + dir: &Path, +) -> Result<&'m cargo_metadata::Package, Error> { + for pkg in metadata.workspace_packages() { + let crate_dir = pkg + .manifest_path + .parent() + .and_then(|p| p.canonicalize().ok()); + + if crate_dir.as_deref() == Some(dir) { + return Ok(pkg); + } + } + + Err(ambiguous_path_error(metadata, dir)) +} + +fn ambiguous_path_error( + metadata: &cargo_metadata::Metadata, + dir: &Path, +) -> Error { + let mut possible_paths = Vec::new(); + + for id in &metadata.workspace_members { + let cargo_toml = &metadata[id].manifest_path; + let crate_dir = cargo_toml + .parent() + .expect("A Cargo.toml always has a parent"); + // Try to make the path relative to the workspace root so error messages + // aren't super long. + let simplified_path = crate_dir + .strip_prefix(&metadata.workspace_root) + .unwrap_or(crate_dir); + + possible_paths.push(simplified_path.into()); + } + + Error::AmbiguousPath { + dir: dir.to_path_buf(), + possible_paths, + } +} + +/// An error that can occur when loading or reloading a model +#[derive(Debug, thiserror::Error)] +pub enum Error { + /// Model failed to compile + #[error("Error compiling model")] + Compile, + + /// I/O error while loading the model + #[error("I/O error while loading model")] + Io(#[from] io::Error), + + /// Failed to load the model's dynamic library + #[error("Error loading model from dynamic library")] + LibLoading(#[from] libloading::Error), + + /// Host version and model version do not match + #[error("Host version ({host}) and model version ({model}) do not match")] + VersionMismatch { + /// The host version + host: String, + + /// The model version + model: String, + }, + + /// Initializing a model failed. + #[error("Unable to initialize the model")] + InitializeModel(#[source] fj::models::Error), + + /// The user forgot to register a model when calling + /// [`fj::register_model!()`]. + #[error("No model was registered")] + NoModelRegistered, + + /// An error was returned from [`fj::models::Model::shape()`]. + #[error("Unable to determine the model's geometry")] + Shape(#[source] fj::models::Error), + + /// Error while watching the model code for changes + #[error("Error watching model for changes")] + Notify(#[from] notify::Error), + + /// An error occurred while trying to use evaluate + /// [`cargo_metadata::MetadataCommand`]. + #[error("Unable to determine the crate's metadata")] + CargoMetadata(#[from] cargo_metadata::Error), + + /// The user pointed us to a directory, but it doesn't look like that was + /// a crate root (i.e. the folder containing `Cargo.toml`). + #[error( + "It doesn't look like \"{}\" is a crate directory. Did you mean one of {}?", + dir.display(), + possible_paths.iter().map(|p| p.display().to_string()) + .collect::>() + .join(", ") + )] + AmbiguousPath { + /// The model directory supplied by the user. + dir: PathBuf, + /// The directories for each crate in the workspace, relative to the + /// workspace root. + possible_paths: Vec, + }, +} From c6e6352a12b778cc1f54c12b4b41acc13553959f Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 14:12:31 +0200 Subject: [PATCH 13/33] Simplify method name --- crates/fj-app/src/main.rs | 2 +- crates/fj-host/src/model.rs | 5 +---- crates/fj-host/src/watcher.rs | 2 +- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/crates/fj-app/src/main.rs b/crates/fj-app/src/main.rs index 0722c8474..cca0e6b9a 100644 --- a/crates/fj-app/src/main.rs +++ b/crates/fj-app/src/main.rs @@ -80,7 +80,7 @@ fn main() -> anyhow::Result<()> { if let Some(export_path) = args.export { // export only mode. just load model, process, export and exit - let shape = model.load_once(&mut status)?; + let shape = model.load(&mut status)?; let shape = shape_processor.process(&shape)?; export(&shape.mesh, &export_path)?; diff --git a/crates/fj-host/src/model.rs b/crates/fj-host/src/model.rs index 3b83aca92..7a019d2d5 100644 --- a/crates/fj-host/src/model.rs +++ b/crates/fj-host/src/model.rs @@ -57,10 +57,7 @@ impl Model { /// /// The passed arguments are provided to the model. Returns the shape that /// the model returns. - pub fn load_once( - &self, - status: &mut StatusReport, - ) -> Result { + pub fn load(&self, status: &mut StatusReport) -> Result { let manifest_path = self.manifest_path.display().to_string(); let mut command_root = Command::new("cargo"); diff --git a/crates/fj-host/src/watcher.rs b/crates/fj-host/src/watcher.rs index cbb9daf23..30c9d2b5f 100644 --- a/crates/fj-host/src/watcher.rs +++ b/crates/fj-host/src/watcher.rs @@ -97,7 +97,7 @@ impl Watcher { ) -> Result, Error> { match self.channel.try_recv() { Ok(()) => { - let shape = match self.model.load_once(status) { + let shape = match self.model.load(status) { Ok(shape) => shape, Err(Error::Compile) => { // An error is being displayed to the user via the From 70fa3747f66f8c5cf00104f68dba2f26e0485ce4 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 14:13:02 +0200 Subject: [PATCH 14/33] Simplify variable name --- crates/fj-host/src/model.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/fj-host/src/model.rs b/crates/fj-host/src/model.rs index 7a019d2d5..e1cdd2a29 100644 --- a/crates/fj-host/src/model.rs +++ b/crates/fj-host/src/model.rs @@ -60,9 +60,9 @@ impl Model { pub fn load(&self, status: &mut StatusReport) -> Result { let manifest_path = self.manifest_path.display().to_string(); - let mut command_root = Command::new("cargo"); + let mut command = Command::new("cargo"); - let command = command_root + let command = command .arg("rustc") .args(["--manifest-path", &manifest_path]) .args(["--crate-type", "cdylib"]); From 69f2bff21606bdcbc2f1e0f712b7ac6a5a8519a8 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 14:16:03 +0200 Subject: [PATCH 15/33] Fix duplicate command execution --- crates/fj-host/src/model.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/crates/fj-host/src/model.rs b/crates/fj-host/src/model.rs index e1cdd2a29..e3581be7b 100644 --- a/crates/fj-host/src/model.rs +++ b/crates/fj-host/src/model.rs @@ -82,14 +82,11 @@ impl Model { .as_str(), ); } else { - let output = match command.output() { - Ok(output) => { - String::from_utf8(output.stderr).unwrap_or_else(|_| { - String::from("Failed to fetch command output") - }) - } - Err(_) => String::from("Failed to fetch command output"), - }; + let output = + String::from_utf8(cargo_output.stderr).unwrap_or_else(|_| { + String::from("Failed to fetch command output") + }); + status.clear_status(); status.update_status(&format!( "Failed to compile model:\n{}", From 3b48b28dd9f943dd09f57c2dd469bb07f48ab9a5 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 14:16:49 +0200 Subject: [PATCH 16/33] Refactor --- crates/fj-host/src/model.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/crates/fj-host/src/model.rs b/crates/fj-host/src/model.rs index e3581be7b..572f02e5e 100644 --- a/crates/fj-host/src/model.rs +++ b/crates/fj-host/src/model.rs @@ -60,14 +60,11 @@ impl Model { pub fn load(&self, status: &mut StatusReport) -> Result { let manifest_path = self.manifest_path.display().to_string(); - let mut command = Command::new("cargo"); - - let command = command + let cargo_output = Command::new("cargo") .arg("rustc") .args(["--manifest-path", &manifest_path]) - .args(["--crate-type", "cdylib"]); - - let cargo_output = command.output()?; + .args(["--crate-type", "cdylib"]) + .output()?; let exit_status = cargo_output.status; if exit_status.success() { From b38ebba4673b2cd15ff56c64165d884c9e195e77 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 14:17:41 +0200 Subject: [PATCH 17/33] Refactor --- crates/fj-host/src/model.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/fj-host/src/model.rs b/crates/fj-host/src/model.rs index 572f02e5e..3a7fd2849 100644 --- a/crates/fj-host/src/model.rs +++ b/crates/fj-host/src/model.rs @@ -65,9 +65,8 @@ impl Model { .args(["--manifest-path", &manifest_path]) .args(["--crate-type", "cdylib"]) .output()?; - let exit_status = cargo_output.status; - if exit_status.success() { + if cargo_output.status.success() { let seconds_taken = str::from_utf8(&cargo_output.stderr) .unwrap() .rsplit_once(' ') From d1fa537917db3eb7f494e8da20d38383134a2d30 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 14:35:24 +0200 Subject: [PATCH 18/33] Add dependency on `crossbeam-channel` to `fj-host` --- Cargo.lock | 1 + crates/fj-host/Cargo.toml | 1 + 2 files changed, 2 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index dd7521047..e9be735fd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -994,6 +994,7 @@ name = "fj-host" version = "0.20.0" dependencies = [ "cargo_metadata", + "crossbeam-channel", "fj", "fj-interop", "libloading", diff --git a/crates/fj-host/Cargo.toml b/crates/fj-host/Cargo.toml index c02df4934..8cd36049e 100644 --- a/crates/fj-host/Cargo.toml +++ b/crates/fj-host/Cargo.toml @@ -13,6 +13,7 @@ categories.workspace = true [dependencies] cargo_metadata = "0.15.0" +crossbeam-channel = "0.5.6" fj.workspace = true fj-interop.workspace = true libloading = "0.7.2" From d9ba12ec485f7a682d02c70ccf3d1dc0609a2465 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 14:39:33 +0200 Subject: [PATCH 19/33] Make use of `crossbeam-channel` --- crates/fj-host/src/watcher.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/crates/fj-host/src/watcher.rs b/crates/fj-host/src/watcher.rs index 30c9d2b5f..403ec246d 100644 --- a/crates/fj-host/src/watcher.rs +++ b/crates/fj-host/src/watcher.rs @@ -1,5 +1,6 @@ -use std::{collections::HashSet, ffi::OsStr, sync::mpsc, thread}; +use std::{collections::HashSet, ffi::OsStr, thread}; +use crossbeam_channel::{Receiver, TryRecvError}; use fj_interop::status_report::StatusReport; use notify::Watcher as _; @@ -8,14 +9,14 @@ use crate::{Error, Model}; /// Watches a model for changes, reloading it continually pub struct Watcher { _watcher: Box, - channel: mpsc::Receiver<()>, + channel: Receiver<()>, model: Model, } impl Watcher { /// Watch the provided model for changes pub fn watch_model(model: Model) -> Result { - let (tx, rx) = mpsc::sync_channel(0); + let (tx, rx) = crossbeam_channel::bounded(0); let tx2 = tx.clone(); let watch_path = model.src_path(); @@ -112,11 +113,11 @@ impl Watcher { Ok(Some(shape)) } - Err(mpsc::TryRecvError::Empty) => { + Err(TryRecvError::Empty) => { // Nothing to receive from the channel. Ok(None) } - Err(mpsc::TryRecvError::Disconnected) => { + Err(TryRecvError::Disconnected) => { // The other end has disconnected. This is probably the result // of a panic on the other thread, or a program shutdown in // progress. In any case, not much we can do here. From 9d3abdd2cc51d2d1bb3be158dd7ca7695c6647fd Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 14:43:56 +0200 Subject: [PATCH 20/33] Return richer error information from `Model::load` --- crates/fj-host/src/model.rs | 12 +++++------- crates/fj-host/src/watcher.rs | 11 +++++++---- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/crates/fj-host/src/model.rs b/crates/fj-host/src/model.rs index 3a7fd2849..512f35d0c 100644 --- a/crates/fj-host/src/model.rs +++ b/crates/fj-host/src/model.rs @@ -83,12 +83,7 @@ impl Model { String::from("Failed to fetch command output") }); - status.clear_status(); - status.update_status(&format!( - "Failed to compile model:\n{}", - output - )); - return Err(Error::Compile); + return Err(Error::Compile { output }); } // So, strictly speaking this is all unsound: @@ -196,7 +191,10 @@ fn ambiguous_path_error( pub enum Error { /// Model failed to compile #[error("Error compiling model")] - Compile, + Compile { + /// The compiler output + output: String, + }, /// I/O error while loading the model #[error("I/O error while loading model")] diff --git a/crates/fj-host/src/watcher.rs b/crates/fj-host/src/watcher.rs index 403ec246d..161c13b90 100644 --- a/crates/fj-host/src/watcher.rs +++ b/crates/fj-host/src/watcher.rs @@ -100,10 +100,13 @@ impl Watcher { Ok(()) => { let shape = match self.model.load(status) { Ok(shape) => shape, - Err(Error::Compile) => { - // An error is being displayed to the user via the - // `StatusReport that is passed to `load_once` above, so - // no need to do anything else here. + Err(Error::Compile { output }) => { + status.clear_status(); + status.update_status(&format!( + "Failed to compile model:\n{}", + output + )); + return Ok(None); } Err(err) => { From a03ea4c0ce7063f4fd8212d031ddd4b5792eb104 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 14:45:49 +0200 Subject: [PATCH 21/33] Refactor --- crates/fj-host/src/model.rs | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/crates/fj-host/src/model.rs b/crates/fj-host/src/model.rs index 512f35d0c..2ebf92981 100644 --- a/crates/fj-host/src/model.rs +++ b/crates/fj-host/src/model.rs @@ -66,18 +66,7 @@ impl Model { .args(["--crate-type", "cdylib"]) .output()?; - if cargo_output.status.success() { - let seconds_taken = str::from_utf8(&cargo_output.stderr) - .unwrap() - .rsplit_once(' ') - .unwrap() - .1 - .trim(); - status.update_status( - format!("Model compiled successfully in {seconds_taken}!") - .as_str(), - ); - } else { + if !cargo_output.status.success() { let output = String::from_utf8(cargo_output.stderr).unwrap_or_else(|_| { String::from("Failed to fetch command output") @@ -86,6 +75,16 @@ impl Model { return Err(Error::Compile { output }); } + let seconds_taken = str::from_utf8(&cargo_output.stderr) + .unwrap() + .rsplit_once(' ') + .unwrap() + .1 + .trim(); + status.update_status( + format!("Model compiled successfully in {seconds_taken}!").as_str(), + ); + // So, strictly speaking this is all unsound: // - `Library::new` requires us to abide by the arbitrary requirements // of any library initialization or termination routines. From a691ea6f49a43aed8102954398d4ff938a9fe3bc Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 14:50:03 +0200 Subject: [PATCH 22/33] Return metadata from `Model::load` --- crates/fj-app/src/main.rs | 2 +- crates/fj-host/src/lib.rs | 2 +- crates/fj-host/src/model.rs | 21 +++++++++++++++++++-- crates/fj-host/src/watcher.rs | 2 +- 4 files changed, 22 insertions(+), 5 deletions(-) diff --git a/crates/fj-app/src/main.rs b/crates/fj-app/src/main.rs index cca0e6b9a..582ac7235 100644 --- a/crates/fj-app/src/main.rs +++ b/crates/fj-app/src/main.rs @@ -81,7 +81,7 @@ fn main() -> anyhow::Result<()> { // export only mode. just load model, process, export and exit let shape = model.load(&mut status)?; - let shape = shape_processor.process(&shape)?; + let shape = shape_processor.process(&shape.shape)?; export(&shape.mesh, &export_path)?; diff --git a/crates/fj-host/src/lib.rs b/crates/fj-host/src/lib.rs index ec5dfc298..9db988908 100644 --- a/crates/fj-host/src/lib.rs +++ b/crates/fj-host/src/lib.rs @@ -22,7 +22,7 @@ mod platform; mod watcher; pub use self::{ - model::{Error, Model}, + model::{Error, LoadedShape, Model}, parameters::Parameters, watcher::Watcher, }; diff --git a/crates/fj-host/src/model.rs b/crates/fj-host/src/model.rs index 2ebf92981..306e9471d 100644 --- a/crates/fj-host/src/model.rs +++ b/crates/fj-host/src/model.rs @@ -57,7 +57,10 @@ impl Model { /// /// The passed arguments are provided to the model. Returns the shape that /// the model returns. - pub fn load(&self, status: &mut StatusReport) -> Result { + pub fn load( + &self, + status: &mut StatusReport, + ) -> Result { let manifest_path = self.manifest_path.display().to_string(); let cargo_output = Command::new("cargo") @@ -137,10 +140,24 @@ impl Model { model.shape(&host).map_err(Error::Shape)? }; - Ok(shape) + Ok(LoadedShape { + shape, + compile_time: seconds_taken.into(), + }) } } +/// A loaded shape, together with additional metadata +/// +/// See [`Model::load`]. +pub struct LoadedShape { + /// The shape + pub shape: fj::Shape, + + /// The time it took to compile the shape, from the Cargo output + pub compile_time: String, +} + fn package_associated_with_directory<'m>( metadata: &'m cargo_metadata::Metadata, dir: &Path, diff --git a/crates/fj-host/src/watcher.rs b/crates/fj-host/src/watcher.rs index 161c13b90..f4b9d25f1 100644 --- a/crates/fj-host/src/watcher.rs +++ b/crates/fj-host/src/watcher.rs @@ -114,7 +114,7 @@ impl Watcher { } }; - Ok(Some(shape)) + Ok(Some(shape.shape)) } Err(TryRecvError::Empty) => { // Nothing to receive from the channel. From 9292058d4889448cc1a47cb55684beabd54756fb Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 14:52:09 +0200 Subject: [PATCH 23/33] Create compile time status update in `Watcher` --- crates/fj-app/src/main.rs | 4 ++-- crates/fj-host/src/model.rs | 9 +-------- crates/fj-host/src/watcher.rs | 10 +++++++++- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/crates/fj-app/src/main.rs b/crates/fj-app/src/main.rs index 582ac7235..6a96e86b5 100644 --- a/crates/fj-app/src/main.rs +++ b/crates/fj-app/src/main.rs @@ -29,7 +29,7 @@ use tracing_subscriber::EnvFilter; use crate::{args::Args, config::Config}; fn main() -> anyhow::Result<()> { - let mut status = StatusReport::new(); + let status = StatusReport::new(); // Respect `RUST_LOG`. If that's not defined or erroneous, log warnings and // above. // @@ -80,7 +80,7 @@ fn main() -> anyhow::Result<()> { if let Some(export_path) = args.export { // export only mode. just load model, process, export and exit - let shape = model.load(&mut status)?; + let shape = model.load()?; let shape = shape_processor.process(&shape.shape)?; export(&shape.mesh, &export_path)?; diff --git a/crates/fj-host/src/model.rs b/crates/fj-host/src/model.rs index 306e9471d..a27039018 100644 --- a/crates/fj-host/src/model.rs +++ b/crates/fj-host/src/model.rs @@ -6,7 +6,6 @@ use std::{ }; use fj::{abi, version::RawVersion}; -use fj_interop::status_report::StatusReport; use crate::{host::Host, platform::HostPlatform, Parameters}; @@ -57,10 +56,7 @@ impl Model { /// /// The passed arguments are provided to the model. Returns the shape that /// the model returns. - pub fn load( - &self, - status: &mut StatusReport, - ) -> Result { + pub fn load(&self) -> Result { let manifest_path = self.manifest_path.display().to_string(); let cargo_output = Command::new("cargo") @@ -84,9 +80,6 @@ impl Model { .unwrap() .1 .trim(); - status.update_status( - format!("Model compiled successfully in {seconds_taken}!").as_str(), - ); // So, strictly speaking this is all unsound: // - `Library::new` requires us to abide by the arbitrary requirements diff --git a/crates/fj-host/src/watcher.rs b/crates/fj-host/src/watcher.rs index f4b9d25f1..53251d7c1 100644 --- a/crates/fj-host/src/watcher.rs +++ b/crates/fj-host/src/watcher.rs @@ -98,7 +98,7 @@ impl Watcher { ) -> Result, Error> { match self.channel.try_recv() { Ok(()) => { - let shape = match self.model.load(status) { + let shape = match self.model.load() { Ok(shape) => shape, Err(Error::Compile { output }) => { status.clear_status(); @@ -114,6 +114,14 @@ impl Watcher { } }; + status.update_status( + format!( + "Model compiled successfully in {}!", + shape.compile_time + ) + .as_str(), + ); + Ok(Some(shape.shape)) } Err(TryRecvError::Empty) => { From 9298a35008fa14cbd178e67ffa0a984ca5b928a0 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 14:57:23 +0200 Subject: [PATCH 24/33] Don't clear status before adding error Not sure if this is better, but it is simpler. This will help me with upcoming changes. --- crates/fj-host/src/watcher.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/fj-host/src/watcher.rs b/crates/fj-host/src/watcher.rs index 53251d7c1..c257d5d75 100644 --- a/crates/fj-host/src/watcher.rs +++ b/crates/fj-host/src/watcher.rs @@ -101,7 +101,6 @@ impl Watcher { let shape = match self.model.load() { Ok(shape) => shape, Err(Error::Compile { output }) => { - status.clear_status(); status.update_status(&format!( "Failed to compile model:\n{}", output From 8adaba8a14a70dd8821a95c4cbd82dce2918529b Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 15:05:48 +0200 Subject: [PATCH 25/33] Provide `Watcher` status updates via channel --- crates/fj-host/src/lib.rs | 2 +- crates/fj-host/src/watcher.rs | 46 +++++++++++++++++++++++------------ crates/fj-window/src/run.rs | 28 +++++++++++++++++++-- 3 files changed, 58 insertions(+), 18 deletions(-) diff --git a/crates/fj-host/src/lib.rs b/crates/fj-host/src/lib.rs index 9db988908..9f39e6281 100644 --- a/crates/fj-host/src/lib.rs +++ b/crates/fj-host/src/lib.rs @@ -24,5 +24,5 @@ mod watcher; pub use self::{ model::{Error, LoadedShape, Model}, parameters::Parameters, - watcher::Watcher, + watcher::{Watcher, WatcherEvent}, }; diff --git a/crates/fj-host/src/watcher.rs b/crates/fj-host/src/watcher.rs index c257d5d75..989ec6638 100644 --- a/crates/fj-host/src/watcher.rs +++ b/crates/fj-host/src/watcher.rs @@ -1,7 +1,6 @@ use std::{collections::HashSet, ffi::OsStr, thread}; -use crossbeam_channel::{Receiver, TryRecvError}; -use fj_interop::status_report::StatusReport; +use crossbeam_channel::{Receiver, Sender, TryRecvError}; use notify::Watcher as _; use crate::{Error, Model}; @@ -11,11 +10,16 @@ pub struct Watcher { _watcher: Box, channel: Receiver<()>, model: Model, + + event_tx: Sender, + event_rx: Receiver, } impl Watcher { /// Watch the provided model for changes pub fn watch_model(model: Model) -> Result { + let (event_tx, event_rx) = crossbeam_channel::bounded(1); + let (tx, rx) = crossbeam_channel::bounded(0); let tx2 = tx.clone(); @@ -85,26 +89,33 @@ impl Watcher { _watcher: Box::new(watcher), channel: rx, model, + + event_tx, + event_rx, }) } + /// Access a channel for receiving status updates + pub fn events(&self) -> Receiver { + self.event_rx.clone() + } + /// Receive an updated shape that the reloaded model created /// /// Returns `None`, if the model has not changed since the last time this /// method was called. - pub fn receive_shape( - &self, - status: &mut StatusReport, - ) -> Result, Error> { + pub fn receive_shape(&self) -> Result, Error> { match self.channel.try_recv() { Ok(()) => { let shape = match self.model.load() { Ok(shape) => shape, Err(Error::Compile { output }) => { - status.update_status(&format!( - "Failed to compile model:\n{}", - output - )); + self.event_tx + .send(WatcherEvent::StatusUpdate(format!( + "Failed to compile model:\n{}", + output + ))) + .expect("Expected channel to never disconnect"); return Ok(None); } @@ -113,13 +124,12 @@ impl Watcher { } }; - status.update_status( - format!( + self.event_tx + .send(WatcherEvent::StatusUpdate(format!( "Model compiled successfully in {}!", shape.compile_time - ) - .as_str(), - ); + ))) + .expect("Expected channel to never disconnect"); Ok(Some(shape.shape)) } @@ -136,3 +146,9 @@ impl Watcher { } } } + +/// An event emitted by the [`Watcher`] +pub enum WatcherEvent { + /// A status update about the model + StatusUpdate(String), +} diff --git a/crates/fj-window/src/run.rs b/crates/fj-window/src/run.rs index 000fc9b67..3fae87d7d 100644 --- a/crates/fj-window/src/run.rs +++ b/crates/fj-window/src/run.rs @@ -5,7 +5,7 @@ use std::error; -use fj_host::Watcher; +use fj_host::{Watcher, WatcherEvent}; use fj_interop::status_report::StatusReport; use fj_operations::shape_processor::ShapeProcessor; use fj_viewer::{ @@ -36,6 +36,8 @@ pub fn run( let window = Window::new(&event_loop)?; let mut viewer = block_on(Viewer::new(&window))?; + let events = watcher.events(); + let mut held_mouse_button = None; let mut egui_winit_state = egui_winit::State::new(&event_loop); @@ -49,7 +51,29 @@ pub fn run( event_loop.run(move |event, _, control_flow| { trace!("Handling event: {:?}", event); - match watcher.receive_shape(&mut status) { + loop { + let event = events + .try_recv() + .map_err(|err| { + if err.is_disconnected() { + panic!("Expected channel to never disconnect"); + } + }) + .ok(); + + let event = match event { + Some(status_update) => status_update, + None => break, + }; + + match event { + WatcherEvent::StatusUpdate(status_update) => { + status.update_status(&status_update) + } + } + } + + match watcher.receive_shape() { Ok(shape) => { if let Some(shape) = shape { match shape_processor.process(&shape) { From a7ceb635c2fbaae9a2d9ff9f29cae86ba9038df7 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 15:17:35 +0200 Subject: [PATCH 26/33] Provide loaded shapes via channel --- crates/fj-host/src/watcher.rs | 18 ++++++++-------- crates/fj-window/src/run.rs | 39 ++++++++++++++++++----------------- 2 files changed, 29 insertions(+), 28 deletions(-) diff --git a/crates/fj-host/src/watcher.rs b/crates/fj-host/src/watcher.rs index 989ec6638..f453b095d 100644 --- a/crates/fj-host/src/watcher.rs +++ b/crates/fj-host/src/watcher.rs @@ -3,7 +3,7 @@ use std::{collections::HashSet, ffi::OsStr, thread}; use crossbeam_channel::{Receiver, Sender, TryRecvError}; use notify::Watcher as _; -use crate::{Error, Model}; +use crate::{Error, LoadedShape, Model}; /// Watches a model for changes, reloading it continually pub struct Watcher { @@ -104,7 +104,7 @@ impl Watcher { /// /// Returns `None`, if the model has not changed since the last time this /// method was called. - pub fn receive_shape(&self) -> Result, Error> { + pub fn receive_shape(&self) -> Result<(), Error> { match self.channel.try_recv() { Ok(()) => { let shape = match self.model.load() { @@ -117,7 +117,7 @@ impl Watcher { ))) .expect("Expected channel to never disconnect"); - return Ok(None); + return Ok(()); } Err(err) => { return Err(err); @@ -125,17 +125,14 @@ impl Watcher { }; self.event_tx - .send(WatcherEvent::StatusUpdate(format!( - "Model compiled successfully in {}!", - shape.compile_time - ))) + .send(WatcherEvent::Shape(shape)) .expect("Expected channel to never disconnect"); - Ok(Some(shape.shape)) + Ok(()) } Err(TryRecvError::Empty) => { // Nothing to receive from the channel. - Ok(None) + Ok(()) } Err(TryRecvError::Disconnected) => { // The other end has disconnected. This is probably the result @@ -151,4 +148,7 @@ impl Watcher { pub enum WatcherEvent { /// A status update about the model StatusUpdate(String), + + /// A shape has been loaded from the model + Shape(LoadedShape), } diff --git a/crates/fj-window/src/run.rs b/crates/fj-window/src/run.rs index 3fae87d7d..c92ffe9de 100644 --- a/crates/fj-window/src/run.rs +++ b/crates/fj-window/src/run.rs @@ -70,13 +70,13 @@ pub fn run( WatcherEvent::StatusUpdate(status_update) => { status.update_status(&status_update) } - } - } + WatcherEvent::Shape(shape) => { + status.update_status(&format!( + "Model compiled successfully in {}!", + shape.compile_time + )); - match watcher.receive_shape() { - Ok(shape) => { - if let Some(shape) = shape { - match shape_processor.process(&shape) { + match shape_processor.process(&shape.shape) { Ok(shape) => { viewer.handle_shape_update(shape); } @@ -98,24 +98,25 @@ pub fn run( } } } - Err(err) => { - // Can be cleaned up, once `Report` is stable: - // https://doc.rust-lang.org/std/error/struct.Report.html + } - println!("Error receiving updated shape: {}", err); + if let Err(err) = watcher.receive_shape() { + // Can be cleaned up, once `Report` is stable: + // https://doc.rust-lang.org/std/error/struct.Report.html - let mut current_err = &err as &dyn error::Error; - while let Some(err) = current_err.source() { - println!(); - println!("Caused by:"); - println!(" {}", err); + println!("Error receiving updated shape: {}", err); - current_err = err; - } + let mut current_err = &err as &dyn error::Error; + while let Some(err) = current_err.source() { + println!(); + println!("Caused by:"); + println!(" {}", err); - *control_flow = ControlFlow::Exit; - return; + current_err = err; } + + *control_flow = ControlFlow::Exit; + return; } if let Event::WindowEvent { event, .. } = &event { From bd38060b763e1f3bc0d38312e631bd3641e9d080 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 15:22:42 +0200 Subject: [PATCH 27/33] Provide `Watcher` errors via channel --- crates/fj-host/src/watcher.rs | 15 +++++++++------ crates/fj-window/src/run.rs | 31 ++++++++++++++++--------------- 2 files changed, 25 insertions(+), 21 deletions(-) diff --git a/crates/fj-host/src/watcher.rs b/crates/fj-host/src/watcher.rs index f453b095d..01846fccd 100644 --- a/crates/fj-host/src/watcher.rs +++ b/crates/fj-host/src/watcher.rs @@ -104,7 +104,7 @@ impl Watcher { /// /// Returns `None`, if the model has not changed since the last time this /// method was called. - pub fn receive_shape(&self) -> Result<(), Error> { + pub fn receive_shape(&self) { match self.channel.try_recv() { Ok(()) => { let shape = match self.model.load() { @@ -117,22 +117,22 @@ impl Watcher { ))) .expect("Expected channel to never disconnect"); - return Ok(()); + return; } Err(err) => { - return Err(err); + self.event_tx + .send(WatcherEvent::Error(err)) + .expect("Expected channel to never disconnect"); + return; } }; self.event_tx .send(WatcherEvent::Shape(shape)) .expect("Expected channel to never disconnect"); - - Ok(()) } Err(TryRecvError::Empty) => { // Nothing to receive from the channel. - Ok(()) } Err(TryRecvError::Disconnected) => { // The other end has disconnected. This is probably the result @@ -151,4 +151,7 @@ pub enum WatcherEvent { /// A shape has been loaded from the model Shape(LoadedShape), + + /// An error + Error(Error), } diff --git a/crates/fj-window/src/run.rs b/crates/fj-window/src/run.rs index c92ffe9de..c0d7c1503 100644 --- a/crates/fj-window/src/run.rs +++ b/crates/fj-window/src/run.rs @@ -97,28 +97,29 @@ pub fn run( } } } - } - } + WatcherEvent::Error(err) => { + // Can be cleaned up, once `Report` is stable: + // https://doc.rust-lang.org/std/error/struct.Report.html - if let Err(err) = watcher.receive_shape() { - // Can be cleaned up, once `Report` is stable: - // https://doc.rust-lang.org/std/error/struct.Report.html + println!("Error receiving updated shape: {}", err); - println!("Error receiving updated shape: {}", 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; + *control_flow = ControlFlow::Exit; + return; + } } - - *control_flow = ControlFlow::Exit; - return; } + watcher.receive_shape(); + if let Event::WindowEvent { event, .. } = &event { // In theory we could/should check if `egui` wants "exclusive" use // of this event here. But with the current integration with Fornjot From 19bdf8033c47a86e4a4fab00b1f7caa25e07b81d Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 15:30:15 +0200 Subject: [PATCH 28/33] Make variable names more explicit --- crates/fj-host/src/watcher.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/crates/fj-host/src/watcher.rs b/crates/fj-host/src/watcher.rs index 01846fccd..20bfc2f23 100644 --- a/crates/fj-host/src/watcher.rs +++ b/crates/fj-host/src/watcher.rs @@ -20,8 +20,8 @@ impl Watcher { pub fn watch_model(model: Model) -> Result { let (event_tx, event_rx) = crossbeam_channel::bounded(1); - let (tx, rx) = crossbeam_channel::bounded(0); - let tx2 = tx.clone(); + let (watch_tx, watch_rx) = crossbeam_channel::bounded(0); + let watch_tx_2 = watch_tx.clone(); let watch_path = model.src_path(); @@ -70,7 +70,7 @@ impl Watcher { // application is being shut down. // // Either way, not much we can do about it here. - tx.send(()).expect("Channel is disconnected"); + watch_tx.send(()).expect("Channel is disconnected"); } }, )?; @@ -83,11 +83,13 @@ impl Watcher { // // Will panic, if the receiving end has panicked. Not much we can do // about that, if it happened. - thread::spawn(move || tx2.send(()).expect("Channel is disconnected")); + thread::spawn(move || { + watch_tx_2.send(()).expect("Channel is disconnected") + }); Ok(Self { _watcher: Box::new(watcher), - channel: rx, + channel: watch_rx, model, event_tx, From 4d88782235db2cb7a4fd728375361703c9aa1881 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 15:40:51 +0200 Subject: [PATCH 29/33] Expand comment --- crates/fj-host/src/watcher.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/fj-host/src/watcher.rs b/crates/fj-host/src/watcher.rs index 20bfc2f23..da189fe52 100644 --- a/crates/fj-host/src/watcher.rs +++ b/crates/fj-host/src/watcher.rs @@ -81,6 +81,9 @@ impl Watcher { // watching, we'll trigger the initial load here, after having started // watching. // + // This happens in a separate thread, because the channel is bounded and + // has no buffer. + // // Will panic, if the receiving end has panicked. Not much we can do // about that, if it happened. thread::spawn(move || { From 16a097bee9c9a8cb3779a319632e0cdc0d31bbbd Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 15:41:30 +0200 Subject: [PATCH 30/33] Derive `Debug` for `ProcessedShape` --- crates/fj-interop/src/debug.rs | 4 ++-- crates/fj-interop/src/processed_shape.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/fj-interop/src/debug.rs b/crates/fj-interop/src/debug.rs index c8086df82..035d1a23a 100644 --- a/crates/fj-interop/src/debug.rs +++ b/crates/fj-interop/src/debug.rs @@ -7,7 +7,7 @@ use fj_math::{Point, Segment}; /// Debug info from the CAD kernel that can be visualized -#[derive(Clone, Default)] +#[derive(Clone, Debug, Default)] pub struct DebugInfo { /// Rays being used during face triangulation pub triangle_edge_checks: Vec, @@ -30,7 +30,7 @@ impl DebugInfo { } /// Record of a check to determine if a triangle edge is within a face -#[derive(Clone)] +#[derive(Clone, Debug)] pub struct TriangleEdgeCheck { /// The origin of the ray used to perform the check pub origin: Point<3>, diff --git a/crates/fj-interop/src/processed_shape.rs b/crates/fj-interop/src/processed_shape.rs index 5042edb62..6a112bd7d 100644 --- a/crates/fj-interop/src/processed_shape.rs +++ b/crates/fj-interop/src/processed_shape.rs @@ -5,7 +5,7 @@ use fj_math::{Aabb, Point}; use crate::{debug::DebugInfo, mesh::Mesh}; /// A processed shape -#[derive(Clone)] +#[derive(Clone, Debug)] pub struct ProcessedShape { /// The axis-aligned bounding box of the shape pub aabb: Aabb<3>, From 11649978a1140b4a10405b6fe925454581f2114e Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 15:51:02 +0200 Subject: [PATCH 31/33] Be more explicit on how to init camera plances --- crates/fj-viewer/src/camera.rs | 104 +++++++++++++++------------------ crates/fj-viewer/src/viewer.rs | 6 +- 2 files changed, 53 insertions(+), 57 deletions(-) diff --git a/crates/fj-viewer/src/camera.rs b/crates/fj-viewer/src/camera.rs index 044909973..5cb41ed40 100644 --- a/crates/fj-viewer/src/camera.rs +++ b/crates/fj-viewer/src/camera.rs @@ -25,9 +25,6 @@ pub struct Camera { /// The locational part of the transform pub translation: Transform, - - /// Tracks whether the camera has been initialized - is_initialized: bool, } impl Camera { @@ -44,8 +41,6 @@ impl Camera { rotation: Transform::identity(), translation: Transform::identity(), - - is_initialized: false, } } @@ -133,59 +128,56 @@ impl Camera { transform } + /// Initialize the planes + /// + /// Call this, if a shape is available for the first time. + pub fn init_planes(&mut self, aabb: &Aabb<3>) { + let initial_distance = { + // Let's make sure we choose a distance, so that the model fills + // most of the screen. + // + // To do that, first compute the model's highest point, as well + // as the furthest point from the origin, in x and y. + let highest_point = aabb.max.z; + let furthest_point = + [aabb.min.x.abs(), aabb.max.x, aabb.min.y.abs(), aabb.max.y] + .into_iter() + .reduce(Scalar::max) + // `reduce` can only return `None`, if there are no items in + // the iterator. And since we're creating an array full of + // items above, we know this can't panic. + .expect("Array should have contained items"); + + // The actual furthest point is not far enough. We don't want + // the model to fill the whole screen. + let furthest_point = furthest_point * 2.; + + // Having computed those points, figuring out how far the camera + // needs to be from the model is just a bit of trigonometry. + let distance_from_model = + furthest_point / (Self::INITIAL_FIELD_OF_VIEW_IN_X / 2.).atan(); + + // And finally, the distance from the origin is trivial now. + highest_point + distance_from_model + }; + + let initial_offset = { + let mut offset = aabb.center(); + offset.z = Scalar::ZERO; + -offset + }; + + let translation = Transform::translation([ + initial_offset.x, + initial_offset.y, + -initial_distance, + ]); + + self.translation = translation; + } + /// Update the max and minimum rendering distance for this camera. pub fn update_planes(&mut self, aabb: &Aabb<3>) { - if !self.is_initialized { - let initial_distance = { - // Let's make sure we choose a distance, so that the model fills - // most of the screen. - // - // To do that, first compute the model's highest point, as well - // as the furthest point from the origin, in x and y. - let highest_point = aabb.max.z; - let furthest_point = [ - aabb.min.x.abs(), - aabb.max.x, - aabb.min.y.abs(), - aabb.max.y, - ] - .into_iter() - .reduce(Scalar::max) - // `reduce` can only return `None`, if there are no items in - // the iterator. And since we're creating an array full of - // items above, we know this can't panic. - .expect("Array should have contained items"); - - // The actual furthest point is not far enough. We don't want - // the model to fill the whole screen. - let furthest_point = furthest_point * 2.; - - // Having computed those points, figuring out how far the camera - // needs to be from the model is just a bit of trigonometry. - let distance_from_model = furthest_point - / (Self::INITIAL_FIELD_OF_VIEW_IN_X / 2.).atan(); - - // And finally, the distance from the origin is trivial now. - highest_point + distance_from_model - }; - - let initial_offset = { - let mut offset = aabb.center(); - offset.z = Scalar::ZERO; - -offset - }; - - let translation = Transform::translation([ - initial_offset.x, - initial_offset.y, - -initial_distance, - ]); - - self.translation = translation * self.translation; - - self.is_initialized = true; - } - let view_transform = self.camera_to_model(); let view_direction = Vector::from([0., 0., -1.]); diff --git a/crates/fj-viewer/src/viewer.rs b/crates/fj-viewer/src/viewer.rs index d3456cb68..c2ec17af6 100644 --- a/crates/fj-viewer/src/viewer.rs +++ b/crates/fj-viewer/src/viewer.rs @@ -77,7 +77,11 @@ impl Viewer { pub fn handle_shape_update(&mut self, shape: ProcessedShape) { self.renderer .update_geometry((&shape.mesh).into(), (&shape.debug_info).into()); - self.shape = Some(shape); + + let aabb = shape.aabb; + if self.shape.replace(shape).is_none() { + self.camera.init_planes(&aabb) + } } /// Handle an input event From edca1483851b7b45979c4ebda14a005f6d8b1a6d Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 15:52:24 +0200 Subject: [PATCH 32/33] Don't block window thread when compiling model --- crates/fj-host/src/watcher.rs | 86 ++++++++++++++--------------------- crates/fj-window/src/run.rs | 2 - 2 files changed, 34 insertions(+), 54 deletions(-) diff --git a/crates/fj-host/src/watcher.rs b/crates/fj-host/src/watcher.rs index da189fe52..4af153239 100644 --- a/crates/fj-host/src/watcher.rs +++ b/crates/fj-host/src/watcher.rs @@ -1,6 +1,6 @@ use std::{collections::HashSet, ffi::OsStr, thread}; -use crossbeam_channel::{Receiver, Sender, TryRecvError}; +use crossbeam_channel::Receiver; use notify::Watcher as _; use crate::{Error, LoadedShape, Model}; @@ -8,10 +8,6 @@ use crate::{Error, LoadedShape, Model}; /// Watches a model for changes, reloading it continually pub struct Watcher { _watcher: Box, - channel: Receiver<()>, - model: Model, - - event_tx: Sender, event_rx: Receiver, } @@ -90,12 +86,41 @@ impl Watcher { watch_tx_2.send(()).expect("Channel is disconnected") }); + // Listen on the watcher channel and rebuild the model. This happens in + // a separate thread from the watcher to allow us to trigger compiles + // without the watcher having registered a change, as is done above. + thread::spawn(move || loop { + let () = watch_rx + .recv() + .expect("Expected channel to never disconnect"); + + let shape = match model.load() { + Ok(shape) => shape, + Err(Error::Compile { output }) => { + event_tx + .send(WatcherEvent::StatusUpdate(format!( + "Failed to compile model:\n{}", + output + ))) + .expect("Expected channel to never disconnect"); + + return; + } + Err(err) => { + event_tx + .send(WatcherEvent::Error(err)) + .expect("Expected channel to never disconnect"); + return; + } + }; + + event_tx + .send(WatcherEvent::Shape(shape)) + .expect("Expected channel to never disconnect"); + }); + Ok(Self { _watcher: Box::new(watcher), - channel: watch_rx, - model, - - event_tx, event_rx, }) } @@ -104,49 +129,6 @@ impl Watcher { pub fn events(&self) -> Receiver { self.event_rx.clone() } - - /// Receive an updated shape that the reloaded model created - /// - /// Returns `None`, if the model has not changed since the last time this - /// method was called. - pub fn receive_shape(&self) { - match self.channel.try_recv() { - Ok(()) => { - let shape = match self.model.load() { - Ok(shape) => shape, - Err(Error::Compile { output }) => { - self.event_tx - .send(WatcherEvent::StatusUpdate(format!( - "Failed to compile model:\n{}", - output - ))) - .expect("Expected channel to never disconnect"); - - return; - } - Err(err) => { - self.event_tx - .send(WatcherEvent::Error(err)) - .expect("Expected channel to never disconnect"); - return; - } - }; - - self.event_tx - .send(WatcherEvent::Shape(shape)) - .expect("Expected channel to never disconnect"); - } - Err(TryRecvError::Empty) => { - // Nothing to receive from the channel. - } - Err(TryRecvError::Disconnected) => { - // The other end has disconnected. This is probably the result - // of a panic on the other thread, or a program shutdown in - // progress. In any case, not much we can do here. - panic!(); - } - } - } } /// An event emitted by the [`Watcher`] diff --git a/crates/fj-window/src/run.rs b/crates/fj-window/src/run.rs index c0d7c1503..ad6bd589b 100644 --- a/crates/fj-window/src/run.rs +++ b/crates/fj-window/src/run.rs @@ -118,8 +118,6 @@ pub fn run( } } - watcher.receive_shape(); - if let Event::WindowEvent { event, .. } = &event { // In theory we could/should check if `egui` wants "exclusive" use // of this event here. But with the current integration with Fornjot From 413524c944f4131aa357b335a8f7b0d181b43d31 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 20 Oct 2022 15:53:12 +0200 Subject: [PATCH 33/33] Remove unnecessary channel buffer --- crates/fj-host/src/watcher.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/fj-host/src/watcher.rs b/crates/fj-host/src/watcher.rs index 4af153239..0456ef6fc 100644 --- a/crates/fj-host/src/watcher.rs +++ b/crates/fj-host/src/watcher.rs @@ -14,7 +14,7 @@ pub struct Watcher { impl Watcher { /// Watch the provided model for changes pub fn watch_model(model: Model) -> Result { - let (event_tx, event_rx) = crossbeam_channel::bounded(1); + let (event_tx, event_rx) = crossbeam_channel::bounded(0); let (watch_tx, watch_rx) = crossbeam_channel::bounded(0); let watch_tx_2 = watch_tx.clone();