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

Merge repair and rearm anim traits into WithResupplyAnimation #15543

Merged
merged 1 commit into from Sep 27, 2018

Conversation

Projects
None yet
3 participants
@reaperrr
Copy link
Contributor

reaperrr commented Aug 19, 2018

Another (and definitely the last) split from #14357.

This is the safest approach to avoid animation conflicts/visual glitches when the 'host' is responsible for both resupply types.
The new WithResupplyAnimation trait will simply play a looping animation as long as the actor is resupplying in any form.

As a byproduct, this makes the repair/reload animations look more like in the originals and less erratic.
We don't have a WithRearmOverlay yet, so I'll leave a refactor of WithRepairOverlay for later.

@reaperrr reaperrr referenced this pull request Aug 19, 2018

Merged

Add Rearmable trait #14357

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Sep 15, 2018

@Mailaender regarding your comment on IRC, I'll add some context:

This was split from a larger PR.

Basically, the ultimate goal of this PR, #14357 which it was split from and the PR(s) that will follow, is to allow actors to be reloaded and repaired at the same time (right now on bleed, actors that need both will be rearmed first, then repaired, as both are separate activities that are queued in Repairable).

The problem is that on bleed, the reload/repair animation (of the building) is played once on every repair/reload step, instead of just looping continuously.
So if repairing and rearming happened in parallel, depending on their interval it's possible that a repair step happens just a tick after a rearm step, resulting in restarting the service depot animation before the last one finished.
This is also already an issue with rearming or repairing when the resupply interval is shorter than the animation, as in that case it restarts prematurely as well.

This PR simply makes the resupply animations of helipads, service depots and so on loop at consistent speed as long as the building is either repairing or rearming (or both) some actor.

By the way, this also makes the animations look more like in the original, where the animation speed wasn't tied to the repair/reload speed, either, making it look less hectic.

@Mailaender

This comment has been minimized.

Copy link
Member

Mailaender commented Sep 16, 2018

If a vehicle gets killed while repairing/rearming the animation continues:

image

I don't know if that is a regression or already existing.

PS: Looking at the old code, there was no check for dead actors either.

@Mailaender
Copy link
Member

Mailaender left a comment

Rearming and repairing still works and the animation plays.

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Sep 16, 2018

If a vehicle gets killed while repairing/rearming the animation continues

Should be an easy fix, if that's the case I might as well include it here.

reaperrr added a commit to reaperrr/OpenRA that referenced this pull request Sep 17, 2018

Fix Activity.OnLastRun being skipped when actor dies
It appears that if an actor dies while an activity is active, it will be disposed before the activities' TickOuter can tick one last time, so OnLastRun is never triggered.
As OpenRA#15543 showed this can lead to some nasty bugs.
This commit makes Activity.Cancel run OnLastRun directly if the actor IsDead when the cancel is triggered.

reaperrr added a commit to reaperrr/OpenRA that referenced this pull request Sep 17, 2018

Fix Activity.OnLastRun being skipped when actor dies
It appears that if an actor dies while an activity is active, it will be disposed before the activities' TickOuter can tick one last time, so OnLastRun is never triggered.
As OpenRA#15543 showed this can lead to some nasty bugs.
This commit enforces a proper activity end on actor death/disposal.

@reaperrr reaperrr force-pushed the reaperrr:merge-resupply-anims branch from 9c25fdb to 799fbf2 Sep 17, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Sep 17, 2018

Rebased onto #15622 and fixed the above issue.

@reaperrr reaperrr force-pushed the reaperrr:merge-resupply-anims branch 2 times, most recently from 7c541e6 to 55ee19d Sep 20, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Sep 20, 2018

Rebased onto updated #15622 and fixed that ResupplyAircraft only notified INotifyRepair (unconditionally).
It now checks which of the (Child-)activities was running on disposal and then notifies the correct interface accordingly.

@reaperrr reaperrr force-pushed the reaperrr:merge-resupply-anims branch from 55ee19d to 00d4cbd Sep 25, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Sep 25, 2018

Rebased and updated again.
The update only affects Rearm activity though, it now properly gets the host passed like Repair instead of relying on the old ActorsAt hacks.

According to my own tests everything still works as it should, but I guess there have been too many changes since @Mailaender's review to keep his +1.

@reaperrr reaperrr removed the PR: Needs +2 label Sep 25, 2018

@@ -33,39 +37,60 @@ protected override void OnFirstRun(Actor self)
// HACK: this really shouldn't be managed from here
foreach (var pool in ammoPools)
pool.RemainingTicks = pool.Info.ReloadDelay;

if (host.Type == TargetType.Invalid)

This comment has been minimized.

@reaperrr

reaperrr Sep 25, 2018

Contributor

Note for reviewers: Target.Type returns Invalid if the actor is null, dead or not in world, which is what I would've checked here anyway.

@abcdefg30
Copy link
Member

abcdefg30 left a comment

Looks fine otherwise and ingame. 👍
Do we want to move the update rule into a folder like in #14664?


namespace OpenRA.Mods.Common.Traits.Render
{
// Possible future addition: Refuel

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 26, 2018

Member

I'd vote to remove this comment, it doesn't have any value for the code imho.

{
None = 0,
Rearm = 1,
Repair = 2,

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 26, 2018

Member

Redundant comma.

Show resolved Hide resolved OpenRA.Mods.Common/Traits/Render/WithResupplyAnimation.cs

@reaperrr reaperrr force-pushed the reaperrr:merge-resupply-anims branch 2 times, most recently from 17cb7f8 to af894e1 Sep 26, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Sep 26, 2018

Updated.

Do we want to move the update rule into a folder like in #14664?

Done.

Merge repair and rearm anim traits into WithResupplyAnimation
This is the safest approach to avoid conflicts/visual glitches when the host is responsible for both resupply types.
The new trait will simply play a looping animation as long as the actor is resupplying in any form.

@reaperrr reaperrr force-pushed the reaperrr:merge-resupply-anims branch from af894e1 to 11d39b0 Sep 27, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Sep 27, 2018

Rebased.

@abcdefg30 abcdefg30 merged commit 8144fca into OpenRA:bleed Sep 27, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Member

abcdefg30 commented Sep 27, 2018

@reaperrr reaperrr deleted the reaperrr:merge-resupply-anims branch Oct 26, 2018

@pchote pchote referenced this pull request Nov 19, 2018

Merged

Fix Repairable crash #15822

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment