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

Animation path syntax for objects referenced by nodes #1346

Closed
donmccurdy opened this issue May 24, 2018 · 12 comments
Closed

Animation path syntax for objects referenced by nodes #1346

donmccurdy opened this issue May 24, 2018 · 12 comments

Comments

@donmccurdy
Copy link
Contributor

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. #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:

  • Because it's important for runtime assets to have as few materials as possible, does duplication of materials for node-specific animation throw away useful animation? Is it better to ask that clients know when to de-duplicate (for performance) or when to duplicate (if per-node animation requires it)?
  • Do we expect animation on anything that isn't associated with a node?
  • Should a single animation be able to target multiple properties? e.g. several materials on the same mesh?
@najadojo
Copy link

  • Maybe, the first part of your question isn't clear. If you're asking is it common to have an animation on a material apply every where the material appears, then no that doesn't seem to be common. The typical case given to me was tanks in battle, you'd want some of them to animate to be come more and more damaged as they take fire. Each tank would instance a bunch of things but each has unique material values. The counter example of this would be aircraft warning lights where you want them to blink all at once. I believe clients have the best knowledge of material states in their scene to know when meshes could be combined or should be broken apart. I have seen this type of optimization in a couple of engines namely Unity seems to do it.
  • Yes there are items that exist on the scene that make sense to be animated, i.e. ambient light color
  • Yes since an animation is a collection of channels multiple properties should be modifiable in the single collection. Channels should stick to being a single property at a time.

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
Copy link
Contributor Author

donmccurdy commented May 24, 2018

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
Copy link

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link

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
Copy link

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor

greggman commented Dec 18, 2018

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
Copy link
Contributor Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants