KhronosGroup / glTF

glTF – Runtime 3D Asset Delivery
Other
7.19k stars 1.14k forks source link

Animation path syntax for objects referenced by nodes #1346

Closed donmccurdy closed 5 years ago

donmccurdy commented 6 years ago

The currently supported values for channel.target.path are translation, rotation, scale, and weights. All of these are properties unique to the mesh itself, rather than properties of referenced objects like materials, cameras, or lights.

Eventually we will want to support animation of other properties (light intensity comes to mind), and will need to adopt a forward-compatible syntax for that. https://github.com/KhronosGroup/glTF/pull/1301 by @najadojo proposes one possible syntax, as part of an extension adding support for animating a broad variety of properties. I'd like to separate the syntax question from that PR and discuss it here — a universal property animation extension has a lot of moving parts and is something I want to consider cautiously, but I am confident that some property animation is needed eventually, and having a syntax agreed upon would allow us to more easily include animation from the beginning in future extensions like KHR_lights.

The proposal from #1301 relies on JSON pointers to reference the affected object without going through a particular node, like:

"target": "/materials/1/pbrMetallicRoughness/baseColorFactor"

Some other syntax ideas:

// Node-relative path.
"target": {
  "node": 23,
  "path": "pbrMetallicRoughness/baseColorFactor"
},

// Node-relative path to specific primitive(s).
"target": {
  "node": 23,
  "path": "mesh/primitives/{}/material/pbrMetallicRoughness/baseColorFactor"
},

// Add 'property' qualifier.
"target": {
  "node": 23,
  "property": "material",
  "path": "pbrMetallicRoughness/baseColorFactor"
},
"target": {
  "node": 23,
  "property": "light",
  "path": "intensity"
}

These all have tradeoffs:

najadojo commented 6 years ago

Node-relative and 'property' qualifier don't seem to be complete in your examples. Which primitive's material is targeted? Same problem if more than one light was allowed per node. This doesn't seem to generalize well.

glTF already embodies a single object referenced multiple times. By this I mean the way that any number of mesh primitives can refer to one material, many materials can refer to a single texture, and animation channels targeting node properties from the top level (weights is the exception (and most interesting for this discussion why?)). Data is always at the top level. Node-relative paths don't really follow that pattern; they imply that the data referred to is unique and breaks this model. Further authoring tools such as Blender and Maya share this data view and an animation is authored is on a "global" value it changes anywhere that value is referred.

donmccurdy commented 6 years ago

Node-relative and 'property' qualifier don't seem to be complete in your examples. Which primitive's material is targeted? Same problem if more than one light was allowed per node.

These follow the existing weights path — all primitives of the mesh are required to "match" and animate together. I don't know if that requirement generalizes well, but I do think we should at least consider the option of continuing that way.

Further authoring tools such as Blender and Maya share this data view and an animation is authored is on a "global" value it changes anywhere that value is referred.

That's a really helpful point, thanks. Part of my concern here was "throwing away" information that would tell the the client it can reuse a material, but if DCC tools can't author it that way, there's nothing lost.

Here are a few additional ideas then, more similar to your original suggestion:

// Each target has a unique property identifying initial scope.
"target": {
  "material": 2,
  "path": "pbrMetallicRoughness/baseColorFactor"
}
"target": {
  "light": 3,
  "path": "intensity"
}

// Target has 'type' enum and 'index'.
"target": {
  "type": "LIGHT",
  "index": 4,
  "path": "intensity"
}

These are more verbose than arbitrary JSON pointers, similarly expressive, and (maybe?) more backward-compatible. The spec would define interpretation of the types; it is not self-defining like a JSON pointer.

najadojo commented 6 years ago

A semantic where all primitives of a mesh match a target isn't expressive enough. Imagine having a skinned mesh that has some primitives that I want to animate material baseColorFactor and other primitives I don't or with a different track. This form suggests that I would create independent nodes to target those material instance at once. Because of that I'd need to create a separate skeleton and pair the channels for skeletons for the animated material and the unanimated material mesh parts.

The primary reason I like JSON pointers is it allows a glTF extension to define its own animatable properties without having to create additional schema and objects. In your light example above you don't reference KHR_lights it seems its implicitly added somewhere. How would a vendor extension allow targeting its properties which are allowed and which aren't? It seems like both JSON pointers and "type" enums would both just declare it in their extension document similar to how MSFT_texture_dds does for mimeType.

Before we can narrow in on a syntax we like we should agree on if we are targeting as objects cloned at their node (weight style) or are we target top level objects (TRS style).

lexaknyazev commented 6 years ago

