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

Add support for gltf morph names. #1100

Merged
merged 11 commits into from Jul 11, 2019

Conversation

Projects
None yet
4 participants
@tlf30
Copy link
Contributor

commented May 28, 2019

OK, so I am putting a RFC (Request For Comments) here to get some feedback.

glTF-Blender-IO just added support for shapekey names (names on mesh morphs) to the import/export in blender. (Info found here: KhronosGroup/glTF-Blender-IO#362)

Seeing as mesh morphs are becoming a more popular feature, I think it would be nice if we had a way to facilitate them. I have made some changes and added glTF support for mesh morph names. This is a rough work in progress, but I wanted some feedback on the core jme changes here to see what everyone thinks.

PS: Sorry in advance about the Geometry formatting changes, those will be reverted before the final revision of the PR.

@stephengold

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

What's holding up the revert of the format changes?

@tlf30

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

@stephengold I am home from work right now, but right now I am getting caught up on chores from being gone.
I will get this PR finished up before I leave again. I leave on June 19th for another two weeks.

@pspeed42

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

@stephengold Note that this PR is in no way complete anyway. The final version probably won't even have any changes to Geometry.java at all. This was a "request for comment" and so my understanding was that it was only a partial start. Probably would have been better done as a fork until ready to be a PR but whatever.

@tlf30

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

Yes, there are several things that will change here. I just wanted to know where to make the changes and if they were acceptable to make. The logic behind the named shape keys will be moved to the Mesh class, and I need to update this branch with the finished changes to the gltf plugin, and the formatting changes reverted.

@tlf30 tlf30 changed the title RFC: Add support for gltf morph names. Add support for gltf morph names. Jun 15, 2019

@tlf30

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2019

@stephengold @pspeed42
I think this is ready for review.

Thanks,
Trevor Flynn

EDIT: Please see my comment on the hub: https://hub.jmonkeyengine.org/t/feedback-requested-pr-for-shapekey-name-support/41892/17

@stephengold

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

I looked this over, and I don't see any major issues, just minor ones with English grammar and formatting. @pspeed42, do you plan to review this PR?

@pspeed42

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

I would like to review it... as soon as I figure out what it's for. :) I've never used morph names before so I have a learning curve.

* Morphs without names will be null
* @return an array
*/
public String[] getMorphTargetNames() {

This comment has been minimized.

Copy link
@Ali-RS

Ali-RS Jun 20, 2019

Member

Not sure why getMorphTargetNames() is needed?
We can already access morph names using getMorphTargets().

@Ali-RS

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

What about replacing this block inside getMorphState()

        int index;
        boolean found = false;
        for (index = 0; index < nbMorphTargets.length; index++) {
            if (nbMorphTargets[index].getName().equals(morphTarget)) {
                found = true;
                break;
            }
        }
        if (found) {
            return morphState[index];
        }

with this one


 int index = mesh.getMorphIndex(morphName); // need to add `getMorphIndex` into Mesh class
 if (index < 0) {
   return -1;
 } else  {
   return morphState[index];
 }

and this inside setMorphState

 int index;
        boolean found = false;
        for (index = 0; index < nbMorphTargets.length; index++) {
            String name = nbMorphTargets[index].getName();
            if (name != null && name.equals(morphTarget)) {
                found = true;
                break;
            }
        }
        if (found) {
            morphState[index] = state;
            this.dirtyMorph = true;
        }

with this one?

int index = mesh.getMorphIndex(morphName); // need to add `getMorphIndex` into Mesh class
if (index < 0) {
   return;
} else {
   morphState[index] = state;
   this.dirtyMorph = true;
}
@tlf30

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

@Ali-RS I do not think it would be a good idea to add a getMorphIndex method, as it is an implementation dependent method. In the future (possible random scenario) if we used a hashmap to store morphs then the method would no longer be relevant and need to be removed. (Just an example, not saying it will happen)

hasMeshTargets looks good. I'll throw that change in and fix the javadoc bugs.

@tlf30

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

@Ali-RS so after thinking it over I did go with your recommendation. I think in the future as we get more support for shape key names, it would be good to move away from storing morph shapes as an array and move to an hashmap, but for now, as the implementation stands, and probably will stand for some time, it is the best approach to just keep is as an array and make it as easy as possible to use in that form.

I think all the final changes are done, thoughts?

@tlf30

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

@Ali-RS thanks for the catch on those. (I was working on it late last night 😪 )

@Ali-RS

This comment has been minimized.

Copy link
Member

commented Jul 7, 2019

Sorry to be picky, but do not you think method Geometry.getMorphNames() is extra? It just shifts the call to Mesh.getMorphTargetNames().

I would prefer to remove it, I might be wrong anyway ;)

@pspeed42

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2019

Yeah, seems unnecessary to me. I'm not sure I understand why Geometry needs to know about morph names since it's a mesh thing.

@tlf30

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

@Ali-RS @pspeed42
While I agree with you, I was trying to be consistent with the way morphs are implemented. The Geometry class has a similar function to get the morph values:

EDIT: What probably should happen is we deprecate getMorphState and do not include getMorphNames in this PR.

EDIT 2: Well, it looks that getMorphState is used internally in the Geometry class....

@pspeed42

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2019

Yes, the morph states are the states of the morphs... which is geometry-specific. That's why the array returned is empty... it's for putting values into. At least that's my understanding.

@tlf30

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

OK, getMorphNames is gone

@Ali-RS

This comment has been minimized.

Copy link
Member

commented Jul 7, 2019

Changes look good to me, I guess it's ready to be merged, let's leave it open for a few more days in case someone else wants to review it.

@pspeed42

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2019

I'm kind of clueless. Can someone describe how this would be used in practice? If it's already been explained then just pointing me to it would be fine. Thanks.

@tlf30

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

@pspeed42
When you have mesh morphs, (or otherwise known as shape keys) they get exported in an arbitrary order from whatever modelling software created them. Applications such as Blender allow the user to assign names to these morphs. When using the morph in JME3, previously it required going though each morph and figuring out which index correlated to which morph, then hard coding it into the application. Now we can use the name of the morph to determine which morph we are accessing. There is no more trial and error, or hard coding indexes of morphs.

An example would also be if someone added a new morph to a model, they would have to repeat the process of figuring out which morph is which in JME, but now they can just access them all by their names.

I hope that clears it up for you

@Ali-RS

This comment has been minimized.

Copy link
Member

commented Jul 7, 2019

To add on what tlf30 said, my understanding is, in JME morph state/weight can be modified in two ways.
1- With MorphTrack animation
2- By manually setting the weight of morph (common use cases are facial expressions and lip-syncing)

I think the aim of this PR is to facilitate manual modification of morph weights.

@pspeed42

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

Thanks, guys.

@Ali-RS Ali-RS merged commit 57db8f6 into jMonkeyEngine:master Jul 11, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.