Skip to content

bugfix(controlbar): Fix possible divisions by zero in Control Bar code#2867

Open
Caball009 wants to merge 3 commits into
TheSuperHackers:mainfrom
Caball009:fix_zero_division_sciences_controlbar
Open

bugfix(controlbar): Fix possible divisions by zero in Control Bar code#2867
Caball009 wants to merge 3 commits into
TheSuperHackers:mainfrom
Caball009:fix_zero_division_sciences_controlbar

Conversation

@Caball009

@Caball009 Caball009 commented Jul 12, 2026

Copy link
Copy Markdown

Using the modified rank file in #797, there are two more places the game would crash because of a division by zero; in ControlBar::populatePurchaseScience and ControlBar::updateContextPurchaseScience.

Issue reproduction steps:

  1. Add the modified rank ini data to a custom map.
  2. Start a match and click on the special powers menu.
  3. Game crashes due to a division by zero.

Callstacks:

generalszh.exe!ControlBar::populatePurchaseScience(Player * player) Line 430
generalszh.exe!ControlBar::showPurchaseScience() Line 2977
generalszh.exe!ControlBar::togglePurchaseScience() Line 3016
generalszh.exe!ControlBarSystem(GameWindow * window, unsigned int msg, unsigned int mData1, unsigned int mData2) Line 418
generalszh.exe!GameWindowManager::winSendSystemMsg(GameWindow * window, unsigned int msg, unsigned int mData1, unsigned int mData2) Line 705
generalszh.exe!GadgetPushButtonInput(GameWindow * window, unsigned int msg, unsigned int mData1, unsigned int mData2) Line 218
generalszh.exe!GameWindowManager::winSendInputMsg(GameWindow * window, unsigned int msg, unsigned int mData1, unsigned int mData2) Line 724
generalszh.exe!GameWindowManager::winProcessMouseEvent(GameWindowMessage msg, ICoord2D * mousePos, void * data) Line 919
generalszh.exe!WindowTranslator::translateGameMessage(const GameMessage * msg) Line 242
generalszh.exe!MessageStream::propagateMessages() Line 1090
generalszh.exe!GameEngine::update() Line 910
generalszh.exe!Win32GameEngine::update() Line 91
generalszh.exe!GameEngine::execute() Line 983
generalszh.exe!GameMain() Line 55
generalszh.exe!WinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, char * lpCmdLine, int nCmdShow) Line 929
generalszh.exe!ControlBar::updateContextPurchaseScience() Line 495
generalszh.exe!ControlBar::update() Line 1530
generalszh.exe!InGameUI::update() Line 2093
generalszh.exe!W3DInGameUI::update() Line 370
generalszh.exe!SubsystemInterface::UPDATE() Line 131
generalszh.exe!GameClient::update() Line 781
generalszh.exe!W3DGameClient::update() Line 94
generalszh.exe!SubsystemInterface::UPDATE() Line 131
generalszh.exe!GameEngine::update() Line 910
generalszh.exe!Win32GameEngine::update() Line 91
generalszh.exe!GameEngine::execute() Line 983
generalszh.exe!GameMain() Line 55
generalszh.exe!WinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, char * lpCmdLine, int nCmdShow) Line 929

TODO:

  • Replicate in Generals.

@Caball009 Caball009 added Bug Something is not working right, typically is user facing GUI For graphical user interface Minor Severity: Minor < Major < Critical < Blocker Gen Relates to Generals ZH Relates to Zero Hour Crash This is a crash, very bad labels Jul 12, 2026
@greptile-apps

greptile-apps Bot commented Jul 12, 2026

Copy link
Copy Markdown

Greptile Summary

This PR updates rank-bound handling to avoid invalid control-bar progress calculations. The main changes are:

  • Normalize equal rank bounds in the Zero Hour player state.
  • Apply normalization after loading player data.
  • Use the normalized bounds in purchase-science and experience-bar progress calculations.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.

Important Files Changed

Filename Overview
Core/GameEngine/Source/GameClient/GUI/ControlBar/ControlBar.cpp Keeps both purchase-science progress calculations aligned with the player rank bounds.
Core/GameEngineDevice/Source/W3DDevice/GameClient/GUI/GUICallbacks/W3DControlBar.cpp Updates general-experience progress rendering to use the player rank interval directly.
GeneralsMD/Code/GameEngine/Include/Common/Player.h Declares the private helper for normalizing rank bounds.
GeneralsMD/Code/GameEngine/Source/Common/RTS/Player.cpp Uses the new helper during rank reset, rank changes, and post-load processing.

