-
Notifications
You must be signed in to change notification settings - Fork 126
bugfix: Do not show veterancy effects for dead units #1968
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
base: main
Are you sure you want to change the base?
bugfix: Do not show veterancy effects for dead units #1968
Conversation
| #if RETAIL_COMPATIBLE_CRC | ||
| if (isEffectivelyDead()) | ||
| return; | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should do:
#if RETAIL_COMPATIBLE_CRC
doAnimation &= isEffectivelyDead();
#endifThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it really matter when they're effectively the same thing and this one is deprecated? Having them the same highlights that both compatible and non-compatible logic is identical and the only difference is when it is called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My motivation here was to get rid of the return in the middle of the function.
| void Object::onVeterancyLevelChanged( VeterancyLevel oldLevel, VeterancyLevel newLevel, Bool provideFeedback ) | ||
| { | ||
| #if !RETAIL_COMPATIBLE_CRC | ||
| if (isEffectivelyDead()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can add TheSuperHackers comment because it is user facing, even if obvious what it does.
| && outerDrawable | ||
| && outerDrawable->isVisible(); | ||
|
|
||
| #if RETAIL_COMPATIBLE_CRC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this require retail compatibility? It looks like this only disables sound and visuals locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically not. The idea is that this code gets removed when retail compatibility is dropped, so leaving this here will make it easier to keep track of.
This change prevents veterancy effects from showing for dead units. It will also save running some redundant upgrade logic once retail compatibility is dropped.
Issue Demonstrations
DEAD_VET_1.mp4
DEAD_VET_2.mp4