Page MenuHomeWildfire Games

Simplify conditions in SetAnimation
ClosedPublic

Authored by Angen 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.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Angen 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

Angen 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 ↗(On Diff #10302)

I think the proper word here is "rigged".

470 ↗(On Diff #10302)

Needs rebase after rP23225

484 ↗(On Diff #10302)

Why do we set anim if !anim?

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

rebase

Angen edited the summary of this revision. (Show Details)Dec 9 2019, 3:47 PM
Angen added inline comments.Dec 9 2019, 3:55 PM
source/graphics/Model.cpp
484 ↗(On Diff #10302)

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

Angen updated this revision to Diff 10547.Dec 9 2019, 8:32 PM

rebase

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
Angen added a comment.Dec 21 2019, 2:28 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
Angen added a comment.Dec 21 2019, 2:56 PM

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?

Angen 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 ↗(On Diff #10547)

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

468 ↗(On Diff #10547)

I guess nuke?

Angen added a comment.Dec 27 2019, 3:13 PM

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.

Angen 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.

Angen 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.