godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.04k stars 21.18k forks source link

Collada importer introduces pops/hitches in animation due to interpolation bug #31005

Open jitspoe opened 5 years ago

jitspoe commented 5 years ago

Godot version: 3.1.1

OS/device including version: Windows 8.1

Issue description: There appear to be 2 issues with the importer: 1) Even if the framerate in Blender and the framerate specified on the import are the same, godot attempts to interpolate some frames. 2) When frames are interpolated, bones get incorrect transforms causing pops in the animation, even on bones that originally had no animation. I noticed this on my character when just animating the hands and the feet popped on a certain frame.

For the longest time, I thought this was an issue with the Better Collada exporter, but I dug through that for a while before finally ruling it out. Then I noticed a behavior change when changing the import framerate in Godot. The pops in the animation shifted to a different time.

In collada.cpp, I discovered something that might be suspect:

            float c = (p_time - keys[i - 1].time) / (keys[i].time - keys[i - 1].time);

            if (keys[i].data.size() == 16) {
                //interpolate a matrix
                Transform src = _read_transform_from_array(keys[i - 1].data);
                Transform dst = _read_transform_from_array(keys[i].data);

                Transform interp = c < 0.001 ? src : src.interpolate_with(dst, c);

First thing I attempted to do was snap to the nearest frame:

            // Quick hack to try snapping to the nearest time
            if (p_time - keys[i-1].time > keys[i].time - p_time)
            {
                c = 1.0;
            }
            else
            {
                c = 0.0;
            }

Strangely, this did not fix the issue. I figured that would cause interpolate_with to use either 100% of the first matrix or 100% of the second.

Finally I just removed the interpolate_with entirely:

                Transform interp = src;

No more animation pops. Not sure if there's a fundamental problem with interpolate_with or if it's being used improperly here, but this could be a pretty core bug.

Steps to reproduce:

I'll have to see if I can create a simpler repro case as my character .dae file is gigantic (very bloated format).

jitspoe commented 4 years ago

Looks like this is a generic bug with interpolate_with(). I noticed rotations breaking when using interpolation with SkeletonIK as well, which also uses the interpolate_with() function. Guess I need to create a simple repro case.

fire commented 4 years ago

If you can help create a reproduction case, that'll help debug this!

jitspoe commented 4 years ago

Hmm, tried for a bit to make something with just interpolate_with() but couldn't repro the issue. Perhaps the issue is not with interpolate_with, but instead how it's being used within a skeleton on both IK and the collada importer. I'll have to look into it more later.

jitspoe commented 4 years ago

I attempted to make a simple collada file to reproduce this issue and stumbled across this interesting piece of code:

        if (nm.anim_tracks.size() == 1) {
            //use snapshot keys from anim track instead, because this was most likely exported baked
            const Collada::AnimationTrack &at = collada.state.animation_tracks[nm.anim_tracks.front()->get()];
            snapshots.clear();
            for (int i = 0; i < at.keys.size(); i++)
                snapshots.push_back(at.keys[i].time);
        }

If there's exactly 1 animation, it ignores the import settings for the framerate and, instead, uses the exact frame times from the exported file.

This makes me wonder: Why isn't this just the default behavior? Why would you want to import at a different framerate than you exported? This is just going to muddy the animation if your export and import framerate don't match. Could be very problematic for snappy, exaggerated poses, like attacks. I guess it could be an override option if, for some reason, you wanted this, but since everything is time-based anyway, I'd imagine you'd want keyframes to be the same as they are in your animation software. Haven't dug into the other importers, but I think they all have the framerate option. Maybe this should default to 0 (use exported framerate) with the option to override?

I did identify why the interpolate was happening at all, even though the exported animation and the import options were both set at 60:

    for (i = 0; i < keys.size(); i++) {

        if (keys[i].time > p_time)
            break;
    }
...
            float c = (p_time - keys[i - 1].time) / (keys[i].time - keys[i - 1].time);
...
                Transform interp = c < 0.001 ? src : src.interpolate_with(dst, c);

c sometimes ends up being 0.999... because of some slight precision issues. Could probably snap to the dest if c > 0.999 or maybe change if statement in the loop to find the key index to something like if (keys[i].time - 0.001 > p_time)

As for the actual interpolation bug, I suspect it MIGHT be due to this:

                Transform interp = c < 0.001 ? src : src.interpolate_with(dst, c);

                Vector<float> ret;
                ret.resize(16);
                Transform tr;
                // i wonder why collada matrices are transposed, given that's opposed to opengl..
                ret.write[0] = interp.basis.elements[0][0];
                ret.write[1] = interp.basis.elements[0][1];
                ret.write[2] = interp.basis.elements[0][2];
                ret.write[4] = interp.basis.elements[1][0];
                ret.write[5] = interp.basis.elements[1][1];
                ret.write[6] = interp.basis.elements[1][2];
                ret.write[8] = interp.basis.elements[2][0];
                ret.write[9] = interp.basis.elements[2][1];
                ret.write[10] = interp.basis.elements[2][2];
                ret.write[3] = interp.origin.x;
                ret.write[7] = interp.origin.y;
                ret.write[11] = interp.origin.z;
                ret.write[12] = 0;
                ret.write[13] = 0;
                ret.write[14] = 0;
                ret.write[15] = 1;

I'm not sure what happens if you try to interpolate a matrix in whatever order it is in collada, then transpose it. Possibly a red herring, but I don't see any obvious issues with the interpolate_with function itself (and couldn't reproduce any issues with it by itself). Unfortunately, I can't seem to get a simple repro case of this, and the animations I had that were problematic in the past have changed and no longer exhibit the issue, so I'll have to dig through source control or something to get a repro case (or maybe it's somehow been fixed?)

Regarding the IK thing, it wasn't the interpolation itself that was the problem. It was the bone AFTER the IK chain that went wonky. Enabling the "Override Tip Basis" with an appropriately oriented target matrix seems to have fixed the issue. Seems there may have been 2 different bugs with the only 2 places interpolate_with was used, making it look very suspect, but I don't think that's where the bug resides.

KoBeWi commented 3 years ago

Can anyone still reproduce this bug in Godot 3.2.3 or any later release?

If yes, please ensure that an up-to-date Minimal Reproduction Project (MRP) is included in this report (a MRP is a zipped Godot project with the minimal elements necessary to reliably trigger the bug). You can upload ZIP files in an issue comment with a drag and drop.

jitspoe commented 3 years ago

I can still reproduce it in 3.2.3.

Here's an example file with 2 animations. One with the issue and one without. Notice the slight movement of the feet and hands, even though the original animation (blend file included) has NO animation at all.

Test Collada Import.zip Note: Files are provided just for the purpose of reproducing the bug and not to be used in other projects without explicit permission. Other notes: When importing, if you try it with a new file, the framerate should be set to 60 (not visible at the default 15fps).

I should make a PR to fix this, but my code is kind of a mess right now. I have some changes to import animations using the framerate they're exported at by default, which avoids the interpolation issue entirely. Still might be worth having somebody else take a peek at it as it may be a fundamental math issue with interpolation that will impact more than just this.

KoBeWi commented 3 years ago

Tried this in 4d9b95f Seems like the skeleton still moves, but it doesn't affect the model. OquRzlK8jM (in 3.2.4 beta4 the model also moves)

jitspoe commented 3 years ago

Weird. Not sure why the mesh isn't updating there, but the skeleton animation is the part that matters.

fire commented 3 years ago

As far as I know collada broke completely. The TPS player is also broken. On vacation so I would welcome any help.

On Dec 24, 2020, at 1:50 PM, jitspoe notifications@github.com wrote:

 Weird. Not sure why the mesh isn't updating there, but the skeleton animation is the part that matters.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

fire commented 3 years ago

fotf_character2zy.dae crashes godot engine on 96410f55b24e47af045e3ad31545331ce124d999

fire commented 1 year ago

The animation still does not play.

Screenshot 2023-02-21 171352
fire commented 1 year ago

I was unable play the animation in blender 3.4.1

image

jitspoe commented 1 year ago

The .blend file I provided actually has no animation (just a single pose). It was used to highlight the bug in the importer that resulted in things moving even when there was no animation.

akien-mga commented 1 year ago

And can you still reproduce the bug on 4.0 RC 3, or is it fixed?