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

Morph Target Names #1036

Open
feiss opened this issue Jul 10, 2017 · 20 comments

Comments

@feiss
Copy link

commented Jul 10, 2017

It's not unusual to name morph targets so artists and programmers can refer to them by name, not index. An example in Blender:

image

But I don't see how 2.0 supports morph target names (https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/README.md#morph-targets). It seems it does not. Any thoughts on this?

@lexaknyazev

This comment has been minimized.

Copy link
Member

commented Jul 10, 2017

glTF 2.0 uses integer indices to refer to different entities, because they're stored in JSON arrays. If an application wants to add string identifiers, it could use extras property like this (not a part of spec, just an example):

{
  "meshes": [
    {
      "primitives": [
        {
          "attributes": {
            "POSITION": 0
          },
          "targets": [
            {
              "POSITION": 1
            },
            {
              "POSITION": 2
            }
          ],
          "extras": {
            "targetNames": [
              "Happy",
              "Sad"
            ]
          }
        }
      ]
    }
  ]
}
@feiss

This comment has been minimized.

Copy link
Author

commented Jul 11, 2017

Yeah I guess that could work

@donmccurdy

This comment has been minimized.

Copy link
Member

commented Jul 11, 2017

Perhaps we should leave this open under glTF Next to consider named morph targets in a future version of the spec. It sounds like something that will be useful enough to implement via extras now, so bringing that into the spec eventually would be helpful.

@lexaknyazev

This comment has been minimized.

Copy link
Member

commented Jul 11, 2017

Having two ways of doing something (in this case, referring to a morph target) isn't a good thing for data format in general.

I would think of some sort of "Pose" extension which could cover this use case as well as named and predefined skeleton states. Sometimes, model "emotion" consists of both skeleton transforms and morph weights.

@donmccurdy

This comment has been minimized.

Copy link
Member

commented Jul 11, 2017

Having two ways of doing something (in this case, referring to a morph target) isn't a good thing for data format in general.

Right, if anything this should be similar to the name properties used on nodes — available as annotations for engines that use them (three.js and BabylonJS both do) but never used as references within the asset.

@takahirox

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2018

I really want morph targets names as @feiss mentioned morph targets are often named for artists/programmers. I know Blender and other tools support morph targets names.

I think adding name property into targets elements is good (if morphTargets in glTF next is based on glTF 2.0). It's user-defined name, like node's, and isn't used as reference as @donmccurdy mentioned.

"primitives": [
  {
    "attributes": {
      "POSITION": 0
    },
    "targets": [
      {
        "name": "happy",
        "POSITION": 1
      },
      {
        "name": "sad",
        "POSITION": 2
      }
    ]
  }
]
@donmccurdy

This comment has been minimized.

Copy link
Member

commented Jan 17, 2018

Another syntax option might be similar to what @lexaknyazev suggested, but without extras:

"primitives": [
  {
    "attributes": {
      "POSITION": 0
    },
    "targets": [
      {
        "POSITION": 1
      },
      {
        "POSITION": 2
      }
    ],
    "targetNames": [
      "happy",
      "sad"
    ]
  }
]

Could help with issues like KhronosGroup/glTF-Blender-Exporter#100, where the order of morph targets is not stable or obvious to the artist within a DCC tool.

@donmccurdy

This comment has been minimized.

Copy link
Member

commented Jan 17, 2018

^The syntax above is easier because it doesn't break syntax and could potentially be in a 2.X release. Unfortunately I think @takahirox's suggestion would have to be considered a breaking change, based on:

Each target in the targets array is a dictionary mapping a primitive attribute to an accessor containing Morph Target displacement data, currently only three attributes (POSITION, NORMAL, and TANGENT) are supported.

If we were willing to break syntax, my vote might be to also move weights inline:

"targets": [
  {
    "name": "smile",
    "weight": 0.5,
    "attributes": {
      "POSITION": 1
    }
  }
]
@takahirox

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2018

Ah, right. And in my suggestion having different types (indices to attributes and strings as name) in a dictionary isn't good. A bit complicate to parse.

I personally prefer the latter one because we don't need to care if "targetNames and weigts lengts are equal to targets length" and that name style is similar to node's and others. But the former one would be better because of non-breaking and less text size?

could potentially be in a 2.X release.

I hope this gets in.

@takahirox

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2018

I came to think it'd better to have targetNames be under mesh like weights, not under primitive.

Is there any cases where we want different morph target name set across primitives, like

mesh.primitives[0]
  - targets[0] : "happy"
  - targets[1] : "sad"
mesh.primitives[1]
  - targets[0] : "smile"
  - targets[1] : "lonely"

?

@donmccurdy

This comment has been minimized.

Copy link
Member

commented Jan 18, 2018

Oh, great point, I'd forgotten weights were on the mesh and not the primitive... worth noting that glTF animations target nodes, not primitives, and so having unrelated morph targets across primitives is probably something to discourage anyway.

"primitives": [
  {
    "attributes": {
      "POSITION": 0
    },
    "targets": [
      {
        "POSITION": 1
      },
      {
        "POSITION": 2
      }
    ]
  }
]
"weights": [0, 0],
"targetNames": [
  "happy",
  "sad"
]
@Ziriax

This comment has been minimized.

Copy link

commented May 22, 2019

I got a request to add this to Maya2glTF. Has a decision been made regarding this? If not, any advice on what to pick for maximum compatibility with existing engines?

Thanks!

@donmccurdy

This comment has been minimized.

Copy link
Member

commented May 22, 2019

@Ziriax three.js looks for .extras.targetNames as a hint, on the mesh definition (not the primitive). Blender supports this on import, with a TODO to support it on export: KhronosGroup/glTF-Blender-IO#362.

