[TD] Incorrect fix for MCV deployment double-power bug

Nyerguds commented 2 years ago

Short overview:

The original C&C has an issue with the MCV deployment where the power usage is applied twice; once when it starts deploying, and once when the build-up animation finishes. Power usage is normally only added after the build-up finishes, so the incorrect one is the first one. That first one is a bug in the code that is meant to activate preplaced builldings; player-owned buildings that are revealed from the start, unexplored player-owned buildings at the moment they get explored, and all AI ones. The Remaster code adds an extra check on the Construction Yard to remove the second power addition, rather than fixing the actual bug causing the first one.

Details:

The Remaster devs tried to fix the MCV deploy power bug by editing a check on the CY into BuildingClass::Mission_Construction()

https://github.com/electronicarts/CnC_Remastered_Collection/blob/7d496e8/TIBERIANDAWN/BUILDING.CPP#L4021 if (*this != STRUCT_CONST || GameToPlay != GAME_NORMAL)

But this fix is wrong. It disturbs the normal order of operations where power usage is only added after the building finished constructing. This, in turn, not only means that Construction Yards (re)built by the AI in missions will never take up any power at all, but also that destroying them will subtract power usage that was never added. This whole if check was not present in the original game, and should be removed.

The real issue is that BuildingClass::Revealed(HouseClass * house) calls BuildingClass::Grand_Opening() on the deployed MCV, before the building is actually built up. The call to Grand_Opening() there is only meant for initialising preplaced buildings on maps, either for the AI, or at the moment they are revealed by the player.

https://github.com/electronicarts/CnC_Remastered_Collection/blob/7d496e8/TIBERIANDAWN/BUILDING.CPP#L4876 if (!In_Radio_Contact() && (house == House && GameToPlay == GAME_NORMAL) && Mission != MISSION_CONSTRUCTION)

The check that should prevent this from taking effect for deployed MCVs is Mission != MISSION_CONSTRUCTION, This check works for including preplaced buildings, since they are set on MISSION_GUARD, but buildings that start from their construction animation never actually get filtered out by this, since this code is executed before they are initially set to execute MISSION_CONSTRUCTION. At that point, they are simply still set on the 'uninitialised' value MISSION_NONE. The only reason they skip the initial reveal run that triggers Grand_Opening is because of another check that tests whether they are radio-tetheted to something that is building them.

However, just changing that check to check on MISSION_NONE instead doesn't seem correct either; it messes up on player-owned preplaced buildings when they are unlimboed, since those are indeed set to MISSION_NONE, and never get Revealed explicitly afterwards. So we should leave that. And to fix the MCV deploy case, radio-tethering is also no solution, since the MCV gets removed immediately. So the only real solution is to artificially set the new Construction Yard to MISSION_CONSTRUCTION before calling Unlimbo:

https://github.com/electronicarts/CnC_Remastered_Collection/blob/7d496e8/TIBERIANDAWN/UNIT.CPP#L1739

if (building) {
    building->Assign_Mission(MISSION_CONSTRUCTION);
    building->Commence();
    if (building->Unlimbo(Adjacent_Cell(Coord, FACING_NW))) {

This seems to fix the issue without any side effects.

Actual fixes:

Remove the entire if (*this != STRUCT_CONST || GameToPlay != GAME_NORMAL) check in BuildingClass::Mission_Construction(void), and always execute Grand_Opening() at that point.
Edit UnitClass::Try_To_Deploy(void) and before the call to building->Unlimbo, add the lines to assign the MISSION_CONSTRUCTION with Assign_Mission and Commence it.

Notes:

A building that is built and then placed down by a Construction Yard fails the if check in BuildingClass::Revealed(HouseClass * house) because In_Radio_Contact() returns true for them. I assume this is the link with the Construction Yard that built it, which in turn makes the CY perform its animation.
Fixing this also means that any modders that may want to add more deployable buildings will not run into this same issue, forcing them to add more checks to the Mission_Construction function.
It is possible this bug still exists in RA as well, but by default, MCVs there don't produce or use any power at all.

xezon commented 2 years ago

That bug report looks more complicated than the fix itself :D

Nyerguds commented 2 years ago

Yea, in the original game, the fix is literally a one-byte edit. The research was tough though.

[edit]

_This was about the original proposed fix of changing the check from MISSION_CONSTRUCTION to MISSION_NONE. The final Assign_Mission + Commence fix is quite a bit more than one byte._

Nyerguds commented 2 years ago

I just realised that the remaster's fix for the power thing probably means that construction yards in the campaign that get built by the AI from other construction yards never use up any power at all, since the only function that would apply that gets skipped.

[edit]

And to make it worse, destroying these CYs will subtract power usage from the House while it was never added. So continually destroying AI CYs that get rebuilt will give the AI more and more free power.

ChthonVII commented 2 years ago

Westwood fixed this in RA by adding the HasOpened member to BuildingClass. Doing that would also provide an easy fix for issue #749. (Looks like #749 affects RA too, even though this issue does not.)

Nyerguds commented 2 years ago

That sounds like a very elegant solution, yes. Then that member can simply be checked instead of the Mission.

This could also help when considering re-enabling the 'Stop' order on defense buildings. I would prefer re-enabling stop because it's useful for aborting force-fire orders.

The 'Stop' order was disabled in (DOS) v1.22 to prevent stopping of selling (which resulted in getting free infantry while keeping the building), but the fact Stop was possible on these buildings also had the side effect that if a player would decide to "speed up" the construction of a defense building by pressing S while it was still building up, the radio contact with the CY would never be broken, and the CY would become completely unusable until the building was sold or destroyed. So to fix this first case, a check on HasOpened would also work.

ChthonVII commented 2 years ago

On second thought, adding HasOpened is a good fix for #749, but fixing this one correctly will require your proposed changes. (Well, maybe do things a little different in BuildingClass::Revealed() -- add a check for MISSION_NONE, keep the check for MISSION_CONSTRUCTION just in case it can happen, restore the "when discovered by anybody MP" condition that Petroglyph got rid of (since HasOpened can fix that), port that MP condition to RA (because it seems reasonable), rewrite RA's conditions into a single streamlined statement, and add a check for HasOpened to avoid wasting function call overhead.)

I've written that up, but haven't had time to test it. In the course of doing that, I noticed that repacking the MCV is missing not only the power logic, but pretty much all logic related to shutting down a building. So I added that and need to test that too.

On a totally unrelated note, whatever happened to https://nyerguds.arsaneus-design.com/cncstuff/?

[Edit: How did the stop command work in 1.22, aside from abuse? Was the intent to stop base defenses from firing? Wouldn't they just go right back to picking a target in range? Was any other form of stopping a building (e.g. stopping a sale or construction) supposed to be valid?]

ChthonVII commented 2 years ago

Well, it seems my "fix" for the MCV repacking is wrong because it's resulting in double loss of both gain and drain. The thing is I cannot for the life of me figure out where power and drain are being altered during a MCV repack. Any insights?

Nyerguds commented 2 years ago

I'm not sure if the Remaster changed anything in the undeploy code. In the original, it was literally the same as sell, just with no infantry spawned, no money refunded and an MCV popped out at the end.

As for the "mission" check, as mentioned, it never worked on MISSION_CONSTRUCTION anyway since on the MCV (and probably any other constructed buildings) the check happens before that order is even assigned. And any buildings that are constructed normally by a CY fail the whole 'if' check even before that, because they are in radio contact with the CY that's building them.

Nyerguds commented 1 year ago

On a totally unrelated note, whatever happened to https://nyerguds.arsaneus-design.com/cncstuff/?

Late reply, but... it's just not https. Remove the 's' and it works fine😛

Nyerguds commented 8 months ago

I just found a huge bug with the fix I proposed (changing the check in BuildingClass::Revealed(HouseClass * house) from MISSION_CONSTRUCTION to MISSION_NONE); it makes preplaced player buildings that are revealed from the mission start not do the opening correctly, resulting in bugs where the storage capacity and power usage of preplaced buildings are not registered.

It seems the actual bad power addition is a result of the call to BuildingClass::Unlimbo() in the code where the MCV spawns the CY. The deeper call to Revealed from inside this Unlimbo erroneously treats it as preplaced building because it doesn't have MISSION_CONSTRUCTION set and isn't radio-tethered to a constructing CY. Oddly enough, before that Revealed call, Unlimbo at one point calls Enter_Idle_Mode, which should queue up the MISSION_CONSTRUCTION mission, and right after that, it calls Commence(), so that should set the mission correctly, but for some reason it doesn't. The radio-tether-check might already have been a hotfix to work around that. But other parts of the code now rely on the way it works (or, doesn't work) there, so fixing that non-initialising MISSION_CONSTRUCTION is probably a bad idea too.

The solution I came up with instead is to just leave all those checks alone, and give it what it really wants; in the UnitClass::Try_To_Deploy function, just explicitly set the mission to MISSION_CONSTRUCTION before the Unlimbo call:

https://github.com/electronicarts/CnC_Remastered_Collection/blob/7d496e8/TIBERIANDAWN/UNIT.CPP#L1739

if (building) {
    building->Assign_Mission(MISSION_CONSTRUCTION);
    building->Commence();
    if (building->Unlimbo(Adjacent_Cell(Coord, FACING_NW))) {

This seems to fix the issue without any side effects. Funnily enough, since actually-constructing buildings are prevented from executing the Grand_Opening function on initial place down by the radio tether with the CY, this makes it the only case in which that check on MISSION_CONSTRUCTION actually gets used as it is intended.

ChthonVII commented 8 months ago

I just realised a huge bug with this proposed system; it makes preplaced player buildings that are revealed from the mission start not do the opening correctly, resulting in bugs where the storage capacity and power usage of preplaced buildings are not registered.

I think I recall fixing this at some point. Can you name a mission where this can be tested?

Nyerguds commented 8 months ago

Any with a player-owned base that is revealed from the mission start. Don't know any like that where you got a refinery, but on "under siege" you should see you have no power usage, only production.

[edit]

GDI 8B. With the old fix, you will be 100% unable to harvest in that mission.

Nyerguds commented 8 months ago

[Edit: How did the stop command work in 1.22, aside from abuse? Was the intent to stop base defenses from firing? Wouldn't they just go right back to picking a target in range? Was any other form of stopping a building (e.g. stopping a sale or construction) supposed to be valid?]

Any force-fire order, really. I've often force-fired on friendly units to destroy the tiberium underneath, because for some reason tiberium itself isn't targetable with force-fire. No idea why; it can be a real nuisance for base planning. (Note, I just mean with units; base defenses can't force-fire on the ground anyway, and that's fine; fairly sure that's done to avoid people abusing spread damage on e.g. Guard Towers. But if you force-fire something on a friendly unit you probably want the ability to make it stop doing that, too.)

One specific case that always annoyed me is tree clumps. You can make base defenses force-fire on trees to destroy them, but some tree types (the "TC" Tree Clumps) are invulnerable. So if you wanted to clear some trees around your base but accidentally targeted one of those, the Stop command could free it up again. Since v1.22, a defense building in this situation is stuck attacking that object until you specifically manually retarget it on some other object.

But even without force-fire, there are valid cases. One that immediately comes to mind is destroying walls. For example, in the Covert Ops. "Infiltrated", I often use the south-east Guard Tower to get rid of some of the useless east-side walls. And in any mission where you capture and build inside an enemy base, you might want to clear walls too, and I often do that using the base defenses I built there. But if any enemy actually approaches, I'd like to quickly make it stop doing that and target enemies again, without requiring me to wait for an actual enemy to get in range and needing to manually target it.

ChthonVII commented 8 months ago

Any with a player-owned base that is revealed from the mission start. Don't know any like that where you got a refinery, but on "under siege" you should see you have no power usage, only production.

[edit]

GDI 8B. With the old fix, you will be 100% unable to harvest in that mission.

Yeah, it looks like that's fixed in my mod using an implementation based on your comment and RA's HasOpened member. I'm getting low power of 475/430 immediately upon starting Under Siege, and the hospital is applying drain immediately upon starting GDI 8B. That's correct, isn't it? Oh, did you mean GDI 8A? Like Under Siege, I'm starting in a low power state until repairing the PPs and selling the silos, and the refinery works fine once I tell the harvester to go harvest. I think the only relevant commits are fdb9f5b and 1dd1a81, with the latter (I think) fixing the bug mentioned here.

Nyerguds commented 8 months ago

Yea, all these test results seem correct.

As I mentioned, there seem to be some cascading bugs. From what I see it seems to be caused by the MISSION_CONSTRUCTION set in Enter_Idle_Mode (called from Unlimbo) not actually taking effect as it should, but I fear that that's another rabbit hole, since the fact that's not taking effect is what makes the preplaced pre-revealed player owned buildings work properly at this moment.

Right now I'm just trying to finish C&C95 v1.06c revision 4 with some knowledge gleamed from the source code so the new patch adapted to the Steam-update of The Ultimate Collection actually contains some new fixes. Everything I'm talking about has only been tested through disassembly and binary patching of the exe. With the source opened and the fact I got a completely identified disassembly of C&C95.exe, I can do pretty much anything in there now. I just haven't had the motivation to work on it.

ChthonVII commented 8 months ago

Right now I'm just trying to finish C&C95 v1.06c revision 4 with some knowledge gleamed from the source code so the new patch adapted to the Steam-update of The Ultimate Collection actually contains some new fixes. Everything I'm talking about has only been tested through disassembly and binary patching of the exe. With the source opened and the fact I got a completely identified disassembly of C&C95.exe, I can do pretty much anything in there now. I just haven't had the motivation to work on it.

Oh, I'm sorry. I didn't realize you were trying to binary patch of the new Ultimate Collection edition. I think an implementation like the one I did is likely too complex for that.

Nyerguds commented 8 months ago

It's doable, but it's a bit out of scope for the patch, yes. As I said, with the info and reference material I have, I can do most stuff that normal programming can, but there are some exceptions.

The main issue with binary patching is that it's extremely difficult to expand the size of existing object types; it usually involves extensive patching to, or even completely remaking, pretty much all functions that handle it. So something like adding the HasOpened bool isn't really feasible for me, unless I can stuff it in some unused upper byte of a 32-bit int somewhere in the existing struct. IIRC that's kind of how I did the custom remapping system on the House objects.

Changing object definitions is also really tedious because nothing in the binary code actually contains these definitions; you just have to derive it from the amount of memory that gets reserved for the object, and the locations inside the object that are accessed by the code to store specific things. Knowing how the objects look isn't much of an issue with access to the code, but knowing which places actually access properties on objects of a specific type is extremely difficult, especially with the presence of compiler optimisation, and the frequent occurrence of in-line operations where the C++ code has real function calls.

But yea, not looking into that kind of extensive modding right now; assigning MISSION_CONSTRUCTION is a decent fix for this specific problem.

zelurker commented 8 months ago

Any with a player-owned base that is revealed from the mission start. Don't know any like that where you got a refinery, but on "under siege" you should see you have no power usage, only production.

[edit]

GDI 8B. With the old fix, you will be 100% unable to harvest in that mission.

Just tried these examples out of curiosity. For 8b you start with a mcv but there is a hospital which drains 20 from the start so power is 0/20 then you build the construction yard : 30 / 35 because the const yard gives 30 but takes 15 at the same time then power plant 130 / 35 then refinery 140 / 75, yeah it's crazy the refinery gives 10 and takes 40 anyway at this point it works without trouble and you can harvest all you want.

For under siege you get 430 / 475 at start so under powered and nothing works, but it's apparently intended in the misson, you have to sell something, the temple seems to be the best option here with its 150 consumption. Anyway detail of the power : enhanced power plant 200 + 2xpower plant 200 again = 400 + 30 for the const yard Detail of the drain : repair 30 Turret 204 Temple 150 Hand 20 Air 30 Sam 204 Silo 103 Const 15 Command 40 475 confirmed. And that's with the "fix" in place, I have indeed at building.cpp:430 : // Construction yard already called this on reveal in normal game mode, so don't do twice if (this != STRUCT_CONST || GameToPlay != GAME_NORMAL) { Grand_Opening(); } So well everything seems to work for me... ! Did I miss something ?

Nyerguds commented 8 months ago

Did I miss something ?

Yea, the whole conversation, it seems. I wasn't talking about the original Remaster fix, I was talking about my own old proposed fix of changing the check in BuildingClass::Revealed(HouseClass * house) from MISSION_CONSTRUCTION to MISSION_NONE.

Start from https://github.com/TheAssemblyArmada/Vanilla-Conquer/issues/747#issuecomment-2002615204

Removing the botched Remaster fix remains necessary, though; I already explained in detail that that fix works fine for the player but messes up on the AI, since the AI builds CYs as buildings, not as deploying MCVs, meaning that with the remaster fix, they don't execute the Opening at all. And as I explained in the original issue description, it's a hotfix applied on the wrong spot (after the build-up anim), not a fix for the real bug (which is the power addition that happens before the build-up anim).

zelurker commented 8 months ago

Did I miss something ?

Yea, the whole conversation, it seems. I wasn't talking about the original Remaster fix, I was talking about my own old proposed fix of changing the check in BuildingClass::Revealed(HouseClass * house) from MISSION_CONSTRUCTION to MISSION_NONE.

Start from #747 (comment)

Removing the botched Remaster fix remains necessary, though; I already explained in detail that that fix works fine for the player but messes up on the AI, since the AI builds CYs as buildings, not as deploying MCVs, meaning that with the remaster fix, they don't execute the Opening at all. And as I explained in the original issue description, it's a hotfix applied on the wrong spot (after the build-up anim), not a fix for the real bug (which is the power addition that happens before the build-up anim).

Well the part I quoted was a reply to ask a way to test the problem, here clearly it didn't test the problem. But you clearly want no external intervention in this super long conversation, your choice, sorry for my intervention then, I'll make sure that doesn't happen again...

Nyerguds commented 8 months ago

"The problem" was specifically only a side effect of a fix mentioned earlier in the conversation. Without that, any testing you might do is simply irrelevant. You apparently started testing stuff without applying anything mentioned in this whole thread.

ChthonVII commented 8 months ago

@zelurker, let me try to clarify the background here:

In C&C95 there are three bugs related to buildings' "grand opening" routine:

The double MCV power/drain bug due to them doing grand opening twice. (The bug reported in this issue.)
The bug where destroying a building during its build-up animation will subtract drain and storage that were never added in the first (issue 749)
The bug where you can issue a stop order to a defensive building during its build-up animation and it will immediately be able to shoot.

In Remastered Petroglyph made some changes:

The double MCV power/drain bug has an improper fix. Petroglyph stopped one of the grand openings from happening, but it was the wrong one. Consequently, con yards that the AI (re)build as normal buildings never do a grand opening at all. (That's what the top post in this issue is about.)
749 is untouched.
Buildings are not allowed to receive stop orders at all.

In the top post here:

Nyerguds proposed a simple fix for the double MCV power/drain bug that would stop the grand opening that ought to be stopped.

Over in my Remastered mod

I've implemented a "full" solution that fixes all three bugs by backporting the HasOpened member from RA to TD, combined with part of Nyergud's fix and undoing some stuff Petroglyph did. (Commits fdb9f5b, 1dd1a81, 585510b, and 032882c.)
MCV only grand opens once, and it's the right one.
Destroyed buildings only subtract drain and storage if they grand opened.
Buildings can generally receive stop orders, but not during de/construction.

In Nyerguds' first post since last year:

He realized that his proposed fix in the top post has a buggy side effect. Due to interaction with another bug, it causes pre-placed buildings to never grand open.
He suggests a different simple fix with no side effects.

In my post replying to that:

I ask for help testing to see if my implementation of the fix has the same bug. (Because I have a vague memory of already fixing this bug in 1dd1a81.)
It turns my implementation does not suffer from this bug. Great!

In the next couple posts:

Nyerguds and I discuss why he wants a simple fix when I've got a full fix for all three related bugs -- he's looking at making a binary patch for the Ultimate Collection release, and adding a member to a struct is highly impractical in that context.

Then you came along and tested something (unmodded Remastered? Vanilla Conquer?) for the bug that was only ever present as a side effect in Nyerguds' first proposed fix. Of course you didn't find it. And everyone else in the discussion was left scratching their heads why you did that.

Summary of where things should go from here:

My "full" fix solves all three bugs and should be adopted for Vanilla Conquer and Remastered mods.
Nyerguds' second simple fix is ideal for fixing the double MCV power/drain bug via binary patching for, e.g., the Ultimate Collection release.
Testing is always welcome and helpful, but please read the thread carefully enough to be sure you're conducting a meaningful test.

All clear now?

Nyerguds commented 8 months ago

Note, the disabling of stop orders on buildings is not a Petroglyph fix. It was only ever possible on defense buildings, and was disabled on those too in the very final official patch (v1.22) of the DOS game. So it has always been in C&C95.

The reason it was disabled was probably not even related to stopping build-ups; it could cancel Selling orders, giving people free infantry while keeping the building.

ChthonVII commented 8 months ago

Note, the disabling of stop orders on buildings is not a Petroglyph fix. It was only ever possible on defense buildings, and was disabled on those too in the very final official patch (v1.22) of the DOS game. So it has always been in C&C95.

Oh. Didn't know that. I assumed that since it's implemented in the DLL interface in Remastered that it's a Petroglyph thing. On a closer look I see now that they copied this from Keyboard_Process() in CONQUER.CPP.

TheAssemblyArmada / Vanilla-Conquer