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

v0.0.18 | Supports Texture Coordinates and Normals #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

farunurisonmez
Copy link

#5 Can it encode normal or uv?

  • Decode supports Texture Coordinates and Normals
  • Encode supports Texture Coordinates and Normals

Co-Authored-By: Faru Nuri Sönmez info@farunurisonmez.com

...

Co-Authored-By: Faru Nuri Sönmez <info@farunurisonmez.com>
@william-silversmith
Copy link
Contributor

Hi Faru!

I'm the new maintainer for this repo and I'll be happy to review your PR! I'm a bit less experienced with the Draco spec compared with the library creator, so I might need some tips to help review. ^_^

I'll leave some comments on the PR. Would you mind also adding an automated test?

Thanks!! Sorry it took so long to get this reviewed.

encoding_test = DracoPy.encode_mesh_to_buffer(mesh_object.points, mesh_object.faces)
print('number of faces in original file: {0}'.format(len(mesh_object.tex_coord)))
print('number of normal in original file: {0}'.format(len(mesh_object.normals)))
encoding_test = DracoPy.encode_mesh_to_buffer(mesh_object.points, mesh_object.face)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

faces?

print('number of points in original file: {0}'.format(len(mesh_object.points)))
print('number of faces in original file: {0}'.format(len(mesh_object.faces)))
encoding_test = DracoPy.encode_mesh_to_buffer(mesh_object.points, mesh_object.faces)
print('number of faces in original file: {0}'.format(len(mesh_object.tex_coord)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be the number of texture coordinates?

print('number of faces in original file: {0}'.format(len(mesh_object.faces)))
encoding_test = DracoPy.encode_mesh_to_buffer(mesh_object.points, mesh_object.faces)
print('number of faces in original file: {0}'.format(len(mesh_object.tex_coord)))
print('number of normal in original file: {0}'.format(len(mesh_object.normals)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

normal -> normals

@@ -69,7 +68,7 @@ def read(fname):
setuptools.Extension(
'DracoPy',
sources=[ os.path.join(src_dir, 'DracoPy.cpp') ],
depends=[ os.path.join(src_dir, 'DracoPy.h') ],
depends=[ os.path.join(src_dir, 'DracoPy.h'), os.path.join(src_dir, 'DracoPy.pyx')],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is right. The pyx is only necessary for generating the DracoPy.cpp files

@@ -35,11 +35,18 @@ namespace DracoFunctions {
};

MeshObject decode_buffer(const char *buffer, std::size_t buffer_len) {

std::cout << "Decode Buffer" << "Begin" << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please either hide the std::couts behind a verbose flag or remove them. This library is often used in large deployments of thousands of CPUs.

@@ -70,7 +74,7 @@ class FileTypeException(Exception):
class EncodingFailedException(Exception):
pass

def encode_mesh_to_buffer(points, faces, quantization_bits=14, compression_level=1, quantization_range=-1, quantization_origin=None, create_metadata=False):
def encode_mesh_to_buffer(points, faces, normals, quantization_bits=14, compression_level=1, quantization_range=-1, quantization_origin=None, create_metadata=False):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a breaking change. Can normals be given a default value that is handled gracefully?

@william-silversmith
Copy link
Contributor

Thanks again for this valuable contribution!

@william-silversmith
Copy link
Contributor

Hi Faru! I adopted some of your changes (the normal decode) and gave you a credit in the setup.py file! I was working rapidly so I didn't adopt the textures as I wasn't sure how to test it. Thanks for your contribution!

@william-silversmith
Copy link
Contributor

Sorry, closed by mistake when I meant to comment.

@cansik
Copy link

cansik commented Apr 9, 2022

@william-silversmith Thank you very much for further upgrading the library, would it be possible to add UV / texture support as well? If you need an example script / files I could provide it to you for testing.

@william-silversmith
Copy link
Contributor

william-silversmith commented Apr 9, 2022 via email

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

Successfully merging this pull request may close these issues.

Encoding does not return resulting faces and points count
3 participants