Skip to content
This repository was archived by the owner on Dec 22, 2023. It is now read-only.

Update SimpleSkin to place the parent bone at the origin.#335

Merged
emackey merged 2 commits intomasterfrom
simple-skin-its-complicated
Nov 19, 2021
Merged

Update SimpleSkin to place the parent bone at the origin.#335
emackey merged 2 commits intomasterfrom
simple-skin-its-complicated

Conversation

@emackey
Copy link
Member

@emackey emackey commented Nov 15, 2021

@javagl This is my attempt at fixing KhronosGroup/glTF-Tutorials#44.

Currently this model's two bones share the same location. This is visible by importing into Blender, although the orientation of both nodes is shown incorrectly. Blender can't correctly guess the bones' primary axis, due to them being colocated.

Simple_Previous.mp4

This PR moves the translation and its associated inverseBindMatrix such that one of the two nodes is positioned at the origin. Blender's import gets this one correct, because it presumes the child has moved to the end of the parent bone. But more importantly, the parent bone really is placed at the origin now.

Simple_Good.mp4

@emackey
Copy link
Member Author

emackey commented Nov 15, 2021

Here's a visual diff of the .bin file before and after. One -1.0 has been changed to 0.0.

bin diff screenshot

@emackey emackey force-pushed the simple-skin-its-complicated branch from e329eac to a01b429 Compare November 15, 2021 22:35
@emackey
Copy link
Member Author

emackey commented Nov 15, 2021

Of course, I started from the sample-model default branch here, not @javagl's latest. So, this probably can't be merged as-is, and would need to be rebased on @javagl's latest work.

@emackey emackey marked this pull request as draft November 15, 2021 22:46
@javagl
Copy link
Contributor

javagl commented Nov 15, 2021

(Sorry, I had written my latest comment in the linked PR before I saw this PR here). I still wonder how it can be justified that the root/parent node does not have a translation. I.e. if the parent bone had a length of 2.0, would a translation of (0,2,0) be stored in the child node? That seems strange.

But more importantly, the parent bone really is placed at the origin now.

Even if the parent has a translation of (0,1,0), it is still at the origin (only everything that is attached to it would not be). But maybe this can be sorted out in the other PR.

@javagl
Copy link
Contributor

javagl commented Nov 17, 2021

Based on the dicussion in the linked issue, I think that this could be merged. (Or I could create a similar PR, and update the data layout images at https://github.com/KhronosGroup/glTF-Sample-Models/tree/a01b42951ed199597936330ada30bfa4a41c2d05/2.0/SimpleSkin#data-layout accordingly...)

@emackey emackey marked this pull request as ready for review November 18, 2021 14:45
@emackey
Copy link
Member Author

emackey commented Nov 18, 2021

@javagl Yes, it looks like KhronosGroup/glTF-Tutorials#64 and this PR are in agreement currently.

Do you want to merge this one? Or should we hold for an updated diagram?

@javagl
Copy link
Contributor

javagl commented Nov 18, 2021

I'll to the final wording tweaks in the other PR and post a notification when ready (not later than during the weekend). Creating the updated diagrams is simple (as in writeDiagrams(createSimpleSkinData())), but I'll probably check and compare the diagrams and data several times...
Depending on what you prefer, I could just add them to this PR branch, or branch off from this one and create a PR with the diagrams added. (Merging this and then creating a PR for the diagrams only would be possible, but not as clean).

@emackey
Copy link
Member Author

emackey commented Nov 18, 2021

If you can just add them to this branch, that sounds easiest. Thanks!

@emackey
Copy link
Member Author

emackey commented Nov 19, 2021

Thanks @javagl!

@emackey emackey merged commit 20656ea into master Nov 19, 2021
@emackey emackey deleted the simple-skin-its-complicated branch November 19, 2021 19:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants