-
Notifications
You must be signed in to change notification settings - Fork 50
Throttled progress/cancel callback for load_obj_buf #79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -241,8 +241,10 @@ use std::{ | |
| fmt, | ||
| fs::File, | ||
| io::{prelude::*, BufReader}, | ||
| ops::ControlFlow, | ||
| path::{Path, PathBuf}, | ||
| str::{FromStr, SplitWhitespace}, | ||
| sync::Arc, | ||
| }; | ||
|
|
||
| #[cfg(feature = "use_f64")] | ||
|
|
@@ -276,6 +278,7 @@ pub const GPU_LOAD_OPTIONS: LoadOptions = LoadOptions { | |
| triangulate: true, | ||
| ignore_points: true, | ||
| ignore_lines: true, | ||
| progress_callback: None, | ||
| }; | ||
|
|
||
| /// Typical [`LoadOptions`] for using meshes with an offline rendeder. | ||
|
|
@@ -293,6 +296,7 @@ pub const OFFLINE_RENDERING_LOAD_OPTIONS: LoadOptions = LoadOptions { | |
| triangulate: false, | ||
| ignore_points: true, | ||
| ignore_lines: true, | ||
| progress_callback: None, | ||
| }; | ||
|
|
||
| /// A mesh made up of triangles loaded from some `OBJ` file. | ||
|
|
@@ -404,6 +408,61 @@ pub struct Mesh { | |
| pub material_id: Option<usize>, | ||
| } | ||
|
|
||
| /// A snapshot of progress made so far while parsing an `OBJ` buffer in | ||
| /// [`load_obj_buf()`]. | ||
| /// | ||
| /// Passed to a [`LoadProgressCallback`] registered via | ||
| /// [`LoadOptions::progress_callback`]. The callback is throttled -- it is not | ||
| /// invoked for every line read. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub struct LoadProgress { | ||
| /// Number of lines read from the buffer so far. | ||
| pub lines_read: u64, | ||
| /// Number of bytes read from the buffer so far. | ||
| /// | ||
| /// This is a lower bound: line-ending bytes stripped by | ||
| /// [`BufRead::lines()`](std::io::BufRead::lines) are not counted, since | ||
| /// they are not seen by the parser. | ||
| pub bytes_read: u64, | ||
| } | ||
|
|
||
| /// A throttled progress-report and cooperative-cancellation callback. | ||
| /// | ||
| /// Wraps a closure that is invoked periodically while [`load_obj_buf()`] | ||
| /// parses a buffer. Returning [`ControlFlow::Break`] from the closure aborts | ||
| /// the load and causes [`load_obj_buf()`] to return | ||
| /// [`LoadError::Cancelled`]. | ||
| /// | ||
| /// Register one via [`LoadOptions::progress_callback`]. | ||
| #[derive(Clone)] | ||
| pub struct LoadProgressCallback(Arc<LoadProgressCallbackFn>); | ||
|
|
||
| type LoadProgressCallbackFn = dyn Fn(&LoadProgress) -> ControlFlow<()> + Send + Sync; | ||
|
|
||
| impl LoadProgressCallback { | ||
| /// Creates a new [`LoadProgressCallback`] from a closure. | ||
| pub fn new(f: impl Fn(&LoadProgress) -> ControlFlow<()> + Send + Sync + 'static) -> Self { | ||
| Self(Arc::new(f)) | ||
| } | ||
|
|
||
| /// Invokes the wrapped closure with the given `progress` snapshot. | ||
| fn call(&self, progress: &LoadProgress) -> ControlFlow<()> { | ||
| (self.0)(progress) | ||
| } | ||
| } | ||
|
|
||
| impl fmt::Debug for LoadProgressCallback { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| f.write_str("LoadProgressCallback(..)") | ||
| } | ||
| } | ||
|
|
||
| impl PartialEq for LoadProgressCallback { | ||
| fn eq(&self, _other: &Self) -> bool { | ||
| true // Not data. | ||
| } | ||
| } | ||
|
|
||
| /// Options for processing the mesh during loading. | ||
| /// | ||
| /// Passed to [`load_obj()`], [`load_obj_buf()`] and [`load_obj_buf_async()`]. | ||
|
|
@@ -427,7 +486,7 @@ pub struct Mesh { | |
| /// * [`OFFLINE_RENDERING_LOAD_OPTIONS`] – if you're rendering meshes with e.g. | ||
| /// an offline path tracer or the like. | ||
| #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))] | ||
| #[derive(Debug, Default, Clone, Copy)] | ||
| #[derive(Debug, Default, Clone)] | ||
| pub struct LoadOptions { | ||
| /// Merge identical positions. | ||
| /// | ||
|
|
@@ -529,6 +588,16 @@ pub struct LoadOptions { | |
| /// Polygon meshes that contains faces with two vertices only usually do so | ||
| /// because of bad topology. | ||
| pub ignore_lines: bool, | ||
| /// Optional progress-report and cooperative-cancellation callback. | ||
| /// | ||
| /// If set, [`load_obj_buf()`] invokes it periodically (throttled; not on | ||
| /// every line) while parsing, passing it a [`LoadProgress`] snapshot. | ||
| /// Returning [`ControlFlow::Break`] from the callback aborts the load and | ||
| /// causes [`load_obj_buf()`] to return [`LoadError::Cancelled`]. | ||
| /// | ||
| /// Not invoked by [`load_obj_buf_async()`]. | ||
| #[cfg_attr(feature = "arbitrary", arbitrary(default))] | ||
| pub progress_callback: Option<LoadProgressCallback>, | ||
| } | ||
|
|
||
| impl LoadOptions { | ||
|
|
@@ -649,6 +718,7 @@ pub enum LoadError { | |
| FaceColorOutOfBounds, | ||
| InvalidLoadOptionConfig, | ||
| GenericFailure, | ||
| Cancelled, | ||
| } | ||
|
|
||
| impl fmt::Display for LoadError { | ||
|
|
@@ -672,6 +742,7 @@ impl fmt::Display for LoadError { | |
| LoadError::FaceColorOutOfBounds => "face vertex color index out of bounds", | ||
| LoadError::InvalidLoadOptionConfig => "mutually exclusive load options", | ||
| LoadError::GenericFailure => "generic failure", | ||
| LoadError::Cancelled => "load cancelled by progress callback", | ||
| }; | ||
|
|
||
| f.write_str(msg) | ||
|
|
@@ -719,8 +790,8 @@ impl VertexIndices { | |
| ) -> Option<VertexIndices> { | ||
| let mut indices = [MISSING_INDEX; 3]; | ||
| for i in face_str.split('/').enumerate() { | ||
| // Catch case of v//vn where we'll find an empty string in one of our splits | ||
| // since there are no texcoords for the mesh. | ||
| // Catch case of v//vn where we'll find an empty string in one of | ||
| // our splits since there are no texcoords for the mesh. | ||
| if !i.1.is_empty() { | ||
| match isize::from_str(i.1) { | ||
| Ok(x) => { | ||
|
|
@@ -899,8 +970,8 @@ fn export_faces( | |
| let mut is_all_triangles = true; | ||
|
|
||
| for f in faces { | ||
| // Optimized paths for Triangles and Quads, Polygon handles the general case of | ||
| // an unknown length triangle fan. | ||
| // Optimized paths for Triangles and Quads, Polygon handles the general | ||
| // case of an unknown length triangle fan. | ||
| match *f { | ||
| Face::Point(ref a) => { | ||
| if !load_options.ignore_points { | ||
|
|
@@ -1135,8 +1206,8 @@ fn export_faces_multi_index( | |
| let mut is_all_triangles = true; | ||
|
|
||
| for f in faces { | ||
| // Optimized paths for Triangles and Quads, Polygon handles the general case of | ||
| // an unknown length triangle fan | ||
| // Optimized paths for Triangles and Quads, Polygon handles the general | ||
| // case of an unknown length triangle fan | ||
| match *f { | ||
| Face::Point(ref a) => { | ||
| if !load_options.ignore_points { | ||
|
|
@@ -1785,7 +1856,8 @@ fn parse_obj_line( | |
| // for them? | ||
| Some("o") | Some("g") => { | ||
| // If we were already parsing an object then a new object name | ||
| // signals the end of the current one, so push it onto our list of objects | ||
| // signals the end of the current one, so push it onto our list of | ||
| // objects | ||
| if !models.faces.is_empty() { | ||
| models.pop_model(load_options)?; | ||
| } | ||
|
|
@@ -1797,7 +1869,8 @@ fn parse_obj_line( | |
| Ok(ParseReturnType::None) | ||
| } | ||
| Some("mtllib") => { | ||
| // File name can include spaces so we cannot rely on a SplitWhitespace iterator | ||
| // File name can include spaces so we cannot rely on a | ||
| // SplitWhitespace iterator | ||
| let mtllib = line.split_once(' ').unwrap_or_default().1.trim(); | ||
| let mat_file = Path::new(mtllib).to_path_buf(); | ||
| Ok(ParseReturnType::LoadMaterial(mat_file)) | ||
|
|
@@ -1807,8 +1880,9 @@ fn parse_obj_line( | |
|
|
||
| if !mat_name.is_empty() { | ||
| let new_mat = materials.mat_map.get(&mat_name).cloned(); | ||
| // As materials are returned per-model, a new material within an object | ||
| // has to emit a new model with the same name but different material | ||
| // As materials are returned per-model, a new material within an | ||
| // object has to emit a new model with the same | ||
| // name but different material | ||
| if models.mat_id != new_mat && !models.faces.is_empty() { | ||
| models.pop_model(load_options)?; | ||
| } | ||
|
|
@@ -2037,24 +2111,63 @@ where | |
| return Err(LoadError::InvalidLoadOptionConfig); | ||
| } | ||
|
|
||
| // How often (in lines) to invoke `load_options.progress_callback`, if set. | ||
| // Kept coarse so the callback's cost stays negligible next to parsing. | ||
| const PROGRESS_REPORT_INTERVAL: u64 = 1000; | ||
|
|
||
| let mut models = TmpModels::new(); | ||
| let mut materials = TmpMaterials::new(); | ||
|
|
||
| let mut lines_read: u64 = 0; | ||
| let mut bytes_read: u64 = 0; | ||
|
|
||
| for line in reader.lines() { | ||
| lines_read += 1; | ||
| // `BufRead::lines()` strips the line terminator, so this | ||
| // undercounts by one byte per line. Good enough for progress | ||
| // reporting. | ||
| bytes_read += line.as_ref().map(|l| l.len() as u64 + 1).unwrap_or(0); | ||
|
|
||
| let parse_return = parse_obj_line(line, load_options, &mut models, &materials)?; | ||
| match parse_return { | ||
| ParseReturnType::LoadMaterial(mat_file) => { | ||
| materials.merge(material_loader(mat_file.as_path())); | ||
| } | ||
| ParseReturnType::None => {} | ||
| } | ||
|
|
||
| if let Some(callback) = &load_options.progress_callback { | ||
| if lines_read.is_multiple_of(PROGRESS_REPORT_INTERVAL) { | ||
| let progress = LoadProgress { | ||
| lines_read, | ||
| bytes_read, | ||
| }; | ||
| if let ControlFlow::Break(()) = callback.call(&progress) { | ||
| return Err(LoadError::Cancelled); | ||
| } | ||
| } | ||
| } | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we always fire the callback on completion?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added an unconditional call after the loop with the true final |
||
| } | ||
|
|
||
| // For the last object in the file we won't encounter another object name to | ||
| // tell us when it's done, so if we're parsing an object push the last one | ||
| // on the list as well | ||
| models.pop_model(load_options)?; | ||
|
|
||
| // One last, unconditional call so a progress UI driven purely off this | ||
| // callback can reach 100% -- `lines_read` only lands on a multiple of | ||
| // `PROGRESS_REPORT_INTERVAL` by chance, so the loop above may never | ||
| // report the true final count. `ControlFlow::Break` here is not | ||
| // honored: the parse has already fully succeeded, so there is nothing | ||
| // left to cancel. | ||
| if let Some(callback) = &load_options.progress_callback { | ||
| let progress = LoadProgress { | ||
| lines_read, | ||
| bytes_read, | ||
| }; | ||
| let _ = callback.call(&progress); | ||
| } | ||
|
|
||
| Ok((models.into_models(), materials.into_materials())) | ||
| } | ||
|
|
||
|
|
@@ -2283,9 +2396,9 @@ pub mod futures { | |
| } | ||
| } | ||
|
|
||
| // For the last object in the file we won't encounter another object name to | ||
| // tell us when it's done, so if we're parsing an object push the last one | ||
| // on the list as well | ||
| // For the last object in the file we won't encounter another object | ||
| // name to tell us when it's done, so if we're parsing an object | ||
| // push the last one on the list as well | ||
| models.pop_model(load_options)?; | ||
|
|
||
| Ok((models.into_models(), materials.into_materials())) | ||
|
|
@@ -2351,8 +2464,8 @@ pub mod tokio { | |
| } | ||
| }; | ||
| load_obj_buf(BufReader::new(file), load_options, |mat_path| { | ||
| // This needs to be "copied" into this closure before moving it into the async | ||
| // one below | ||
| // This needs to be "copied" into this closure before moving it into | ||
| // the async one below | ||
| let file_name: &Path = file_name.as_ref(); | ||
| let file_name = file_name.to_path_buf(); | ||
| async move { | ||
|
|
@@ -2427,9 +2540,9 @@ pub mod tokio { | |
| } | ||
| } | ||
|
|
||
| // For the last object in the file we won't encounter another object name to | ||
| // tell us when it's done, so if we're parsing an object push the last one | ||
| // on the list as well | ||
| // For the last object in the file we won't encounter another object | ||
| // name to tell us when it's done, so if we're parsing an object | ||
| // push the last one on the list as well | ||
| models.pop_model(load_options)?; | ||
|
|
||
| Ok((models.into_models(), materials.into_materials())) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like
is_multiple_ofis a relatively recent Rust function https://doc.rust-lang.org/std/primitive.u64.html#method.is_multiple_of , can we instead just usePROGRESS_REPORT_INTERVAL > 0 && lines_read % PROGRESS_REPORT_INTERVAL == 0? to guard against prog report interval being 0 ? It looks like that's the main difference of not using%directly, theis_multiple_ofdoesn't panic of the rhs is 0Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Swapping to the
%form failed clippy's ownmanual_is_multiple_oflint under-D warnings(the exact check in this PR's own test plan).Since
PROGRESS_REPORT_INTERVALis a fixedconst = 1000it can never actually be 0 either way, and the method has been stable since 1.87 with nothing here pinning an older toolchain, so I keptis_multiple_of.Happy to add
#[allow(clippy::manual_is_multiple_of)]and go back to the explicit guard if you'd still rather have that for other reasons.