Skip to content

[GEN][ZH] Initialize supplyTruck variable #820

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

Merged
merged 3 commits into from
May 11, 2025
Merged

[GEN][ZH] Initialize supplyTruck variable #820

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
42fc40d
GEN ZH: supplyTruck needs to be initialized to true to prevent mm whe…
helmutbuhler May 5, 2025
f61b6b6
Set supplyTruck = false on newer compilers because that was the origi…
helmutbuhler May 6, 2025
846a9ee
Minimize diff in comments
xezon May 8, 2025
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
9 changes: 2 additions & 7 deletions Generals/Code/GameEngine/Source/GameLogic/AI/AIPlayer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1041,13 +1041,8 @@ Bool AIPlayer::isLocationSafe(const Coord3D *pos, const ThingTemplate *tthing )
void AIPlayer::onUnitProduced( Object *factory, Object *unit )
{
Bool found = false;
// TheSuperHackers @fix Mauller 26/04/2025 Fixes uninitialized variable.
// To keep retail compatibility this needs to remain uninitialized in VS6 builds.
#if defined(_MSC_VER) && _MSC_VER < 1300
Bool supplyTruck;
#else
Bool supplyTruck = false;
#endif
// TheSuperHackers @fix helmutbuhler 05/05/2025 Fixes uninitialized variable.
Bool supplyTruck = true;

Copy link

Choose a reason for hiding this comment

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

Based on the subsequent code that the uninitialised variable was hitting, the correct initialisation value should be false.

If we think it is better to initialise this to true then set the VC6 specific variable to initialise to true, this way we maintain the correct behaviour when removing VC6 support.

Copy link

Choose a reason for hiding this comment

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

Can we figure out what user facing behaviour is changing when it is set to false?

Copy link

Choose a reason for hiding this comment

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

It's code specific to construction vehicles that aren't also supplytrucks.

if (!supplyTruck && unit->isKindOf(KINDOF_DOZER)) {
	if (m_dozerQueuedForRepair) {
		m_repairDozer = unit->getID();
		m_dozerQueuedForRepair =false;
	} else {
		m_buildDelay = 0;
		m_structureTimer = 1;
	}
}

Essentially dozers will always be treated as though they are also supply trucks and not have variables relevant to them being set properly.

Copy link
Author

Choose a reason for hiding this comment

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

Fair enough. I just changed it as requested.

Copy link

Choose a reason for hiding this comment

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

It's code specific to construction vehicles that aren't also supplytrucks.

Can you please figure out what changes user facing? Does it fix a bug?

Copy link

Choose a reason for hiding this comment

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

Could we verify this observation with AI match?

Copy link

Choose a reason for hiding this comment

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

We should be able to spot it if we play against a large map with all other slots filled with AI.

In theory the playback should show:

  • With the value set to false: Dozers should not idle after being constructed and should go about building something.
  • With the value set to true: Dozers may idle for a random period before constructing something.

Copy link

Choose a reason for hiding this comment

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

So in essence the outward effect would be that a newly constructed dozer would not immediately go about constructing a new building as was originaly intended for the AI to do. And instead it could idle for an random period till the build delay timers ran down.

Based on my earlier logging, the delay would be between 0 and 60. I presume those are frames, so up to roughly two second delay for building anything.

Copy link

Choose a reason for hiding this comment

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

2 seconds is not much :D

Copy link

Choose a reason for hiding this comment

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

I can create an AI to test this but I'm not exactly sure what conditions should be generated. m_buildDelay is set to 0 in many cases, including when AI queue buildings for construction (in buildSpecificAIBuilding).

// factory could be NULL at the start of the game.
if (factory == NULL) {
Expand Down
9 changes: 2 additions & 7 deletions GeneralsMD/Code/GameEngine/Source/GameLogic/AI/AIPlayer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1048,13 +1048,8 @@ Bool AIPlayer::isLocationSafe(const Coord3D *pos, const ThingTemplate *tthing )
void AIPlayer::onUnitProduced( Object *factory, Object *unit )
{
Bool found = false;
// TheSuperHackers @fix Mauller 26/04/2025 Fixes uninitialized variable.
// To keep retail compatibility this needs to remain uninitialized in VS6 builds.
#if defined(_MSC_VER) && _MSC_VER < 1300
Bool supplyTruck;
#else
Bool supplyTruck = false;
#endif
// TheSuperHackers @fix helmutbuhler 05/05/2025 Fixes uninitialized variable.
Bool supplyTruck = true;

// factory could be NULL at the start of the game.
if (factory == NULL) {
Expand Down
Loading