Repository navigation
EMSUSD-4025 - part1/2 reader writer for ply file - #4768
adsk-gagnonf wants to merge 5 commits into
Conversation
|
tinyply.h is a third party, you can focus review on gaussiansplatply.h/cpp and cmake files |
a525775 to
469b67b
Compare
469b67b to
6aa7c12
Compare
Add Gaussian splat PLY reader/writer, including cmake/include wiring and a roundtrip test reading from a stream.
6aa7c12 to
9ea82de
Compare
81bef3e to
f94ccc4
Compare
seando-adsk
left a comment
seando-adsk
left a comment
There was a problem hiding this comment.
A few things to look at and fix.
|
@AramAzhari-adsk Can you have a look at |
| case tinyply::Type::UINT16: convertToFloat<uint16_t>(src, dst, n); break; | ||
| case tinyply::Type::INT32: convertToFloat<int32_t>(src, dst, n); break; | ||
| case tinyply::Type::UINT32: convertToFloat<uint32_t>(src, dst, n); break; | ||
| default: *errorMsg = std::string(label) + " has an unsupported type"; return false; |
There was a problem hiding this comment.
You have // \p out is left untouched on failure. yet here we have a situation where you potentially manipulate the out param and can fail.
| return false; | ||
| } | ||
|
|
||
| out->resize(n); |
There was a problem hiding this comment.
this can throw, and with large data that we might be dealing with i'd probably do checks at the callsites
There was a problem hiding this comment.
maybe expand the try/catch to cover the block of code after you file reading part
There was a problem hiding this comment.
moved the catch{} after the type casting block
| std::vector<float> positions; | ||
| // half-axis lenght (s0, s1, s2) activated using exp of PLY log-scale | ||
| std::vector<float> scales; | ||
| // real-first convention (w, x, y, z) |
There was a problem hiding this comment.
(minor) I think it's imaginary first in USD, FYI.
There was a problem hiding this comment.
https://openusd.org/dev/api/class_gf_quaternion.html
Openusd Quaternions have (real, vec3) layout
e86f4da to
225d6a3
Compare
225d6a3 to
4d25aa0
Compare
seando-adsk
left a comment
seando-adsk
left a comment
There was a problem hiding this comment.
Just one minor question about a cmake change (moving utils to top).
1bbfbe5
3 participants
Add Gaussian splat PLY reader/writer, including cmake/include wiring and a roundtrip test reading from a stream.