-
Notifications
You must be signed in to change notification settings - Fork 26
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
Support metadata parsing #20
base: master
Are you sure you want to change the base?
Conversation
Hi Romanov! Thanks for this excellent contribution! I'm not as familiar with the draco standard as the last maintainer, so I'm learning as I go. Would you mind adding some examples and tests to help me review this? Excited to get this integrated! One question I have: does this change affect the bitstream if no metadata is provided? We use DracoPy with Neuroglancer which has its own custom Draco decoder, so I want to be careful about any backwards incompatible byte streams. I'll add some more comments per line as I see them. |
Hi William! Thank you for the response! I suppose this change will not affect existed functionality, but yes I have to provide some tests to make sure that it works. Thank you again for the great library! |
src/DracoPy.h
Outdated
@@ -8,6 +11,7 @@ | |||
#include "draco/point_cloud/point_cloud_builder.h" | |||
|
|||
namespace DracoFunctions { | |||
using namespace draco; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer not to use this to make it more obvious where each function is coming from unless it becomes excessively cumbersome.
Okay, I'll wait for your updated submission then. Thanks for the effort you're putting into this! |
Sorry for the long delay. I was deeping in draco to know more about geometry serialization features. Currently I added support not only support metadata with entires but geometry attributes (that stores additional data for points) too. Unfortunately one test for point cloud doesn't work as data for geometry attributes is not serialized. I didn't investigate this issue because I don't need in point cloud interfaces. Currently the test is disabled. It would be nice if you reviewed my changes. |
Hi Romanov, Thanks for pushing this contribution forward! I've started to review the PR. I've had some trouble compiling it on MacOS (a successful PR needs to compile on Mac, Windows, and Ubuntu). I can help you with checking that. The first problem is:
I had some issues fixing this with C++ casting, The second problem is the use of C++17 which indirectly generates compile errors. Switch the c++11 compiler flag to c++17 in setup.py and I'm able to successfully compile.
|
This is kind of really basic, so basic it's kind of dumb, but can you explain a bit more about what kind of uses metadata has per object? Would it be like noting that a given model is a building or a person? Is the idea to allow the storage of a JSON-like data structure inside the draco bitstream? I see there's global vs geometric metadata. Can there be more than one model in the bitstream? I didn't see anything about that skimming the spec, but I could have missed it. If so, is it possible to simplify the interface? It looks like in the tests that the user has to know the exact form of the internal draco data structures. Would it be possible to accept a simple list or dictionary of key-values? Thanks so much for this excellent contribution! |
Hope all c++11 issues fixed, please let me know if some of them I missed. |
Hope I can explain my view. AbstractSo far I understand correctly in Draco we have one global GeometryMetadata for each mesh or point cloud. The Metadata contains "entries" (simple key-value storage) and "submetadatas" (list of Metadata). The GeometryAttribute contains its own Metadata and additional data for any point of point cloud or mesh (let me call it "MappedData"). Actually we can store any labeling of mesh per point in "MappedData", i.e. color for each point. So we have the following structure:
More detailes can be found in sources of Draco. Tree structure of MetadataSo far Metadata has a tree structure I couldn't forward it from c++ to python directly. Tree requires recursive definition that is not supported in Cython, c++ and python simultineously (Cython couldn't compile that code). So I tried to solve it with different approaches. JSON like structureI don't think I understand your questions about JSON like structure. Maybe you surprised that I used a lot of dicts in tests, don't you? Please provide more details to help me to reply=) Interface simplificationYes, I agree that current implementation is not so easy to understand. Finally, sorry for my english, I hope =) |
Thank you for the detailed reply! It's very helpful. I have an even more basic question as someone that isn't a 3D modeler: What are some typical uses of the metadata and geometry attribute fields? My lab only uses the topological compression to represent meshes of neurons. We haven't really branched out and looked at the other Draco abilities. |
You may image that you have some mesh, in example human jaw with teeth and gingiva. And you want to save this mesh with Draco but don't want to loose the information which point is a tooth and which one is a gingiva. So you can use GeometryAttribute in GeometryMetadata that help you discover is it a gingiva or tooth point. {
# label can be '0' (gingiva) or '1' (tooth)
# <point index>: <label>
0: 0,
1: 0,
2: 0,
3: 1,
....
987: 0
} Also one more technical case. |
Hi William, I have add single interface for point cloud and mesh encoding. Only one thing I would check additionally. It seems like I have to test if encoding options are applied correctly. |
src/DracoPy.pyx
Outdated
quantization_bits, compression_level, | ||
quantization_range, quant_origin, create_metadata, | ||
) | ||
if isinstance(faces, list): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Often we put numpy arrays into this function. Perhaps a check for iterable is better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, thank you for finding
Sorry for the wait, I'll start running these changes through their paces. :D |
I kind of doubt there were tests before... For my purposes, we're working with https://github.com/google/neuroglancer which has its own stripped down Draco encoder compiled to Web Assembly so it can decode draco meshes within a web browser. That decoder expects certain limited options. For example, the number of vertex quantization bits can only be 10 or 16. This is too much information, but you can view it here: https://github.com/google/neuroglancer/blob/056a3548abffc3c76c93c7a906f1603ce02b5fa3/src/neuroglancer/datasource/precomputed/meshes.md#multi-resolution-mesh-format You can also see me try to use these options here: https://github.com/seung-lab/igneous/pull/112/files#diff-15fd2ba7077a3ef6422a3b10c4a3c1c662aa83ff0450ba7d2c8e2f51fbff1e72R105-R112 I'm not sure if I helped answer your question though. Let me know if I can be of more assistance! |
Looks like it is not so easy to test my changes for
Thank you for the efforts and your time! |
The latest version compiles on MacOS x86_64 👍 I also visually checked that it reproduces meshes from the fly hemibrain. I'll have to check compression too. |
Hi William, Is it possible to activate your repo workflows for my PR? I want to use Linux wheel, but it seems github workflow was not cloned in my account. |
And one more question. |
Sorry for the wait! I have to switch laptops to evaluate the code b/c compiling on my M1 Macbook is too difficult. Finally doing the last bit of validation. I'll highlight some issues I'm finding along the way.
This code results in a segfault for me. import os.path
import DracoPy
metadata = {
"entries": {b"name": b"meta1"},
"sub_metadata_ids": {b"custom_metadata_name": 1},
}
with open(os.path.join("testdata_files", "bunny.drc"), "rb") as draco_file:
file_content = draco_file.read()
mesh_object = DracoPy.decode_buffer_to_mesh(file_content)
binary = DracoPy.encode_to_buffer(
mesh_object.points, mesh_object.faces,
metadatas=[ metadata ],
# geometry_metadata=geometry_metadata,
)
print(binary) |
(each triple means one face). If faces is None then point cloud | ||
will be encoded, otherwise mesh will be encoded | ||
:param List[dict] metadatas: list of metadatas each of them containing | ||
"entries" - dictionary with strings (entry name) and binary data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What key is needed to pass the binary data? It would be good to show an example of a dict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I can add an example. But usually if I need in some examples I try to find tests and understand them=)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I try (but don't always succeed) to make it so that in theory someone can figure out how to use the module from embedded documentation. Tests do help, but sometimes its easier for the documentation to be explicit. You have to wonder if examples are comprehensive or realistic when learning from them. In this case, I didn't see a test with the binary data inside the metadata (or perhaps this is meant to mean the geometry metadata?).
:param List[dict] metadatas: list of metadatas each of them containing | ||
"entries" - dictionary with strings (entry name) and binary data | ||
related to that entry | ||
"sub_metadata_ids" - dictionary with strings (submetadata name) and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise here, an example would help a lot!
I tried:
Output:
|
Thank you for the response, I will check it tomorrow and try to fix comments. |
Sorry I didn't catch the example right away.
Looks like I have to add additional comments to this function |
for _ in range(2) | ||
] | ||
attribute = { | ||
"data": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
geometry data is per a vertex right? If so, a dict is kind of an odd structure to use. Ideally it would be a one-dimensional numpy array (not supported in DracoPy yet) but a list seems okay too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
geometry data is per a vertex right?
Yes, it is.
If so, a dict is kind of an odd structure to use. Ideally it would be a one-dimensional numpy array (not supported in DracoPy yet) but a list seems okay too.
I see one rare use case when map is preferred. If you save data only for some vertices (not all), i.e. for 5 vertices from 10'000'000. In this case length map will be 5, but array will have length 10'000'000.
But it seems not so often case.
I will fix it
I think I still don't understand the point of metadata. Is it only to provide a name for the geometry metadata? Could it also be used to tag objects with a name (which wouldn't be per vertex)? |
I'm sorry I didn't catch what you meant under "to tag objects with a name" Let me remind metadata structure example that I sent before
Here you can see root object called As any |
So I watched this video and I think I'm starting to get a better idea of the context in which this system is supposed to work. Sorry this is taking me so long. The Draco structure is very generic and I am only slowly understanding what the point of each part is. I tried compiling the
I think improved my little example test, but I'm still getting a segfault. Could you help me modify it to make sense? Ideally, the code not segfault even if the data is screwed up though. import os.path
import DracoPy
metadata = [
{
"entries": {
b"name": b"blueness",
b"whodat": b"1234",
},
"sub_metadata_ids": {},
},
{
"entries": {
b"name": b"redness",
b"somethingelse": b"1234",
},
"sub_metadata_ids": {},
},
]
geo = {
"metadata_id": 0,
"generic_attributes": [{
"data": { i: int.to_bytes(i, byteorder="little", length=2, signed=True) for i in range(4) },
"datatype": DracoPy.DataType.DT_INT32,
"dimension": 1,
"metadata_id": 1,
}],
}
with open(os.path.join("testdata_files", "bunny.drc"), "rb") as draco_file:
file_content = draco_file.read()
mesh_object = DracoPy.decode_buffer_to_mesh(file_content)
binary = DracoPy.encode_to_buffer(
mesh_object.points, mesh_object.faces,
metadatas=metadata,
geometry_metadata=geo,
)
print(binary) Thanks again for your patience! |
Which python and cython version did you use? My environment: > python --version
Python 3.8.2
> python -m cython --version
Cython version 0.29.24 I compile cython files using the following command and I get no errors: > python -m cython --cplus -v -I "E:\project\DracoPy\_skbuild\win-amd64-3.8\cmake-install\include\draco" E:\project\DracoPy\src\DracoPy.pyx
Compiling E:\project\DracoPy\src\DracoPy.pyx
E:\project\DracoPy\venv\Lib\site-packages\Cython\Compiler\Main.py:369: FutureWarning: Cython directive 'language_level' not set, using 2 for now (Py2). This will change in a later release! File: E:\project\DracoPy\src\DracoPy.pxd
tree = Parsing.p_module(s, pxd, full_module_name) |
This is for Python 3.8.6 and cython 0.29.24 on MacOS.
I ran this command:
cython -3 --cplus -v -I
"_skbuild/macosx-10.15-x86_64-3.8/cmake-install/include/draco/"
src/DracoPy.pyx
…On Thu, Dec 2, 2021 at 2:33 AM Andrey ***@***.***> wrote:
I tried compiling the pyx with cython -3 --cplus src/DracoPy.pyx and got
the following errors. I think they can be fixed by adding a b in front.
Which python and cython version did you use?
My environment:
> python --version
Python 3.8.2> python -m cython --version
Cython version 0.29.24
I compile cython files using the following command and I get no errors:
> python -m cython --cplus -v -I "E:\project\DracoPy\_skbuild\win-amd64-3.8\cmake-install\include\draco" E:\project\DracoPy\src\DracoPy.pyx
Compiling E:\project\DracoPy\src\DracoPy.pyx
E:\project\DracoPy\venv\Lib\site-packages\Cython\Compiler\Main.py:369: FutureWarning: Cython directive 'language_level' not set, using 2 for now (Py2). This will change in a later release! File: E:\project\DracoPy\src\DracoPy.pxd
tree = Parsing.p_module(s, pxd, full_module_name)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#20 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AATGQSKPX7HU3LE3V2CF4J3UO4OMNANCNFSM5GMSL4LQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Could you try to add prefix |
Oh yea, it fixed it! Sorry that I wasn't more clear.
…On Thu, Dec 2, 2021 at 2:50 AM Andrey ***@***.***> wrote:
Could you try to add prefix b to pyx strings please?
Does it help?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#20 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AATGQSI7O6TCHV2U5IS7CP3UO4QOBANCNFSM5GMSL4LQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
I will try your example to check and fix segfault, thanks! William, is it possible to run your repo workflow for my PR? |
It looks like the build for Windows succeeded on AppVeyor, but I'm not sure how to run the build for Ubuntu and MacOS an unmerged public fork. Some of the issues I've been finding have been from manual testing that finds edge cases the tests haven't found, so it'll take a bit more work on my part regardless. |
Hi Romanov, Sorry to cause conflicts with this PR. I ran into an issue in my own work and ended up making some changes to radically simplify the interface and add a few needed features. If you'd like, I can resolve the conflicts I created for you, but I think you'll have to make a PR to merge onto a development branch so I can update it for you. The big things that changed:
Thank you for bearing with me! |
0c52fd3
to
27f89cc
Compare
No description provided.