Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

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

📖 Add VdbReader struct with read_grid() functionality to allow users to specify which grid to load #41

Merged
merged 6 commits into from
Jul 11, 2023

Conversation

EmilioLaiso
Copy link
Contributor

@EmilioLaiso EmilioLaiso commented Jul 11, 2023

This PR removes the read_vdb function in favor of the new approach of having the user create a VdbReader class, which takes ownership over a reader and parses the ArchiveHeader and the vector of GridDescriptors.
The VdbReader then exposes a read_grid function on which the user can specify the type of the grid data and the name of the grid being requested.

@EmilioLaiso EmilioLaiso changed the title 📖 Add VdbReader struct with read_grid() functionality to allow users to specify which grid to load from the available ones 📖 Add VdbReader struct with read_grid() functionality to allow users to specify which grid to load Jul 11, 2023
…ve header, while exposing a `read_grid(name)` API to load specific grids
@EmilioLaiso EmilioLaiso force-pushed the separate-vdb-reader-from-grid-parsing branch from 66ce223 to afa0a29 Compare July 11, 2023 09:27

let grid = read_vdb::<_, half::f16>(&mut reader).unwrap();
let mut vdb_reader = VdbReader::new(BufReader::new(f)).unwrap();
let grid = vdb_reader.read_grid::<half::f16>("density").unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

👍

@MarijnS95
Copy link
Member

Sadly the reader.rs file is quite hard to review from the diff due to having moved all the free functions under the VdbReader implementation

https://github.com/Traverse-Research/vdb-rs/pull/41/files?diff=unified&w=1

(Which enables "ignore whitespace") so that all reindented code isn't counted as "changed" :)

src/reader.rs Outdated Show resolved Hide resolved
src/reader.rs Outdated Show resolved Hide resolved
Copy link
Member

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

Looks simple on the surface, not sure if we should more cautiously document that read_grid loosely uses name (how about just using ==, and adding an accessor function that returns the names of all the grids available?).

Feels like some helper functions don't need to become associated functions, even if it simplifies some repetitive generics and trait bounds.

src/reader.rs Show resolved Hide resolved
src/reader.rs Outdated Show resolved Hide resolved
src/reader.rs Outdated Show resolved Hide resolved
@EmilioLaiso EmilioLaiso merged commit 4fd6279 into main Jul 11, 2023
4 checks passed
@EmilioLaiso EmilioLaiso deleted the separate-vdb-reader-from-grid-parsing branch July 11, 2023 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants