fixed several isses in vdb_tool by kmuseth · Pull Request #2349 · AcademySoftwareFoundation/openvdb

kmuseth · 2026-09-18T19:16:37Z

Fixed some corner cases in vdb_tool, some uncovered by Astra in extreme mode, and others resulting from tests on the Thingi10K data set:

Preserve rebuilt CSG results when replacing input grids.
Return a failing exit status after action errors and stop immediately on
control-flow errors instead of entering invalid scopes.
Preserve input grids after failed VDB/NanoVDB writes and avoid changing
the grid stack or names when voxel-kernel variable validation fails.
Preserve modulo expressions and loop counters in configuration files.
Inline comments now use '#'; '%' still supports full-line comments.
Reject truncated or malformed PLY data instead of hanging or reading
incomplete records.
Support descending loops and reject zero or non-progressing steps.
Keep binary stdout separate from diagnostic logs in both logging modes.

Signed-off-by: Ken Museth <ken.museth@gmail.com>

swahtz

A few comments that would clean up or address some small gotchyas. I think the one issue that's definitely blocking is the change in round-trip half grid behaviour in writeVDB by calling setSaveFloatAsHalf for every grid.

swahtz · 2026-10-01T21:41:56Z

+    GridPtrVec output;
+    for (const auto &grid : grids) {
+      output.push_back(grid->copyGrid());
+      output.back()->setSaveFloatAsHalf(half);


🚩 issue: ‏Because setSaveFloatAsHalf is called for every grid, this changes behaviour for round-tripping half grids. -read half.vdb -write out.vdb used to round-trip as 16-bit. With this PR, because bits=32 is the default, it silently writes 32-bit instead of preserving the half grid.

swahtz · 2026-10-01T21:46:24Z


    } catch (const std::exception& e) {

        std::cerr << "Fatal error in " << argv[0] << ":\n\t" << e.what() << std::endl;


💭 thought (non-blocking): ‏ I don't know if it is intended behavior or not, but fatal errors like this are no longer going to reach the -log file. The exception unwinds and destroys Tool, whose destructor calls endLog(), before main prints the message. With tee=false, the log has no record of why the run failed.

swahtz · 2026-10-01T21:48:25Z

+        if (sum < std::numeric_limits<T>::lowest() || sum > std::numeric_limits<T>::max())
+            throw std::invalid_argument("ForLoop: loop value out of range");
+        const T next = static_cast<T>(sum);
+        if (vec[2] > T(0) ? next <= vec[0] : next >= vec[0])


🎵 note (non-blocking): ‏The "step does not advance" check compares against vec[0] instead of current. A loop body that moves i backward to retry an iteration now throws, where before it simply repeated.

swahtz · 2026-10-01T21:50:43Z

-                line = line.substr(0, line.find_last_not_of(" \t") + 1);// remove tailing whitespaces
+                const size_t start = line.find_first_not_of(" \t\r");
+                if (start == std::string::npos || line[start] == '#' || line[start] == '%') continue;
+                line = trim(stripConfigComment(accum + line), " \t\r");


❓ question: ‏Config lines no longer strip inline % comments, but the config version wasn't bumped. isCompatible() only checks the major version, so old configs silently parse differently. Do we want to bump the config version or do we consider this a bug with the old behaviour?

swahtz · 2026-10-01T21:53:15Z

🚩 issue: ‏-sdf2udf will catch its own exception, log and exit status 0 on failure which contradicts the promise of a nonzero exit after action errors.

swahtz · 2026-10-01T21:58:45Z

🚩 issue: ‏The PR switches stdin to binary mode (main.cpp:31-33), so on Windows a CRLF blank line in an OFF body arrives as "\r" instead of an empty line. That line gets parsed as data, which pushes a stale or uninitialized vertex or face.

swahtz · 2026-10-02T00:27:53Z

                    if (xyz[i].size == 4) {
-                        float v = *(float*)(p + xyz[i].offset);
+                        float v;
+                        std::memcpy(&v, buffer.data() + xyz[i].offset, sizeof(v));


🚩 issue: ‏This is a pre-existing bug but might as well fix it if we are already changing this function. With a binary PLY, readPLY copies the raw bytes of int/int32 vertex properties into a float, so integer coordinates load as garbage values like 1.4e-45. The ASCII PLY path is correct.

swahtz · 2026-10-02T00:30:40Z

            return grids[n.gridIdx].grid.get() == outputGridPtr.get() &&
                   (n.dx || n.dy || n.dz);
        });
    GridT::Ptr out_snap = needSnapshot ? grid->deepCopy() : nullptr;


🤓 nitpick (non-blocking): ‏With keep=true, forValues deep-copies the grid twice, once for the output and once for the neighbor snapshot. That doubles peak memory. Neighbor reads could use the untouched original instead.

Signed-off-by: Ken Museth <ken.museth@gmail.com>

kmuseth · 2026-10-16T18:29:05Z

Thanks for the thorough review. All the comments were valid, and the latest push addresses each one:

Half grids written back as 32-bit (writeVDB)
Fixed. The bits option of -write now defaults to empty, which keeps each grid's own half-float setting (OpenVDB restores it when it reads a file). So -read half.vdb -write out.vdb round-trips as 16-bit again. bits=16 or bits=32 still force the precision, and NanoVDB output still defaults to 32-bit. New test: WriteVdbPreservesHalf.

Fatal errors missing from the -log file
Good catch, that was a regression from moving error handling out of Tool::run(). Tool::run() now writes the error directly to the log file before rethrowing, so the log records why the run failed in both tee=true and tee=false mode. main() still prints it once to the terminal. New test: CliLogFatalError.

-for progress check compares against the start value instead of the current one
Agreed. The check now compares the next value with the loop variable's current value. A loop body can move the variable backward to retry an iteration, while a float step that no longer advances (e.g. i=16777216,16777220,0.5) is still rejected. Added a retry case to LoopProgress.

Inline % comments in config files without a version bump
I consider the old behavior a bug. % is valid syntax outside comments, e.g. modulo in calc n = 5 % 2 and name substitution in name=sphere_%v, and the old parser silently truncated those lines. To make the change visible, I bumped the config version from 10.8.0 to 10.9.0; 10.x configs still load. I also updated the README examples and the pending-changes note. Full-line % comments are still supported, and inline comments now use #.

-sdf2udf exits with status 0 on failure
Fixed. I removed its local try/catch, so errors go through the common handler in Parser::run and the tool exits with a nonzero status. The level-set check now runs before the grid is copied or renamed, so a failure leaves the grid stack unchanged. Writing the test also turned up an older bug: the name option defaulted to "sphere", so every converted grid was renamed sphere. It now defaults to sdf2udf_. New test: Sdf2UdfFailure.

CRLF blank lines in OFF input on binary stdin
Fixed. The OFF reader now skips lines that are empty or contain only whitespace or a CR, as well as indented # comments. Malformed or truncated vertex and face data now throws instead of adding old or uninitialized values. The XYZ reader had the same CR problem and is fixed too. New test: OffBlankLinesAndTruncation.

Binary PLY with integer vertex coordinates
Fixed. It also affected the fast path for x, y, z packed as three consecutive 4-byte values, which read int32 bits as floats as well. Each coordinate now records whether it's an int32, int32 values are converted to float, and the fast path only applies when all three are floats. Added cases to MalformedPly, with and without extra vertex properties.

Double deep copy in -forValues with keep=true
Fixed. The check for whether a snapshot is needed now compares against the actual output grid. With keep=true the output is a private copy, so no snapshot is taken and neighbor values are read from the unchanged input grid.

All vdb_tool unit tests pass locally.

swahtz

Looks good, thanks for addressing my notes.

fixed several isses in vdb_tool

b67500b

Signed-off-by: Ken Museth <ken.museth@gmail.com>

kmuseth requested review from Idclip, apradhana, danrbailey, jmlait and richhones as code owners September 18, 2026 19:16

kmuseth requested a review from swahtz September 18, 2026 19:17

kmuseth added 6 commits September 18, 2026 09:27

snapshot

cdfb0ad

Signed-off-by: Ken Museth <ken.museth@gmail.com>

resolved an issue in readSTL

344f1dc

Signed-off-by: Ken Museth <ken.museth@gmail.com>

now vdb_tool can read some bit corrupted files

4a7e689

Signed-off-by: Ken Museth <ken.museth@gmail.com>

fixed warning

0388617

Signed-off-by: Ken Museth <ken.museth@gmail.com>

fixed broken unit-test

bc5a67d

Signed-off-by: Ken Museth <ken.museth@gmail.com>

resolved a run-time issue on Windows realted to I/O

05a6121

Signed-off-by: Ken Museth <ken.museth@gmail.com>

swahtz requested changes Oct 2, 2026

View reviewed changes

addressed review comments

aa33e6c

Signed-off-by: Ken Museth <ken.museth@gmail.com>

swahtz approved these changes Oct 17, 2026

View reviewed changes

kmuseth merged commit c5ad9b7 into AcademySoftwareFoundation:master Oct 17, 2026
45 checks passed

kmuseth deleted the fix_vdb_tool branch October 17, 2026 05:59

Navigation Menu

Repository navigation

fixed several isses in vdb_tool - #2349

fixed several isses in vdb_tool#2349
kmuseth merged 8 commits into
AcademySoftwareFoundation:masterfrom
kmuseth:fix_vdb_tool

kmuseth commented Sep 18, 2026 •

edited

Loading

Uh oh!

swahtz left a comment

Uh oh!

swahtz Oct 1, 2026

Uh oh!

swahtz Oct 1, 2026

Uh oh!

swahtz Oct 1, 2026

Uh oh!

swahtz Oct 1, 2026

Uh oh!

swahtz Oct 1, 2026

Uh oh!

swahtz Oct 1, 2026

Uh oh!

swahtz Oct 2, 2026

Uh oh!

swahtz Oct 2, 2026

Uh oh!

kmuseth commented Oct 16, 2026

Uh oh!

swahtz left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		} catch (const std::exception& e) {

		std::cerr << "Fatal error in " << argv[0] << ":\n\t" << e.what() << std::endl;

Conversation

kmuseth commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swahtz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmuseth commented Oct 16, 2026

Uh oh!

swahtz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kmuseth commented Sep 18, 2026 •

edited

Loading