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

Possible incorrect variable usage #8

Open
victorubieto opened this issue May 28, 2024 · 3 comments
Open

Possible incorrect variable usage #8

victorubieto opened this issue May 28, 2024 · 3 comments

Comments

@victorubieto
Copy link

Hi,

I'm trying to implement a VDB parser for a personal engine without using the OpenVDB library because it is quite large and has many dependencies. I found your repository, which is easier to understand than the original, and I must say that I am very impressed with your work.

While looking at the BufferIteration class, I think I found a bug in your code. Specifically, when you call precision.size, I believe you meant precisionLUT.size. I have debugged it, and precision.size returns undefined, but it does not break the execution. I don't know if I am right or not, but I thought it was a good idea to let you know.

const mantissa = '1' + binary.slice(precisionLUT.exp + 1, precision.size);

Cheers,

@victorubieto
Copy link
Author

Hi again,

I saw that replacing by precisionLUT.size is not the solution since some times precisionLUT.exp + 1 is greater than precisionLUT.size. Then, I don't know what is the purpose of adding precision.size. I guess it can just be removed, since when it is undefined, the slice() function gets the string until the end.

@mjurczyk
Copy link
Owner

mjurczyk commented May 29, 2024

Hallo hallo - yup, should 100% be precisionLUT, but in this case it could have coincidentally be irrelevant? Iirc reading values from the VDB file was handled correctly for both HalfFloat and Floats (I tested most of the PR and porting by running NanoVDB info in parallel on the same .vdb file - as long as the amount of grids and values matched, I didn't dive deeper into whether the code was actually correct or not 🙈)

Feel free to make a PR on that - I did notice very compressed VDBs currently no longer load, but sample VDBs from Embergen do load nicely.

At some point I'll get back to this lib and push a refreshed API - but no changes in the core VDB loader are currently in the works, so if there's something in need of fix - feel free to push a PR 🙌

(If you'd like to brainstorm parts of the API or some specific issues, feel free to msg on discord, @mjurczyk, always happy to code some clouds.)

@victorubieto
Copy link
Author

Hi,

Thanks for the answer! Yes, from the examples I got the value type HalfFloats and Floats were enough. I'm doing my implementation in C++, which is funny because in theory I should prefer using the original OpenVDB library as reference, but recently I've been coding more often in JS, so I understand your repo better.

I'll write down if I find any improvement or extend any unsupported versions, and I'll try to do a PR at some point (cannot promise though).

(I love clouds, so it would be nice to chat about them in discord 😶‍🌫️)

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

No branches or pull requests

2 participants