PR for shapekey name support

Anyway i got one question about PR.

there is:

JsonArray targetNames = meshObject.getAsJsonArray("targetNames");

do it magically work via some reflection? or maybe something is missing?

because i dont seen any usage of targetNames.

so i just let know, because maybe some additional code is missing. Sometimes fresh view can catch errors like this.

Note, as mentioned in the first post this PR is still WIP means it is not completed yet and missing some parts.

1 Like

ah ok, sorry :slight_smile:

one more thing i just noticed. (i know its about PR, but its shape key related, i dont want create new topic now, its just a reminder for future shape key fixes)

for some reason i cant add more shape keys - i understand there were some limit for one verticle shapes. but i touch verticles that are not used by other shape keys, but anyway it throw error.

its about:

/**
 * Vertex attribs currently bound and enabled. If a slot is null, then
 * it is disabled.
 */
public VertexBuffer[] boundAttribs = new VertexBuffer[16];

each shape key seems to add 2 indexes (before remove i had 19)

SEVERE: Uncaught exception thrown in Thread[jME3 Main,5,main]
java.lang.ArrayIndexOutOfBoundsException: 17
at com.jme3.renderer.opengl.GLRenderer.setVertexAttrib(GLRenderer.java:2755)
at com.jme3.renderer.opengl.GLRenderer.setVertexAttrib(GLRenderer.java:2808)
at com.jme3.renderer.opengl.GLRenderer.renderMeshDefault(GLRenderer.java:3040)
at com.jme3.renderer.opengl.GLRenderer.renderMesh(GLRenderer.java:3077)
at com.jme3.material.logic.DefaultTechniqueDefLogic.renderMeshFromGeometry(DefaultTechniqueDefLogic.java:70)
at com.jme3.material.logic.SinglePassAndImageBasedLightingLogic.render(SinglePassAndImageBasedLightingLogic.java:260)
at com.jme3.material.Technique.render(Technique.java:166)
at com.jme3.material.Material.render(Material.java:1024)
at com.jme3.renderer.RenderManager.renderGeometry(RenderManager.java:614)
at com.jme3.renderer.queue.RenderQueue.renderGeometryList(RenderQueue.java:266)
at com.jme3.renderer.queue.RenderQueue.renderQueue(RenderQueue.java:305)
at com.jme3.renderer.RenderManager.renderViewPortQueues(RenderManager.java:877)
at com.jme3.renderer.RenderManager.flushQueue(RenderManager.java:779)
at com.jme3.renderer.RenderManager.renderViewPort(RenderManager.java:1108)
at com.jme3.water.WaterFilter.preFrame(WaterFilter.java:180)
at com.jme3.post.FilterPostProcessor.preFrame(FilterPostProcessor.java:368)
at com.jme3.renderer.RenderManager.renderViewPort(RenderManager.java:1078)
at com.jme3.renderer.RenderManager.render(RenderManager.java:1158)
at com.jme3.app.SimpleApplication.update(SimpleApplication.java:270)
at com.jme3.system.lwjgl.LwjglAbstractDisplay.runLoop(LwjglAbstractDisplay.java:151)
at com.jme3.system.lwjgl.LwjglDisplay.runLoop(LwjglDisplay.java:197)
at com.jme3.system.lwjgl.LwjglAbstractDisplay.run(LwjglAbstractDisplay.java:232)

its connected somehow with water filter?

anyway i currently have only 5 shape keys used, and its max for me…

did you had similar issues?

OK, I have hopped back into this again.
SO, the state of the morphs is currently (jme-master) handled by the geometry class.
What I have added to geometry is manipulating the state of the morphs via name. There is no way as far as I can tell to do this via the mesh class. What I am moving into the mesh class is the function to get the morph names, and I am exposing it (as getMorphNames) in the geometry class just like getMophState is exposed.

With that, I have also added the morphName to be saved and loaded in the MorphTarget class.

Thoughts?

1 Like

OK, I think it is ready. Everyone can check it out on github.

1 Like

thank you, recently i had some issues with random shape key number on export so it sounds like important PR. code looks ok for me.

btw i hope there is no way that JSON “extras” have less or more records than “targets” because targetNames.pop() might not work correctly. because i understand it cant be done in one loop.

1 Like

That is correct. My understanding is that a correctly formatted gltf file should always have the same number of targets as it does extra.targetNames. With that said, I have been debating about some error checking in there. I think the best course of action might be to throw some form of a runtime exception, but I am not sure yet.

EDIT:
With this PR, if someone with experience with FBX would like to, they can modify that importer to pull the shape key names as well. I have no experience working with FBX except for exporting from Daz Studio to import to Blender. But I know it keeps the shape key name information.

1 Like

Due to how I have this:

if (targetNames.size() > 0) {
    morphTarget.setName(targetNames.pop());
}

Even if there are more or less targetNames than targets, the code will at least not break anything. Which is a good behavior when dealing with a malformed file.

1 Like

sorry if i mislead you with my earlier comment.

without if condition seems to be more proper, i just said i hope in gltf there will never be situation when extra.targetNames is different length than targets.

it will not break code using this, but it would lead to odd issues without any exceptions.

like i would got shape keys in blender: 1:a, 2:b, 3:c, 4:d

and 1:a,2:c,3:d would be in targetNames,
then a = a, b = c, c = d that would be very unpredictible.

i understand probably it will never happen anyway, because extra.targetNames will probably always be same length as targets.

but anyway some exception or severe log should be thrown too.

i red https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/README.md

but i was unable to find anything about targetNames length or key relation.

Anyway Its just my personal opinion, but as i know its required to know something is wrong. (much easier to find issue later)

At this point in time, the extras.targetNames is not part of the official specification. (But I have heard that there is likely to be an official method of doing this in the future) But right now, the consensus between tools is to use the extras.targetNames.

Yes, I think there probably should be an exception thrown, I need to test an export in blender where I only set some of the shapekey names but not all to see what happens… (Can I even do that?)

1 Like

hmm. i would try add 3 shape keys in blender, and dont touch name for second and setup for 1 and 3.
Probably default names( like “Shape 1”) are send anyway? i dont know it, but might worth to check.

anyway if you say extras is not part of the official specification, then some condition would be worth to add just to know if there are extras like

if(extras) {

like you have “meshData.has(“extras”)” just for

if (meshData.has("extras") && meshData.getAsJsonObject("extras").has("targetNames")) {
    morphTarget.setName(targetNames.pop());
}

or put it in some variable to not execute getAsJsonObject a lot times.

boolean hasTargetNames = meshData.has(“extras”) && meshData.getAsJsonObject(“extras”).has(“targetNames”)

and use it later for both.

or am i wrong?

Ah, that is probably a good idea. Tonight I’ll make some optimization improvements to it.

1 Like

SO after looking at it. At the moment, the only thing stored in extras (that I can find) are the targetNames. The only thing I can optimize out by making the change is a single call to getJsonObject Considering that the this provides almost no benefit, I am going to leave the code as it is. In the future as the extras field is use more, it would be a good idea to build a section of the importer to handle the extras.

1 Like

ok then, seems like its done, thank you again :slight_smile:

OK, if everyone would like to review the PR, it is open for a bit for review before it gets merged in.

@tlf30 regarding morph state, my understanding is getMorphState() in Geometry class is actually returning current morph weight, Yes? If so then why it’s not named as getMorphWeight()?

@Ali_RS seeing as it is an existing function, I do not think it would be a good idea to rename it, as we do not want to break backwards compatibility for all the apps that are currently using the function.

@sgold what do you think?

Of course, I do not mean to rename it, as you said it may break stuff, I just wanted to get sure that I am understanding it right and morph state is actually the morph weight.

Ah sorry, I misunderstood you.

I believe that it is the raw values. It gets updated here from the mesh when the state is set.

public void setMorphState(float[] state) {
        if (mesh == null || mesh.getMorphTargets().length == 0){
            return;
        }

        int nbMorphTargets = mesh.getMorphTargets().length;

        if (morphState == null) {
            morphState = new float[nbMorphTargets];
        }
        System.arraycopy(state, 0, morphState, 0, morphState.length);
        this.dirtyMorph = true;
    }