Reviews (11): Last reviewed commit: "Avoided possible divisions by zero in Co..." | Re-trigger Greptile

Comment thread Core/GameEngine/Source/GameClient/GUI/ControlBar/ControlBar.cpp Outdated
@Caball009
Caball009 force-pushed the fix_zero_division_sciences_controlbar branch from 47b4cdc to 2ac70a7 Compare July 12, 2026 12:07
@Caball009 Caball009 changed the title bugfix(controlbar): Fix possible divisions by zero in ControlBar code bugfix(controlbar): Fix possible divisions by zero in Control Bar code Jul 12, 2026
@Skyaero42

Copy link
Copy Markdown

If skillPointsRequired is 0, than this implies that a rank up didn't go through - after all the player has enough points to rank up to the next level. So I wonder if the fix is at the wrong place

// TheSuperHackers @bugfix Caball009 12/07/2026 Prevent possible division by zero.
if (skillPointsRequired > 0)
{
progress = ((player->getSkillPoints() - player->getSkillPointsLevelDown()) * 100) / skillPointsRequired;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This code now exists at 3 locations. Maybe consolidate it to one?

Comment thread Core/GameEngine/Source/GameClient/GUI/ControlBar/ControlBar.cpp Outdated
@Caball009
Caball009 force-pushed the fix_zero_division_sciences_controlbar branch from 2ac70a7 to ed174e2 Compare July 15, 2026 12:09
@Caball009
Caball009 force-pushed the fix_zero_division_sciences_controlbar branch from ed174e2 to ede4635 Compare July 15, 2026 12:46
@Caball009 Caball009 closed this Jul 15, 2026
@Caball009
Caball009 force-pushed the fix_zero_division_sciences_controlbar branch from ede4635 to 480a9a7 Compare July 15, 2026 21:51
@Caball009 Caball009 reopened this Jul 15, 2026
@Caball009

Copy link
Copy Markdown
Author

@greptileai re-review this pull request.

const RankInfo* nextRank = TheRankInfoStore->getRankInfo(m_rankLevel+1);
m_levelUp = nextRank ? nextRank->m_skillPointsNeeded : INT_MAX;

// TheSuperHackers @bugfix Prevent possible division by zero in the control bar code.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

How about we introduce a hasNextRank function? Then it can be explicitly tested whether it has a next rank.

Reason: The current interface sort of implies that it can reach this INT_MAX points level to reach the next rank, but it is not possible.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'm not sure I understand. Is this relevant to the division by zero?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes. Right now the code relies on MAX_INT to work, but its shady.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Perhaps you can post some code for what you have in mind.

@Caball009
Caball009 force-pushed the fix_zero_division_sciences_controlbar branch from c8c0c76 to 2f4d086 Compare July 21, 2026 11:21
Comment thread GeneralsMD/Code/GameEngine/Source/Common/RTS/Player.cpp Outdated
@Caball009
Caball009 force-pushed the fix_zero_division_sciences_controlbar branch 3 times, most recently from fadf08f to 4444518 Compare July 21, 2026 11:38
Comment on lines +2794 to +2799
if (levelUp == levelDown)
{
// TheSuperHackers @bugfix Prevent possible division by zero in the control bar code.
m_levelUp = INT_MAX;
m_levelDown = 0;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Do not terminalize duplicates

Equal adjacent SkillPointsNeeded values are valid custom-rank input for the crash case this code handles. When a player reaches rank N and rank N+1 has the same threshold, this branch replaces the next threshold with INT_MAX; addSkillPoints() can then never advance the player to rank N+1 even though the threshold is already met. Handle a zero-width transition without converting it into the terminal-rank sentinel.

Prompt To Fix With AI
This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Source/Common/RTS/Player.cpp
Line: 2794-2799

Comment:
**Do not terminalize duplicates**

Equal adjacent `SkillPointsNeeded` values are valid custom-rank input for the crash case this code handles. When a player reaches rank N and rank N+1 has the same threshold, this branch replaces the next threshold with `INT_MAX`; `addSkillPoints()` can then never advance the player to rank N+1 even though the threshold is already met. Handle a zero-width transition without converting it into the terminal-rank sentinel.

How can I resolve this? If you propose a fix, please make it concise.

@Caball009
Caball009 force-pushed the fix_zero_division_sciences_controlbar branch from 4444518 to c4b084e Compare July 21, 2026 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something is not working right, typically is user facing Crash This is a crash, very bad Gen Relates to Generals GUI For graphical user interface Minor Severity: Minor < Major < Critical < Blocker ZH Relates to Zero Hour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants