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

Decode texture coordinates #39

Merged
merged 4 commits into from
Mar 27, 2023

Conversation

denisri
Copy link
Contributor

@denisri denisri commented Mar 3, 2023

After failing to locally merge PR #17 (which seems somewhat abandoned), I took from it just the needed code to decode texture coordinates. This PR is lighter and can be merged on the current master, but doesn't implement the encoding part.

src/DracoPy.pyx Outdated
def tex_coord(self):
tex_coord_ = self.data_struct['tex_coord']
if len(tex_coord_) == 0:
return np.array([], dtype=tex_coord_.dtype)
Copy link
Contributor

Choose a reason for hiding this comment

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

This array will have the wrong shape. It needs to be reshaped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting point (well, two points actually).

  • Which shape is actually expected for texture corrdinates ?
    Using it this way, for a mesh with a texure with 2 coordinates per vertex, I actually get an array of shape (NV, 2) (where NV is the number of vertices). This is consistent with points or normals arrays which are (NV, 3) for instance. But here I get "flipped" coordinates (both coordinates are switched) compared to the initial non-draco-encoded mesh, but as I don't know Draco at all I am not sure whether it is expected or not, and why. Is it what you were pointing out ? And thus it is not really a "reshape", but I need to do: tex_corrd[:, -1::-1] to get back the expected coordinates. Should I include this operation in the tex_coord property here (and is there a reason for this ?) ?
  • in the situation where there are no texture coords, here I return an empty array of shape (0, ). Maybe it's not the best and I shoudl return None instead, or an array of shape (0, 0) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to return the tex coordinates in the same order as the original model.

For the second point, I think it makes sense to return None if the model has no texture attribute, but if it is labeled as having one, then (0,0) would be preferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK it now returns None when there are no texture coords. And testing again I don't find the coordinates inversions I had when testing at first. So I must have done something wrong, but it seems to work as expected now (otherwise there is something I don't understand)

@denisri
Copy link
Contributor Author

denisri commented Mar 21, 2023

I am coming back again on this PR. Texcoords seem to work correctly for me now using this branch. Or maybe I don't understand the texcoord array shape issue. Are there more things to change on it ?

@william-silversmith
Copy link
Contributor

Hi! I'm sorry, I've just been overwhelmed with various projects and the tests are broken on this repo. I need to fix them before I can release a new version. I will try to do this soon!

@william-silversmith
Copy link
Contributor

william-silversmith commented Mar 26, 2023

Hi! I think I've fixed the build process. Please just fix the merge conflict on DracoPy.cpp and we can get moving!

@denisri
Copy link
Contributor Author

denisri commented Mar 27, 2023

I have merged from master and re-generated DracoPy.cpp using cython.

@william-silversmith william-silversmith merged commit 3ed3da8 into seung-lab:master Mar 27, 2023
@william-silversmith
Copy link
Contributor

Thank you for your patience @denisri!

@denisri denisri deleted the decode_texture branch March 27, 2023 15:34
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.

2 participants