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.

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

Conversation

helmutbuhler
Copy link

@helmutbuhler helmutbuhler commented May 6, 2025

So I did a bunch of automated testing with about 1400 replays (that's about 20 days of combined gametime). All these replays

  • did not mismatch when they were originally played (that is stored in the replay)
  • have at least 2 human players
  • have at least one AI player
  • were played on a compatible 1.04 or 1.05 retail version

I checked to leave the supplyTruck variable uninitialized and initialized to true. The end result was that not a single replay mismatched with it initialized to true. (I already confirmed that initializing to false causes mismatches, so I didn't check that again)

So I suggest to initialize it. Why?

  • I already confirmed that enabling some logging in StateMachine::internalGetState will cause mismatches when the variable is left uninitialized. (See [GEN][ZH] Fix Mismatch in Release Build when Logging is enabled #759)
  • Some future changes in very different areas can cause mismatches in the future
  • Even in the current version, leaving it uninitialized can potentially cause mismatches when two peers happen to have different stack memory there (that's quite difficult to check though)

I realize that some tests were already made where it was concluded to leave it uninitialized. But those tests were made on a version that added some logging in the function with the uninitialized variable. That likely skewed the results. I also asked for replays that were used in those tests, but have received none so far.

If there is demand I can upload the replays I tested with. I will also hopefully be able to make a PR with the automated testing code soon.

I can also conditionally initialize the variable to false on newer compilers, if that would be more logical (I haven't studied the code that uses that variable much).

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.

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

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).

@xezon
Copy link

xezon commented May 6, 2025

Very nice investigation. Reads plausible to me that changing the stack would introduce mismatches with that uninitialized variable.

@xezon xezon added this to the Stability fixes milestone May 6, 2025
@xezon xezon added Major Severity: Minor < Major < Critical < Blocker Stability Concerns stability of the runtime Fix Is fixing something labels May 6, 2025
@aliendroid1
Copy link

I also checked the crc codes that are generated for each object with supply truck initialized and uninitialized and could not find any difference on both vs6 and vs22

Copy link

@xezon xezon left a comment

Choose a reason for hiding this comment

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

I think this change needs test in multiplayer with AI, because this is where originally the incompatibility was reported.

@roossienb
Copy link

I think this change needs test in multiplayer with AI, because this is where originally the incompatibility was reported.

iirc specifically when one uses a SH client and the other 1.04, while playing against multiple AI opponents.

@roossienb
Copy link

I think this change needs test in multiplayer with AI, because this is where originally the incompatibility was reported.

I'll ask the QA team

@sorcerer86pt
Copy link

sorcerer86pt commented May 11, 2025

Testing Report

Procedure:

  • Used 6 person map
  • 2 Human players
  • 4 AI players (Medium difficulty)
  • 1 used vanilla exe, other used this one
  • one game has created with this fixed exe, the other 2 was with vanilla

Results:

  • 1st Test, 2 Human USA vs 4 GLA AI Players, Vanilla Host: No Mismatch
  • 2nd Test, 2 Human China vs 4 China, Vanilla Host: No Mismatch
  • 3rd Test, 2 Human GLA vs 4 USA, Fixed Exe as Host: No Mismatch

Proof:

Copy link

@xezon xezon left a comment

Choose a reason for hiding this comment

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

Looks good.

@xezon xezon added the Bug Something is not working right label May 11, 2025
@xezon xezon merged commit 1b1682d into TheSuperHackers:main May 11, 2025
18 checks passed
@xezon xezon deleted the supplyTruck_true_sh branch May 11, 2025 13:33
@xezon xezon added Gen Relates to Generals ZH Relates to Zero Hour labels May 16, 2025
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 Fix Is fixing something Gen Relates to Generals Major Severity: Minor < Major < Critical < Blocker Stability Concerns stability of the runtime ZH Relates to Zero Hour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants