Page MenuHomeWildfire Games

Simplify conditions in SetAnimation
ClosedPublic

Authored by Silier on Nov 11 2019, 6:03 PM.

Details

Reviewers
Stan
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP23495: Simplify conditions in SetAnimation
Summary

Introduced in rP3653.
There are 2 mane cases we cannot set animation.
1st there are not bones
2nd animation is not valid

I am not sure what will currently happen if we do not have bones and we do not have valid animation. In that case no one from these condition is true so invalid animation is going to be set ?

That being said, these conditions can be merged into one with one bonus.
We do not have valid animation and bones, we should fail.

Test Plan

Check logic is correct.

Event Timeline

Silier created this revision.Nov 11 2019, 6:03 PM
Owners added a subscriber: Restricted Owners Package.Nov 11 2019, 6:03 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/561/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1075/display/redirect

Silier added a reviewer: Restricted Owners Package.Nov 17 2019, 5:49 PM
Stan added a subscriber: Stan.Dec 9 2019, 3:33 PM
Stan added inline comments.
source/graphics/Model.cpp
462–463

I think the proper word here is "rigged".

468

Needs rebase after rP23225

485

Why do we set anim if !anim?

Silier planned changes to this revision.Dec 9 2019, 3:34 PM

rebase

Silier edited the summary of this revision. (Show Details)Dec 9 2019, 3:47 PM
Silier added inline comments.Dec 9 2019, 3:55 PM
source/graphics/Model.cpp
485

You would need to ask philip rP1920, probably should be inside the if, but it changes nothing if condition is false so does not matter

Vulcan added a comment.Dec 9 2019, 8:36 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/724/display/redirect

Vulcan added a comment.Dec 9 2019, 8:42 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1240/display/redirect

Stan requested changes to this revision.Dec 21 2019, 2:18 PM

According to this the check simplification is wrong.

var a = {};
var b = undefined;

console.log((!!a && !b) || (!a && !!b)); // true
console.log(!a && !b) // false
This revision now requires changes to proceed.Dec 21 2019, 2:18 PM

but i have || no & & :)

Stan added a comment.EditedDec 21 2019, 2:52 PM
var a = undefined;
var b = undefined;

console.log((!!a && !b) || (!a && !!b)); // false
console.log(!a ||!b) // true

yes, that is the point:

There are 2 mane cases we cannot set animation.
1st there are not bones
2nd animation is not valid

Or do you know about usage when we can animate without bones and without valid animation?

Silier requested review of this revision.Dec 21 2019, 3:03 PM
Stan added a comment.Dec 21 2019, 3:05 PM

Ah yes you are right! I forgot

What about moving

if (!m_BoneMatrices)
 return false;

to line 453 in that case?

I wonder if we could early return if there is no anim

Where is m_BoneMatrices used? Somewhere in the function?

if (!anim || !anim->m_AnimDef)
 return false;

Can you try to edit the following file
binaries\data\mods\public\art\actors\structures\athenians\civil_centre.xml
and replace

<animation event="0.5" load="0.0" name="attack_ranged" speed="100"/>

by

<animation event="0.5" file="quadraped/bovidae_death_a.dae" load="0.0" name="attack_ranged" speed="100"/>

Civic_center should have no bone matrices :)

source/graphics/Model.cpp
453

We just need to make sure that's set. nullptr?

468

I guess nuke?

What about moving

if (!m_BoneMatrices)
return false;
to line 453 in that case?

That would be not the same.

if (!anim || !anim->m_AnimDef)
return false;

No because !anim now returns true so again not the same.

Silier updated this revision to Diff 10802.Dec 27 2019, 3:21 PM

nullptr, nuke comment

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/898/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1414/display/redirect

Stan accepted this revision.Dec 27 2019, 3:45 PM

Looks good. I assume we tested all the possible cases with the static mesh. Maybe had a capital to the commen near the nullptr, put it on top, and add a final dot.

This revision is now accepted and ready to land.Dec 27 2019, 3:45 PM

What was in case !m_BoneMatrices && !anim->m_AnimDef before the patch? It seems it returns true.

Silier added a comment.Jan 9 2020, 6:08 PM

setting invalid animation to entity without bones. Failed to find use case for that one and logical reasoning. Do you know why that would be usefull?

In D2416#106689, @Angen wrote:

Do you know why that would be usefull?

Nope, just wondering.

You might temporarily add ENSURE and test few models with different animations.

This revision was automatically updated to reflect the committed changes.