Skip to content

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.

Sign up for GitHub

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

Open
Stubbjax wants to merge 1 commit into
base: main
Choose a base branch
from
+20 −0
Open

bugfix: Do not show veterancy effects for dead units #1968

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions Generals/Code/GameEngine/Source/GameLogic/Object/Object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2789,6 +2789,11 @@ Bool Object::hasSpecialPower( SpecialPowerType type ) const
//-------------------------------------------------------------------------------------------------
void Object::onVeterancyLevelChanged( VeterancyLevel oldLevel, VeterancyLevel newLevel, Bool provideFeedback )
{
#if !RETAIL_COMPATIBLE_CRC
if (isEffectivelyDead())
Copy link

@xezon xezon Dec 10, 2025

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.

return;
#endif

updateUpgradeModules();

const UpgradeTemplate* up = TheUpgradeCenter->findVeterancyUpgrade(newLevel);
Expand Down Expand Up @@ -2843,6 +2848,11 @@ void Object::onVeterancyLevelChanged( VeterancyLevel oldLevel, VeterancyLevel ne
&& outerDrawable
&& outerDrawable->isVisible();

#if RETAIL_COMPATIBLE_CRC
if (isEffectivelyDead())
return;
#endif
Copy link

@xezon xezon Dec 10, 2025

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();
#endif

Copy link
Author

@Stubbjax Stubbjax Dec 11, 2025 •
edited
Loading

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.

Copy link

@xezon xezon Dec 11, 2025

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.

Copy link
Author

@Stubbjax Stubbjax Dec 17, 2025

Choose a reason for hiding this comment

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

Why is a return undesirable?

Copy link

@xezon xezon Dec 17, 2025

Choose a reason for hiding this comment

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

Breakdown, courtesy of Chat Gippity

Returning early from a function (i.e., having multiple return statements, especially in the middle of long or complex C++ functions) is often debated. What speaks against it are mainly concerns around readability, maintainability, and correctness in complex code. Here are the main arguments.

1. Loss of a clear control-flow narrative

In long or complex functions, early returns can fragment the logic.

  • You no longer have a single “entry → processing → exit” flow.
  • Readers must mentally track many possible exit points.
  • Understanding which code runs in which situations becomes harder.

This is especially problematic when:

  • The function has multiple phases (setup, processing, cleanup).
  • Later code depends subtly on earlier conditions.

2. Harder reasoning about invariants and state

Complex functions often establish invariants:

// After this point, x and y are valid and synchronized

If you return early:

  • Some invariants may only hold on some paths.
  • You must verify that every return happens before or after invariants are established.
  • Future modifications can easily violate assumptions.

This increases the cognitive load when modifying the function later.

3. Resource management and cleanup risks (without RAII)

Historically (and still in some legacy code):

  • Multiple return points made it easy to forget cleanup:

    • Memory deallocation
    • Unlocking mutexes
    • Closing files

Example risk:

lock(m);
if (error) return; // mutex never unlocked

Modern C++ mitigates this with RAII, but:

  • Legacy code may not use RAII consistently.
  • Mixing RAII and manual cleanup is still error-prone.

4. Debugging and instrumentation complexity

Multiple return points complicate:

  • Placing breakpoints at “the end” of a function
  • Logging exit conditions consistently
  • Adding profiling or tracing later

With one exit point, you can:

  • Log once
  • Validate postconditions
  • Centralize error reporting

5. Increased risk when modifying or extending the function

In long functions, future changes are common:

  • Adding new cleanup logic
  • Adding metrics or tracing
  • Enforcing postconditions

With many early returns:

  • Every return may need updating
  • Missing one return can introduce subtle bugs

This scales poorly as complexity grows.

6. Violates some team or project conventions

Many codebases explicitly prefer:

  • “Single exit point” for non-trivial functions

  • Or early returns only for:

    • Guard clauses at the top
    • Simple error handling

Violating conventions reduces consistency and codebase readability.

7. Encourages overly large functions

Early returns can mask deeper design issues:

  • A function doing too much
  • Many responsibilities and states

Refactoring into smaller functions often:

  • Makes early returns unnecessary
  • Improves readability and testability

Balanced perspective (important)

Early returns are not inherently bad.

They are generally good when:

  • Used as guard clauses at the top
  • The function is short and simple
  • They improve readability by avoiding deep nesting

They are more questionable when:

  • The function is long and multi-phase
  • There are resources or invariants to maintain
  • The function evolves over time

Practical guideline

A commonly accepted compromise in modern C++:

Use early returns for validation and error handling at the top.
Avoid mid-function returns in long, stateful, or multi-phase functions.

If mid-function returns feel necessary:

  • Consider extracting a helper function
  • Or restructuring into clearer phases

Copy link
Author

@Stubbjax Stubbjax Dec 17, 2025

Choose a reason for hiding this comment

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

Not many if any of those concerns apply here. The method is clearly separated into two blocks - logic and then animation; placing a return between the two blocks is reasonable. Matching the code within both precompiler directives aids in understanding that it is the same logic with only the placement being the difference.


if( doAnimation && TheGameLogic->getDrawIconUI() )
{
if( TheAnim2DCollection && TheGlobalData->m_levelGainAnimationName.isEmpty() == FALSE )
Expand Down
10 changes: 10 additions & 0 deletions GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3104,6 +3104,11 @@ Bool Object::hasAnySpecialPower() const
//-------------------------------------------------------------------------------------------------
void Object::onVeterancyLevelChanged( VeterancyLevel oldLevel, VeterancyLevel newLevel, Bool provideFeedback )
{
#if !RETAIL_COMPATIBLE_CRC
if (isEffectivelyDead())
return;
#endif

updateUpgradeModules();

const UpgradeTemplate* up = TheUpgradeCenter->findVeterancyUpgrade(newLevel);
Expand Down Expand Up @@ -3158,6 +3163,11 @@ void Object::onVeterancyLevelChanged( VeterancyLevel oldLevel, VeterancyLevel ne
&& outerDrawable
&& outerDrawable->isVisible();

#if RETAIL_COMPATIBLE_CRC
Copy link

@Caball009 Caball009 Dec 11, 2025

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.

Copy link
Author

@Stubbjax Stubbjax Dec 11, 2025

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.

if (isEffectivelyDead())
return;
#endif

if( doAnimation && TheGameLogic->getDrawIconUI() )
{
if( TheAnim2DCollection && TheGlobalData->m_levelGainAnimationName.isEmpty() == FALSE )
Expand Down
Loading