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

Remove SmokeTrailWhenDamaged #19152

Merged
merged 1 commit into from
Mar 27, 2021
Merged

Conversation

reaperrr
Copy link
Contributor

@reaperrr reaperrr commented Feb 12, 2021

One of the most outdated and limited traits remaining, which can do nothing LeavesTrails doesn't cover by now.
Had this on my agenda for quite a while, just never found the motivation to deal with the update rule until now.

All yaml changes done via update rule.
The two edge cases not covered by our mods (explicit Sprite definition, as well as update message when a MinDamage of for example Medium was defined), have been tested and confirmed to work locally.

Closes #10864.
Closes #19104.

Copy link
Member

@pchote pchote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine overall, just a couple of style nits

@reaperrr
Copy link
Contributor Author

Updated.

I decided to add TrailWhenStationary: true via update rule to replicate bleed behaviour, but only manually added it to the 3 RA attack helis and their husks instead of running the update rule on both affected mods again.

@pchote
Copy link
Member

pchote commented Mar 14, 2021

The smoke animation for stationary helicopters still doesn't look very good IMO (now a static blob of grey pixels, still worse than the SmokeTrailWhenDamaged behaviour).

I have two ideas on how we could improve this:

  • Change helicopters to use WithDamageOverlay (which uses the smoke_m animation). They move slow enough that the lack of trailing doesn't look quite so bad.
  • Remove the smoke trails from aircraft completely (I just checked to confirm that the original game did not have them).

We could drop TrailWhenStationary again then.
IMO the first would be better than the second.

@Smittytron
Copy link
Member

Would this close #10864?

@reaperrr
Copy link
Contributor Author

I think so.

@reaperrr
Copy link
Contributor Author

The smoke animation for stationary helicopters still doesn't look very good IMO (now a static blob of grey pixels, still worse than the SmokeTrailWhenDamaged behaviour).

I just forgot to set StationaryInterval as well, updated and now looks like on bleed while not moving, too.

Change helicopters to use WithDamageOverlay (which uses the smoke_m animation). They move slow enough that the lack of trailing doesn't look quite so bad.

On bleed the duration of smoke_m is fairly short, we need #19240 to do that properly. In my opinion we should stick to reproducing bleed here and get both PRs merged, RA/TD need another PR dedicated to polishing things up in terms of smoke offsets and durations anyway, once the plumbing is there.

Smittytron
Smittytron previously approved these changes Mar 20, 2021
Copy link
Member

@Smittytron Smittytron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Needs rebase.

@teinarss
Copy link
Contributor

@pchote anything before to add before I merge this?

One of the most outdated and limited traits remaining,
which can do nothing LeavesTrails doesn't cover by now.
@reaperrr
Copy link
Contributor Author

Rebased again.

@pchote
Copy link
Member

pchote commented Mar 27, 2021

@pchote anything before to add before I merge this?

Nope.

@pchote pchote merged commit b8e64df into OpenRA:bleed Mar 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SmokeTrailWhenDamaged should allow for multiple sequences Property name inconsistencies
4 participants