KhronosGroup / glTF

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

Tightening up animations #704

Closed lexaknyazev closed 7 years ago

lexaknyazev commented 8 years ago
  1. Specify componentType for animation accessors (suggest only FLOAT).
  2. Remove count from example.
  3. To be more explicit,

    All parameters must have the same number of elements.

    could be changed to something like

    All accessors referenced by the same animation must have the same count property.

CC @pjcozzi @javagl @lasalvavida

lexaknyazev commented 8 years ago

More animation stuff:

Maybe it's useful to allow some output accessors to have less elements than TIME samples (if one channel stops before others).

javagl commented 8 years ago

I think whether it's called TIME or not is not sooo important - it does not have a "semantic" in that sense, and could equally be called timeKeyFrames or whatever.

The constraint of time values being monotonically increasing is already pointed out for sampler.input at https://github.com/KhronosGroup/glTF/tree/master/specification#samplerinput-white_check_mark .

(One could consider requiring it to be strictly monotonic. Handling the special case of time[n+1] == time[n] , could be fiddly and error-prone. But one would have to investigate further whether this special case is really never required)

EDIT: I mean, what should

TIME     0.0  1.0  1.0  2.0
output   3.4  5.6  7.8  9.0   

actually mean? I.e. what should the output be for time=1.0 ?

lexaknyazev commented 8 years ago

I think whether it's called TIME or not is not sooo important - it does not have a "semantic" in that sense, and could equally be called timeKeyFrames or whatever.

In that case we should demand all samplers to use the same input parameter. And that parameter still has to have TIME "semantic" (even when called otherwise).

The constraint of time values being monotonically increasing is already pointed out for sampler.input.

I've overlooked it, thanks.

One could consider requiring it to be strictly monotonic. Handling the special case of time[n+1] == time[n] , could be fiddly and error-prone. But one would have to investigate further whether this special case is really never required

I'd say it otherwise: we should forbid such case or define exact behavior (like always use last output value).

pjcozzi commented 8 years ago
  1. Specify componentType for animation accessors (suggest only FLOAT).

Do you mean for the examples?

  1. Remove count from example.

Yes, please open a PR into master.

TIME should be the only valid value for sampler.input #573 (comment);

Looking at the Cesium implementation, I don't think it requires this to be named TIME, but it does need to have the semantic as @javagl mentioned so I'm fine with relaxing this name requirement if you guys want.

One could consider requiring it to be strictly monotonic. Handling the special case of time[n+1] == time[n] , could be fiddly and error-prone. But one would have to investigate further whether this special case is really never required I'd say it otherwise: we should forbid such case or define exact behavior (like always use last output value).

The suggestion is to change the wording from "monotonically increasing" to "increasing", right? I think that is fine unless there is a real use case for several key frames to have the same time.

lexaknyazev commented 8 years ago

Specify componentType for animation accessors (suggest only FLOAT).

Do you mean for the examples?

I mean that FLOAT must be the only allowed animation accessor componentType (for TIME input as well as for translation/rotation/scale outputs).

Looking at the Cesium implementation, I don't think it requires this to be named TIME, but it does need to have the semantic as @javagl mentioned so I'm fine with relaxing this name requirement if you guys want.

Do you propose to state that input always refers to "time" semantic?

pjcozzi commented 8 years ago

I mean that FLOAT must be the only allowed animation accessor componentType (for TIME input as well as for translation/rotation/scale outputs).

+1 from me.

Do you propose to state that input always refers to "time" semantic?

Yes, so the name could be something else, but it follows the "time" semantic.

lexaknyazev commented 8 years ago

Yes, so the name could be something else, but it follows the "time" semantic.

Consider this example:

{
    "animations" : {
        "an_animation": {
            "channels": [
                {
                    "sampler": "a_sampler",
                    "target": {
                        "id": "node_id",
                        "path": "rotation",
                    },

                },
                {
                    "sampler": "b_sampler",
                    "target": {
                        "id": "node_id",
                        "path": "scale",
                    },

                }
            ],
            "parameters": {
                "TIME_a": "time_accessor_a",
                "TIME_b": "time_accessor_b",
                "rotation": "rotation_accessor",
                "scale": "scale_accessor"
            },
            "samplers": {
                "a_sampler": {
                    "input": "TIME_a",
                    "interpolation": "LINEAR",
                    "output": "rotation"
                },
                "b_sampler": {
                    "input": "TIME_b",
                    "interpolation": "LINEAR",
                    "output": "scale"
                }
            }     
        }
    }
}