Before we can narrow in on a syntax we like we should agree on if we are targeting as objects cloned at their node (weight style) or are we target top level objects (TRS style).

FWIW, it could be specified per-channel: someone may need to apply the same animation to all instances of the top-level object.

donmccurdy commented 6 years ago

A semantic where all primitives of a mesh match a target isn't expressive enough.

Ok, I'm happy to eliminate pbrMetallicRoughness/baseColorFactor applying to all primitives' materials, but there are variations of node-relative and type-qualifier syntax that do not have this issue. For example:

// Add 'property' qualifier.
"target": {
  "node": 23,
  "path": "mesh/primitives/3/material/pbrMetallicRoughness/baseColorFactor"
},

Because of that I'd need to create a separate skeleton and pair the channels for skeletons

Your point stands regardless, but note that two meshes can reference the same skin.

The primary reason I like JSON pointers is it allows a glTF extension to define its own animatable properties without having to create additional schema and objects.

I agree that glTF extensions should not have to define additional schema for this, but if some one-time additions to the schema like property or type will result in a cleaner starting point for future extensions, that's worth it. In particular I'm not sure that allowing target be an object OR a string is backward-compatible.

How would a vendor extension allow targeting its properties which are allowed and which aren't?

In any case a vendor extension should define which properties may be targeted and which cannot. I feel pretty strongly that if we can't enumerate animatable properties, we can't expect engines to implement consistent support.

It seems like both JSON pointers and "type" enums would both just declare it in their extension document...

Yes, I think both are similar in that respect.

FWIW, it could be specified per-channel: someone may need to apply the same animation to all instances of the top-level object.

I'm not sure what you mean by per-channel? A channel could say that animation applies to just one instance of a material, or to the shared top-level material? That seems too flexible, I think, as having just top-level object animation enables the same features with less complex implementation.

najadojo commented 6 years ago

FWIW, it could be specified per-channel: someone may need to apply the same animation to all instances of the top-level object.

Something like:

// all of Node 23's use of material 1 is replaced with a cloned material 1 when the animation is playing
"target": {
  "node": 23,
  "path": "/materials/1/pbrMetallicRoughness/baseColorFactor"
},
// All instance of material 1 animate when playing
"target": {
  "path": "/materials/1/pbrMetallicRoughness/baseColorFactor"
}

I wonder if something like a object clone indicator would be helpful for clients.

"materials": [
  {
    "pbrMetallicRoughness": {
      "baseColorFactor": [0.8,0.4,0.8,1],
      "metallicFactor": 0
    }
  },
  {
    "extensions": {
      "KHR_clone": {
        "clone": "/materials/0"
      }
    }
  }
]

This way the node channel targeting wouldn't be needed; the path would just index the cloned material.

najadojo commented 6 years ago

In particular I'm not sure that allowing target be an object OR a string is backward-compatible.

In what context do you mean backward-compatible? Are you referring to having something in v.next that is still parseable 2.0 parsers? I'm assuming this just in the context of a 2.0 extension where we can redefine channels and targets in our extension object.

lexaknyazev commented 6 years ago

I'm not sure what you mean by per-channel?

I mean that a channel could target, e.g. material "prototype" or material "instance" (as in @najadojo's example above). DCC tools usually support flexible bindings. Also, targeting "prototype" may work as runtime optimization so that engines have some guarantees and don't need to clone animated objects.

donmccurdy commented 6 years ago

I'm assuming this just in the context of a 2.0 extension where we can redefine channels and targets in our extension object.

Even in an extension targeting glTF 2.0, it is preferable to allow the extension to be "optional". If the type of an existing property is changed, I think the extension would always need to be included in extensionsRequired.

najadojo commented 6 years ago

Even in an extension targeting glTF 2.0, it is preferable to allow the extension to be "optional". If the type of an existing property is changed

This why #1301 has its own channels list, no types are changed.

greggman commented 5 years ago

I think this discussion is above but the current format has issues.

Wouldn't it arguably be more open if the format changed to something like

"target": {
  "type": "node",
  "index": 23,
  "path": "rotation"
},

Then based on type you could effectively do this

  const target = gltf[target.type + 's'][target.index];

Where as now as you expand to new types of animation you have to just special case a bunch of stuff

     if (target.node !== undefined) {
         target = gltf.nodes[target.node];
     } else if (target.material !== undefined) {
         target = gltf.nodes[target.node];
     } else {
         // unknown target type
     }
donmccurdy commented 5 years ago

Closing this issue, since most of the discussion is in #1301 and it seems better to keep things moving there.

@greggman thanks — any thoughts on #1301 would be welcome!