Enable generic attributes for point cloud encode/decode #61
Conversation
|
Hi @kbongort, to take some of the burden off of the (impressively busy, according to his GH profile) maintainer @william-silversmith would you be willing to give this PR a quick review? No worries if not (and thanks in any case for your previous work!). |
| uint32_t next_unique_id = 0; | ||
| auto max_unique_id = std::max_element(unique_ids.begin(), unique_ids.end()); | ||
| if (max_unique_id != unique_ids.end()) { | ||
| next_unique_id = *max_unique_id + 1; |
There was a problem hiding this comment.
It's theoretically possible for max_unique_id to be an arbitrary negative integer, but unique IDs must be non-negative. I'd recommend also checking in the condition that *max_unique_id >= 0.
There was a problem hiding this comment.
I think that notionally the least *max_unique_id can be is -1; user specified unique_ids are checked to be nonnegative and -1 is used as a sentinel value for non-user-specified (I assume this is why the type of unique_ids was originally chosen in these wrappers to be int8_t instead of the upstream uint32_t).
Code deep dives aside, the upshot is that *max_unique_id == -1 is actually an expected case, and I've added a check (in the C++ wrapper code) that min(unique_ids) >= -1.
|
Hi @schmrlng , I don't have the ability to review PRs in this repo, but I think yours looks good, with the one minor fix I requested. |
william-silversmith
added
bug
src/DracoPy.h
Outdated
| draco::DataType dtype = static_cast<draco::DataType>(attr_data_types[j]); | ||
| int num_components = attr_num_components[j]; | ||
| if (dtype != draco::DT_FLOAT32 && dtype != draco::DT_UINT8 && dtype != draco::DT_UINT16 && dtype != draco::DT_UINT32) { | ||
| // Unsupported data type, skip |
There was a problem hiding this comment.
Should this silently fail? Maybe it should be a flag ignore_unsupported_attributes? Otherwise there could be silent data loss.
There was a problem hiding this comment.
Sure, this was just keeping the existing behavior but I agree it shouldn't silently fail. I feel like the user should probably just fix their invocation on the python side rather than adding this flag to force things through, so I've added a runtime_error (but could go with the flag if you feel strongly about it).
src/DracoPy.h
Outdated
| } | ||
| int att_id = pcb.AddAttribute(draco::GeometryAttribute::GENERIC, num_components, dtype); | ||
| if (att_id == -1) { | ||
| // Failed to add attribute, skip |
There was a problem hiding this comment.
same here, i feel like skipping should be opt in? what do you think
There was a problem hiding this comment.
As noted above, my feeling is that skipping probably just shouldn't be allowed to happen.
|
Thanks for these updates! |
|
Thanks for taking a look! I've tried to address @kbongort's comment in 19689ba and removed the possibility for silent failures in favor of failing loudly in b08896f. The latter is technically a breaking change (not sure who out there would notice/care) but the commit can be independently rolled back if it turns out to be an issue. |
3 participants
Building upon #59 and #60, this PR contains two commits:
unique_ids can be duplicated between native attributes (i.e., of typePOSITION,NORMAL,COLOR,TEX_COORD) andGENERICattributes, which may interfere withDracoPointCloud.get_attribute_by_unique_id.Thanks for creating this wrapper library!