Is it valid? Otherwise all inputs must reference the same parameter.

lexaknyazev commented 8 years ago

A bit more general question: why do we need parameters object at all? Why not reference accessors directly?

javagl commented 8 years ago

The suggestion is to change the wording from "monotonically increasing" to "increasing", right? I think that is fine unless there is a real use case for several key frames to have the same time.

I think it would be "strictly increasing", as of https://en.wikipedia.org/wiki/Monotonic_function#Monotonicity_in_calculus_and_analysis

Regarding the proposal to require FLOAT: Right now, the animation.target.path is constrained to rotation, translation and scale, so they are already implicitly FLOAT. But similarly to the point above, I'm wondering whether in the future there might be paths that need a different type (but can't think of one right now). I'll still have to wrap my head around the related questions, particularly the one about the use of parameters. Right now, I'm doing some archeology by reading some related issues ( https://github.com/KhronosGroup/glTF/issues/158 (the FLOAT question is discussed there!), https://github.com/KhronosGroup/glTF/issues/143 ... )

lexaknyazev commented 8 years ago

I'm wondering whether in the future there might be paths that need a different type (but can't think of one right now).

With disabled interpolation, it will be possible to animate integer properties and even enums (such as switching textures).


For 1.0.1 I propose this wording (for sampler.output):

The ID of a parameter in this animation to use as keyframe output. When parameter is used with TRS target, referenced accessor's componentType must be FLOAT.

lexaknyazev commented 8 years ago

A bit more general question: why do we need parameters object at all? Why not reference accessors directly?

Looks like some time ago parameters contained its own objects (very similar to current accessors). Then they were replaced with accessor references. Does parameters have any purpose aside from 1:1 links?

javagl commented 8 years ago

I thought about the time constraints again

I'd say it otherwise: we should forbid such case or define exact behavior (like always use last output value).

Yes, this makes more sense. Imagine someone wants to model

When time[n+1]==time[n] is allowed, this can simply be written as

TIME     0.0  1.0  1.0  2.0
xPos     0.0  1.0  8.0  9.0

When time[n+1]==time[n] is not allowed, then there's no proper way to express this...

TIME     0.0  0.999999999  1.0  2.0  // yuck!
xPos     0.0  1.0          8.0  9.0

So I think that there is no need to change the current wording/spec regarding this point. It should still be time[n+1] >= time[n] (greater-than or equal). The case of time[n+1] == time[n] will involve a special case treatment, but nothing that cannot be resolved (maybe with a few explanatory words in the spec).