@Ybalrid

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

Looks like the mesh.extra.targeNames string array is getting some consensus. However this is not part of the specification. (Maybe something in a future 2.x version?)

In the mean time, would it be acceptable to add a non-normative "implementation note" to section §Concepts/Geometry/Meshes/Morph Targets

Something like that:

begin of text to add:


Implementation note: Although morph target are referenced by their position in the mesh.primitives[].targets array, many tools and application can use and benefit for having an associated name to a morph target. This functionality is not currently supported by the glTF 2.0 file format.

In practice however, many current implementation will check for the presence of an array of strings in mesh.extras.targetNames of the same size as the morph target array.

This practice is not currently standard. A future version of the glTF file format may include a standard way to store morph target names.

Each string of this array, if present, is a hint about the name of the morph target that can be presented in an user interface. See content of extras property in the example below :

"meshes": [{
	"extras": {
		"targetNames": [
			"target_Smile",
			"target_Cry"
		]
	},
	"weights": [
		0,
		0
	],
	"primitives": [{
		"attributes": {
			"POSITION": 0,
			"NORMAL": 1
		},
		"targets": [{
				"POSITION": 2,
				"NORMAL": 3
			},
			{
				"POSITION": 4,
				"NORMAL": 5
			}
		],
		"indices": 6
	}]
}]

end of text to add

I would like to open a PR to implement the proposed addition...
Any suggestion about this? (I know the pasted JSON section is probably a bit too much -- I'll probably leave it out.)

Edit: This could also be formalized in an tiny extension? Would that be worth it? The extras parameters is already implemented in a number of places, for example the blender gltf plugin...

@donmccurdy

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

I'd definitely like to see targetNames added in whatever the next version of glTF is. Whether we should do anything additional in the meantime, I'm not sure... it feels a little silly to make such a small extension, even though it isn't much trouble. But documenting .extras.targetNames in the official spec feels a bit weird too. 😕

@Ybalrid

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

Hey @donmccurdy thanks for your input!

For me, It really depends on when "the next version of glTF" will happen... If it's in a little while, well, maybe we should bother with explaining the stop-gap solution?

Such an extension is indeed a bit silly, and like you I also feel like it is a bit strange to have documentation about an extra field (that is supposed to exist to be able to add application specific things to the glTF format if I understand the design logic behind extras properties).

It is just that I had to google for and find this discussion for finding the answer for "is there a way to get the name of morph targets in a glTF?" And I found this, and that the exporter in the lattes beta of blender 2.8 I downloaded was writing this array of strings, so I could use that directly.

I am aware of another exporter that also bother exporting target names in that way, and I think your glTF webGL viewer (three.js based?) also can read these names and show them in the UI, but I haven't checked this usage was super widespread among software compatible with glTF. Judging by the number of times this issue has been referenced by external repository discussions using the # feature of GitHub markdown, it seems that whether or not we hint about this in the main document, people are today writing software that check if that property exists and loads it's content anyway.

It is not ugely important, and not all application that consume glTF assets needs to have names for their morph targets, it's just a "nice to have feature".

What I suggest is obviously not to set in stone the extras property at all, it's to add a pointer as a non-normative note that this is a relatively common usage of mesh.extras.targetNames. I don't really know if it's a good idea, I'm just asking for input here.

I have updated my comment above to add a blurb explicitly saying "this is not standard, a future version of glTF may do this differently".

Personally, I think this is so small of a thing that it doesn't deserve an extension. However, some extensions like KHR_materials_unlit are also way smaller. (it's only content is "does it exist on this material") so maybe a mesh could have an XXX_target_names extension field that contains morph target names as a simple array of strings ? But in that case, instead of having 1 "non standard" way of doing things, we would have 2 (or 1.5 if you consider the fact that extensions are documented here to be also part of the file format specifications, but are more akin to "optional additions").

@donmccurdy

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

There is no ETA on glTF 2.X or 3.0 at this time – we are currently focused on extensions.

I'd be OK with adding a non-normative implementation note in the spec. Any objections?

@Ybalrid

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

Ybalrid added a commit to Ybalrid/glTF that referenced this issue Jun 18, 2019

Add non-normative note about the current usage of mesh.extras.targetN…
…ames

Morph target names are not required to perform morphing. A real time application that consume glTF assets doesn't need morph target names in the general case. 

Rightly so, glTF doesn't impose assets to have target names, but it also doesn't provide a way to optionally add them.

It seems that, since the release of glTF 2.0 in mid-2017, a number of implementation (three.js, the official Blender exporter, UnityGLTF, a and at least one Maya exporter (and probably some others software, I haven't made a list, these are the few I'm aware of)).

This has been discussed in issue KhronosGroup#1036. 

The goal is to help implementations that wants to do so have a way to do so in an interoperably way. Currently this has been done by adding a `targetNames` extras properties to the object.

Maybe a more proper way would be to have an extension doing so, however, this practice is already quite widespread, so I would argue that at least documenting it here and integrate a better (and standardized) way of doing so in glTF 2.x/3.0 is the better thing to do.

I am not opposed to the idea of writing an extension to support morph target names, but that would add 2 *not really standard* way of doing it instead of the one already out in the wild.
@Ybalrid

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

I've opened PR #1631. If you agree that's a good idea, we can go discuss the actual wording to add to the document here.

@donmccurdy

This comment has been minimized.

Copy link
Member

commented Jun 23, 2019

Implementation note (#1631) has been merged. Leaving this issue open under the "glTF Next" milestone, so that we can consider something in the spec when we get there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.