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

Do not interrupt scripted animations #1749

Merged
merged 15 commits into from Jun 19, 2018

Conversation

Projects
None yet
6 participants
@akortunov
Copy link
Collaborator

commented Jun 11, 2018

This PR is aimed to fix bug #3486, bug #4286, bug #4307 and bug #4291 since all these problems are related.

Tested with NPC_Commands_V8b.esp, Abot's Silt Striders, and Castle Hestatur mannequins.

Main changes:

  1. Scripted animations have higher priority than common ones, so common animations should not interrupt them.
    Allows such mods as Abot Silt Striders and NPC_Commands to work better.

  2. Cell cleanup checks if death animation is finished before remove the corpse 72 hours after death.

  3. If an actor has Health = 0 in editor, but does not have the Persistent flag, play death animation.
    These two allows mannequins from Castle Hestatur work properly.

  4. Scripted animations ignore SkipAnim console command.

  5. "PlayGroup idle" removes scripted animations from actor and resumes a common NPC behaviour.
    These two should fix other mannequin mods, which use scripted anims on dead actors.

Should work now, but I suppose that this PR has some side effects.

@Capostrophic

This comment has been minimized.

Copy link
Collaborator

commented Jun 11, 2018

what was the culprit with mannequins (bug #4307)?

Can't say, sorry. My usual blunt approach (>_>) didn't work for such a nuanced situation and the mannequins played death animation in result when they shouldn't.

@akortunov

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 11, 2018

Some additional changes:

  1. Skip death animations only for actors with Persistent flag.
  2. Do not interrupt current animation by death animation, if the current animation is persistent (scripted).

Now we should create a list of cases to test.
@Capostrophic, any suggestions? Which mannequin mods did you try to use?

@akortunov

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 11, 2018

About mannequins in Castle Hestatur: OpenMW for now does not check if death animation is finished before delete the corpse. Also isDeathAnimationFinished() returns true for any actor with Health = 0 at start.
Mostly works now, but mannequins appear laying on ground because of 0 Fatigue - they are just knocked out and using "knockout death" animation. Not sure how to fix it properly.
Probably we can do not update animations for [initially] dead actors.

@akortunov

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 11, 2018

For now I do not update animation state for dead actors.

@akortunov

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 12, 2018

The only things left are to make PlayGroup to work for corpses (bug #4286) and to determine which parts of animation update should be skipped when scripted animation is playing (bug #4367). In vanilla PlayGroup is bugged for NPCs (for example, they still try to attack).

@akortunov

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 12, 2018

make PlayGroup to work for corpses

It seems PlayGroup in vanilla has higher priority than SkipAnim.

@psi29a

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

That and changelog entries, you're on fire! ;)

@akortunov

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 12, 2018

and to determine which parts of animation update should be skipped when scripted animation is playing (bug #4367).

Is not blocker since PlayGroup is bugged for NPCs in vanilla. We need a forum discussion to determine desired behaviour.

@psi29a

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

Is that for another PR then?

@akortunov

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 12, 2018

Is that for another PR then?

Yes, it is.

Another quirks to reproduce:

  1. Looks like PlayGroup ignores SkipAnim, and Morrowind plays only scripted animation in this case.
    For example: you have a mannequin (an initially dead actor with simple SkipAnim script).
    You use the PlayGroup on him, and he starts to play scripted animation, despite of SkipAnim. GetDeadCount still returns 0.

  2. You use "sethealth 0" for living actor, wait for start of death animation, use the PlayGroup on him.
    He starts to play new animation, GetDeadCount returns old count.
    If you use "PlayGroup idle" for him, he plays death animation and GetDeadCount returns old value + 1.

@akortunov

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 12, 2018

About Abot's Silt Striders: I encountered a lot of twitching during scenic travel due to "walkforward" animation in both vanilla and OpenMW (a bit more noticable).
Is it supposed to be, or I just do something wrong?

EDIT: looks like it is not only my problem - on some videos there is a twitching too.

@akortunov

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 12, 2018

Highly likely that the cause of twitching is movement in the WalkForward animation.
But abot denies it:

I think walk/run animations need to move along axis (maybe it is needed for collision detection to work).
I recall before a Morrowind Code Patch "Slow movement anim fix" you had to even have movement faster then 1/unit sec else it was giving "animation in place" warnings with e.g. snail creatures
[EDIT]it's been too much time, I don't remember things...
I checked strider nif in NIFSkope/CS animation window and I think horizontal movement/vertical animation bobbing are already 0 (only things moving are legs) so it should be as smooth as scripting possible
movement while scenic travelling is vertically bumpy because even using fly magic effect to damp it the strider mesh collides with terrain level at high speed, to make it smoother you would have to store/manage Z coordinate of waypoints, it's already hard enough managing X/Y coordinates

I'l try to investigate and check if the issue is on my side.

@akortunov akortunov changed the title [Do not merge yet] Do not interrupt scripted animations Do not interrupt scripted animations Jun 14, 2018

@akortunov

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 14, 2018

I think this PR is ready for review now.

About Silt Striders issue: runAnimation() returns a forward movement for WalkForward animation (a quite expected behaviour).
This movement conflicts with movement from SetPos console command for some reason and produces a lot of jittering during scenic travel.
I will create a separate bugreport after merge.


mAnimation->disable(mCurrentIdle);
mCurrentIdle.clear();

mIdleState = CharState_SpecialIdle;
bool loopfallback = (entry.mGroup.compare(0,4,"idle") == 0);
mAnimation->play(groupname, Priority_Default,
mAnimation->play(groupname, persist && groupname != "idle" ? Priority_Persistent : Priority_Default,

This comment has been minimized.

Copy link
@psi29a

psi29a Jun 14, 2018

Member

for the sake of readability/maintainability, can we do assignment outside of a function call and just pass the resulting variable through?

@psi29a

This comment has been minimized.

Copy link
Member

commented Jun 14, 2018

A four in one bug squasher! Aside from one comment, I see no deal breakers.

@zinnschlag, @kcat or @AnyOldName3 : any thoughts

@AnyOldName3

This comment has been minimized.

Copy link
Member

commented Jun 14, 2018

I don't know enough about this part of the engine to have thoughts.

@kcat

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2018

This Priority_Persistent priority honestly feels a bit hacky to me. All the other priorities are named for their use (holding a torch, death, etc), but Priority_Persistent says nothing about what and when it's actually used for. It looks like it doesn't know whether it wants to be the highest priority or a flag, and is special-cased all over making it hard to know what'll happen in various parts of the code.

Now, I've never been very happy with the animation state code, even as I was writing it (I didn't/couldn't put in the necessary effort to making something that made sense as a system, versus something that just worked vaguely like vanilla), so maybe this is the best option with what's there. But the animation state handling can do with some serious cleaning and refactoring.

@zinnschlag zinnschlag merged commit bce6d79 into OpenMW:master Jun 19, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

zinnschlag added a commit that referenced this pull request Jun 19, 2018

@akortunov akortunov deleted the akortunov:scriptedanims branch Aug 26, 2018

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