Regarding the parameters: It looks like this had more sense once, but then "collapsed" through the use of accessors, and ... maybe it was just an oversight to not remove it. (Although I still didn't analyze whether they carry a meaning that goes beyond the 1:1 mapping)

lexaknyazev commented 8 years ago

So, do you mean this?

lexaknyazev commented 8 years ago

Also, time values sequence can nor begin with equal values, neither end with them.

javagl commented 8 years ago

Yes, I think that these constraints make sense.

Unambiguously and compactly describing all this in the spec is a bit fiddly, keeping in mind the inherent ambiguitiy that is imposed by >=. The paragraph from https://github.com/KhronosGroup/glTF/tree/master/specification#samplerinput-white_check_mark would have to be a bit more elaborate...

The values represent time in seconds with time[0] >= 0.0. The values have to be monotonically increasing, i.e. time[n+1] >= time[n]. The sequence may not start or end with consecutive equal values. There may not be more than two consecutive equal values. When time[n+1] == time[n] , then the key frame of time[n+1] will be used.

And still, the last sentence sounds fuzzy....


In general, in the implementation, one has to look up a start- and an end index for a given time to find the interpolation interval. I just played a bit with my current implementation (which does not support equal keys yet), and one indeed has to be careful

EDIT The wording in the //comments below is wrong - c.f. the following discussion

// Compute the smallest index where the currentTime is greater than or equal to TIMES[index]
int startIndex = computeStartIndex(TIMES, currentTime); 
int endIndex = startIndex + 1;

// Some special cases have to be treated here ...
if (endIndex >= TIMES.length || TIMES[startIndex] == TIMES[endIndex]) ...

but of course, it's manageable once the constraints and intended meaning are clear.

lexaknyazev commented 8 years ago

One more corner case (if multiple inputs are allowed): Can we have multiple channels with the same target (suppose no)? If yes, can their samplers have overlapping time frames (suppose no)?

Also, if different samplers can have different inputs, accessor.count values should be equal per-sampler, not per-animation.

RemiArnaud commented 8 years ago

I don't think this makes sense, having two possible values for the same time. One object cannot be at two different positions at the same time.

Regards,

-- Rémi

On Sep 9, 2016, at 4:12 AM, Marco Hutter notifications@github.com wrote:

I thought about the time constraints again

I'd say it otherwise: we should forbid such case or define exact behavior (like always use last output value).

Yes, this makes more sense. Imagine someone wants to model

a linear motion a jump a linear motion When time[n+1]==time[n] is allowed, this can simply be written as

TIME 0.0 1.0 1.0 2.0 xPos 0.0 1.0 8.0 9.0 When time[n+1]==time[n] is not allowed, then there's no proper way to express this...

TIME 0.0 0.999999999 1.0 2.0 // yuck! xPos 0.0 1.0 8.0 9.0 So I think that there is no need to change the current wording/spec regarding. It should still be time[n+1] >= time[n](greater-than or equal). The case of time[n+1] == time[n] will involve a special case treatment, but nothing that cannot be resolved (maybe with a few explanatory words in the spec).

Regarding the parameters: It looks like this had more sense once, but then "collapsed" through the use of accessors, and ... maybe it was just an oversight to not remove it. (Although I still didn't analyze whether they carry a meaning that goes beyond the 1:1 mapping)

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

lexaknyazev commented 8 years ago

One object cannot be at two different positions at the same time.

@javagl proposes to treat this as a special case: image instead of image

javagl commented 8 years ago

The problem obviously is that you would see artifacts at 5.005 in the second example. If this is not considered an issue, or there is a different solution, then that's fine.

accessor.count values should be equal per-sampler

That sounds reasonable. (Right now, I don't see any constraints on the count at all. Maybe that was the count from the example that was removed recently? ;-))

lexaknyazev commented 8 years ago

That sounds reasonable. (Right now, I don't see any constraints on the count at all

It's mentioned in readme, not in schemes.

pjcozzi commented 8 years ago

@lexaknyazev please let me know if I missed answering any questions above.

I have reviewed the Cesium implementation a bit (I wrote it a few years ago, and I'm not saying that it is the spec authority, but it may give some insight into how we intended things):

Otherwise all inputs must reference the same parameter.

If this constraint doesn't limit common use cases, I think it is fine. It could make the client implementation a tad easier. The Cesium implementation currently allows each time input to be separate (not sure that case has been tested though).

Does parameters have any purpose aside from 1:1 links?

It would be a prominent breaking change from 1.0 to 1.0.1 to remove this, so we would have to confirm with @tparisi and Neil, but I am also not sure what purpose they serve at this point.

pjcozzi commented 8 years ago

Also:

For 1.0.1 I propose this wording (for sampler.output):

The ID of a parameter in this animation to use as keyframe output. When parameter is used with TRS target, referenced accessor's componentType must be FLOAT.

This is OK with me; the Cesium implementation explicitly checks for this.

lexaknyazev commented 8 years ago

If this constraint doesn't limit common use cases, I think it is fine. It could make the client implementation a tad easier. The Cesium implementation currently allows each time input to be separate (not sure that case has been tested though).

Different sampling rate of each target could be better for filesize. We still need to demand equal accessor.count values for each input/output pair.

pjcozzi commented 8 years ago

If this constraint doesn't limit common use cases, I think it is fine. It could make the client implementation a tad easier. The Cesium implementation currently allows each time input to be separate (not sure that case has been tested though).

Different sampling rate of each target could be better for filesize. We still need to demand equal accessor.count values for each input/output pair.

I am also OK with this and think the filesize case is a good point.

lexaknyazev commented 8 years ago

@lexaknyazev please let me know if I missed answering any questions above.

This one

One more corner case (if multiple inputs are allowed): Can we have multiple channels with the same target (suppose no)? If yes, can their samplers have overlapping time frames (suppose no)?


And jumps handling: https://github.com/KhronosGroup/glTF/issues/704#issuecomment-245885318 https://github.com/KhronosGroup/glTF/issues/704#issuecomment-245950483

pjcozzi commented 8 years ago

One more corner case (if multiple inputs are allowed): Can we have multiple channels with the same target (suppose no)?

No, to keep the client simple.

pjcozzi commented 8 years ago

And jumps handling: https://github.com/KhronosGroup/glTF/issues/704#issuecomment-245885318 https://github.com/KhronosGroup/glTF/issues/704#issuecomment-245950483

I am still not following the solution to the special case; it sounds like the client is supposed to add an epsilon to time[n + 1] when time[n] == time[n + 1]? If that is the case, they could be done offline to keep the client simple.

lexaknyazev commented 8 years ago

It's exactly the opposite. As @javagl said, an offline epsilon could cause visual artifacts. With time[n] == time[n + 1] client uses the first output value as an interpolation target, while jumping instantly to the second as time comes.

lexaknyazev commented 8 years ago

Other solution could be to recommend such epsilon to be significantly smaller than 16ms (screen refresh rate).

pjcozzi commented 8 years ago

As @javagl said, an offline epsilon could cause visual artifacts. With time[n] == time[n + 1] client uses the first output value as an interpolation target, while jumping instantly to the second as time comes.

Ah, I see. That is OK with me.

lexaknyazev commented 8 years ago

Quick summary before PR:

Any comments?

RemiArnaud commented 8 years ago

Still makes no sense. what is the value at t=5 ?

Is it 2? Is it 10 ?

How can you make sur that different implementations do the same thing? Much more rules need to be established if you allow several values at the same time stamp.

RemiArnaud commented 8 years ago

Here's a rule I think would accomplish your goal and be disambiguous.

Following rules must be true : for all n<count-2 : T(n) <= T(n+1) < T(n+2) T(0) < T(1)

If T(n) < T(n+1) intervals are defined as: [T(n),T(n+1)T(n+1)],T(n+2)[

If T(n) = T(n+1) intervals are defined as: [T(n-1), T(n)[ and [T(n),T(n+2)[

So now one can say v=10 for t=5 in example above.

If T(n) > T(n

javagl commented 8 years ago

These rules seem to cover the constraints pretty well. Admittedly, from just reading them, I'd still find it hard to derive an implementation, although I think it boils down to a simple algorithm: For a given time t, the start index of the interpolation interval is the largest index for which times[index] <= t. The end index is index+1. (There are subtleties involved. Particularly the case where the index already is the last index, and/or t > max(times). This is also when the https://github.com/KhronosGroup/glTF/issues/573 kicks in. But maybe this can be covered elegantly by some modulo computation on the indices or the time t)

pjcozzi commented 8 years ago

@lexaknyazev these are OK with me: https://github.com/KhronosGroup/glTF/issues/704#issuecomment-246082807

As for T(n) <= T(n+1) < T(n+2), I am open to whatever @lexaknyazev @RemiArnaud @javagl conclude, but I will also put forward the idea that if we require strictly increasing times, it will be easy to relax that in future glTF versions; whereas if we allow equal adjacent times, but come up with awkward behavior, that will be harder to take back.

lexaknyazev commented 8 years ago

The whole T(n) == T(n+1) case is needed to enable representation of:

  1. animation, that has some jumps
  2. animation, that has only jumps:

    T(0) < T(1) == T(2) < T(3) == T(4) with V(0) == V(1); V(2) == V(3)

First case could be refactored to several animations, for the second case we could allow STEP interpolation (and reduce data size in 2, btw). Maybe, such solution will be easier to implement than handling "special case".

javagl commented 8 years ago

The STEP may already have been a reference to this, but ... I just thought: "How has this been solved before?". So I had a look at https://www.khronos.org/files/collada_spec_1_5.pdf . They are going far beyond what can currently be represented in glTF (with slightly different concepts), but a possibly relevant statement at page 158 is

Animations are limited to monotonic curves in the key axis. In other words, animation keys need to be sorted in increasing order of INPUT and cannot be duplicated.

If I understood this correctly, then there will hardly be a tool that writes duplicate times, and if it does, such a file likely could not properly pass through the COLLADA layer. So it might be OK to keep it simple and require > for 1.0.1. As Alexey said, the case of a single jump can probably be emulated with two distinct animations, and the >= case could be tackled later, as one of (many!) other possible extensions (STEP interpolation, splines, looping behavior...).

lexaknyazev commented 8 years ago

I would prefer having strict > and STEP, so we don't reduce functionality from 1.0.0 level.

lexaknyazev commented 8 years ago

Please review language in #712. Equal times are still unaddressed. I'll update readme (also with more examples) as soon as we agree on everything here.

javagl commented 8 years ago

Looks fine for me.

("Different channels must have different targets": One could now start (or, admittedly: continue) nitpicking and arguing whether this refers to a single animation, or globally to all animations, or how much flexibility should be allowed here. As far as I see, right now, there's nothing to prevent two animations from animating the same node property - and in fact, that would be OK, as long as the time spans that are covered by the inputs of every animation channel target are disjoint over all animations...)

lexaknyazev commented 8 years ago

Same targets within one animation must be definitely forbidden. Same targets for different animations could be possible even if they cover the same time spans, since nothing requires playing all animations at once (maybe we need to elaborate a bit more here).

pjcozzi commented 8 years ago

Same targets within one animation must be definitely forbidden. Same targets for different animations could be possible even if they cover the same time spans, since nothing requires playing all animations at once (maybe we need to elaborate a bit more here).

@lexaknyazev could you please make this more explicit as part of #712?

pjcozzi commented 8 years ago

I would prefer having strict > and STEP, so we don't reduce functionality from 1.0.0 level.

Strict > is OK with me. As for STEP, do we know if any WebGL engines actually implemented the special case? I don't think Cesium did. If none of them did, perhaps we should hold off on STEP.

lexaknyazev commented 8 years ago

Same targets within one animation must be definitely forbidden. Same targets for different animations could be possible even if they cover the same time spans, since nothing requires playing all animations at once (maybe we need to elaborate a bit more here).

@lexaknyazev could you please make this more explicit as part of #712?

Do we have any implementation guideline on animations start (such as "always play everything", or "play only on-demand")?

pjcozzi commented 8 years ago

Do we have any implementation guideline on animations start (such as "always play everything", or "play only on-demand")?

Do you think that is something glTF can explicitly define? I think it is too dependent on the model to make a general rule.

lexaknyazev commented 8 years ago

If we allow overlapping keyframe inputs with the same target, it's inevitable. Otherwise we can state that behavior is undefined in such case.

lexaknyazev commented 8 years ago

As for STEP, do we know if any WebGL engines actually implemented the special case?

I'm not 100% sure, but looks like three.js is one of them: https://github.com/mrdoob/three.js/blob/master/src/math/interpolants/DiscreteInterpolant.js

pjcozzi commented 8 years ago

As for STEP, do we know if any WebGL engines actually implemented the special case? I'm not 100% sure, but looks like three.js is one of them: https://github.com/mrdoob/three.js/blob/master/src/math/interpolants/DiscreteInterpolant.js

@tparisi can you confirm

@lexaknyazev or can we test with a sample model?

pjcozzi commented 8 years ago

If we allow overlapping keyframe inputs with the same target, it's inevitable. Otherwise we can state that behavior is undefined in such case.

Is this related to auto start all animations or not? Seems like a separate issue that we could disallow.

lexaknyazev commented 8 years ago

Is this related to auto start all animations or not? Seems like a separate issue that we could disallow.

Let's say we have two animations with the same target and overlapping (or even exact) timeranges. There is no right answer for runtime what to do with them.

Since we allow different inputs, there is no need in multiple animation objects for simple usecases. So to keep spec simple, we can state that if an asset contains only one animation object, it could be started automatically. In case of multiple animation objects runtime behavior is undefined.