Page MenuHomeWildfire Games

Allow to play upgrade animations
ClosedPublic

Authored by Stan on Oct 13 2019, 1:37 PM.

Details

Reviewers
Angen
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP23185: Allow artists to add upgrade animations.
Summary

Currently when you upgrade a building or a unit there is no animation. This patch fixes that

Test Plan

Choose the Cart civ, build a long wall upgrade it to a gate, notice it now have scaffoldings and dust. If it had rigged animation it would also play the anim specified in the variant.

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

There are a very large number of changes, so older changes are hidden. Show Older Changes

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

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

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

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

Angen requested changes to this revision.Nov 22 2019, 8:14 AM
Angen added a subscriber: Angen.

Description says, it is setting variant, but code is selecting animation.
So it should call SetVariant instead :)

One needs to keep also in mind what will happen if one cancel upgrade.

This revision now requires changes to proceed.Nov 22 2019, 8:14 AM
Stan updated this revision to Diff 10376.Nov 22 2019, 10:44 AM

Reset the upgrade animation when the upgrade is cancelled.

As for the other point cmpVisual.SelectAnimation() calls cmpVisual.SetVariant() so no need for the later as we also want to play specific anims.

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

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

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Upgrade.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Upgrade.js
| 268| 268| 	this.upgrading = false;
| 269| 269| 	this.CancelTimer();
| 270| 270| 	this.SetElapsedTime(0);
| 271|    |-	
|    | 271|+
| 272| 272| 	let cmpVisual = Engine.QueryInterface(this.entity, IID_Visual);
| 273| 273| 	if (!cmpVisual)
| 274| 274| 		return;
Executing section cli...

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

Stan updated this revision to Diff 10377.Nov 22 2019, 10:59 AM

Remove useless parameter.

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

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

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Upgrade.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Upgrade.js
| 268| 268| 	this.upgrading = false;
| 269| 269| 	this.CancelTimer();
| 270| 270| 	this.SetElapsedTime(0);
| 271|    |-	
|    | 271|+
| 272| 272| 	let cmpVisual = Engine.QueryInterface(this.entity, IID_Visual);
| 273| 273| 	if (!cmpVisual)
| 274| 274| 		return;
Executing section cli...

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

Stan updated this revision to Diff 10378.Nov 22 2019, 2:01 PM

Optimize stuff a bit.

Build failure - The Moirai have given mortals hearts that can endure.

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

Build failure - The Moirai have given mortals hearts that can endure.

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

Angen requested changes to this revision.Nov 22 2019, 3:07 PM
Angen added inline comments.
binaries/data/mods/public/simulation/components/Upgrade.js
270 ↗(On Diff #10378)

choice && this.template[choice].Variant

This revision now requires changes to proceed.Nov 22 2019, 3:07 PM
Stan updated this revision to Diff 10379.Nov 22 2019, 3:09 PM
Stan marked an inline comment as done.

Fix check. Add variants and template changes.

Build failure - The Moirai have given mortals hearts that can endure.

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

Build failure - The Moirai have given mortals hearts that can endure.

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

Polakrity added inline comments.
binaries/data/mods/public/simulation/components/Upgrade.js
313 ↗(On Diff #10378)

No more parameter.
You forgot to remove Jsdoc.

320 ↗(On Diff #10378)

merge the 2 conditions.

Stan updated this revision to Diff 10380.Nov 22 2019, 3:18 PM

Fix tests. Apply @Polakrity's suggestion.

Stan updated this revision to Diff 10381.Nov 22 2019, 3:19 PM

Remove jsdoc.

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

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

Stan marked 2 inline comments as done.Nov 22 2019, 3:19 PM

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

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

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

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

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

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

Angen requested changes to this revision.Nov 22 2019, 8:13 PM

Missing animations:
brit_sentry_tower
gaul_sentry_tower

cart_s_wall_long has size of wooden palisade, so should not have scaf

rome_siege_wall_long errors out when trying to place

This revision now requires changes to proceed.Nov 22 2019, 8:13 PM
Stan updated this revision to Diff 10383.Nov 23 2019, 12:44 AM

Add scaffolds for the two missing towers, fix wall. tweak the tower so roof doesn't get overriden.

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

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

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

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

Angen requested changes to this revision.EditedNov 23 2019, 12:30 PM

The feature looks good and it is nice visual update. Before accepting it I found two bigger problems and some smaller.
So this needs to be fixed:
Mace sentry and defence towers loose their windows, doors and other important props while in upgrade animations.
Rome defence tower is loosing also some wooden props which changes its overall look significantly.

Following is not required to be fixed but it would be nice to have it:
name of animation should start with lowercase letter to keep it consistant with others, mainly in atlas dropdown.
some structures have ground props like bushes, which should not disappear while in upgrade animation.
also some towers have visual prop of the path around which is too missing while in upgrade

This revision now requires changes to proceed.Nov 23 2019, 12:30 PM
Stan updated this revision to Diff 10392.Nov 23 2019, 3:36 PM

This should fix the remaining issues. let me know if it doesn't.

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

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

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

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

Stan planned changes to this revision.Nov 23 2019, 6:47 PM

Spoiler alert it didn't, I forgot to change the capitalization.

Angen added a comment.Nov 23 2019, 8:42 PM

Notes:
ptolemies/defense_tower: pls add <prop actor="props/structures/decals/spart_1x1.xml" attachpoint="root"/>
romans/scout_tower pls add <prop actor="props/structures/romans/rome_tower_wood.xml" attachpoint="root"/>
actually roman siege wall is about the size of palisade in height, maybe scaf should go off there too, I think it is a bit off by height and width

Stan added a comment.Nov 23 2019, 9:47 PM
In D2371#101720, @Angen wrote:

Notes:
ptolemies/defense_tower: pls add <prop actor="props/structures/decals/spart_1x1.xml" attachpoint="root"/>
romans/scout_tower pls add <prop actor="props/structures/romans/rome_tower_wood.xml" attachpoint="root"/>
actually roman siege wall is about the size of palisade in height, maybe scaf should go off there too, I think it is a bit off by height and width

Would a two story scaffolding work ?

Angen added a comment.Nov 23 2019, 9:51 PM

I think it would need to go closer a bit too to look good. Now there is a pretty big gap.

Stan updated this revision to Diff 10399.Nov 24 2019, 12:54 AM

Fix ptol and rome towers. Remove scaffolds from rome siege wall. Fix missing props when upgrading. Upgrading → upgrading.

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

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

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

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

Angen requested changes to this revision.Nov 24 2019, 11:15 AM

one more thing aside inline before ok, I just noticed that cart defense tower should not use specific cart scaf because it has its own stairs and they are doubled, looks weird.


binaries/data/mods/public/art/actors/structures/romans/scout_tower.xml
24 ↗(On Diff #10399)

keep that things here for ivy, because attachpoint="root" is resenting root props

This revision now requires changes to proceed.Nov 24 2019, 11:15 AM
Stan updated this revision to Diff 10402.Nov 24 2019, 11:55 AM

Fix the rome_tower and use the default scaf for the carth tower.

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

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

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

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

Angen accepted this revision.Nov 24 2019, 12:19 PM
This revision is now accepted and ready to land.Nov 24 2019, 12:19 PM
This revision was automatically updated to reflect the committed changes.

Looks good in-game. Great job.

Perhaps extend this to add a progress bar when selected (just below the health bar)? Perhaps use the same gray/white one for unpacking.