-
Notifications
You must be signed in to change notification settings - Fork 29
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
+2,854
−2,542
Merged
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
51819d1
decode texture coordinates
denisri 443c0d7
fixed status when texcoords cannot be decoded
denisri 257ecac
tex_coord returns None when there are no texture coordinates
denisri e2a0da8
Merge remote-tracking branch 'origin/master' into decode_texture
denisri File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next
Next commit
decode texture coordinates
inspired by PR#17 which has diverged
commit 51819d12d868d7322da9c632cd6d5777558bd734
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This array will have the wrong shape. It needs to be reshaped.
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.
Interesting point (well, two points actually).
Using it this way, for a mesh with a texure with 2 coordinates per vertex, I actually get an array of shape
(NV, 2)
(whereNV
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 thetex_coord
property here (and is there a reason for this ?) ?(0, )
. Maybe it's not the best and I shoudl returnNone
instead, or an array of shape(0, 0)
?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 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.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 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)