bugfix(input): Prevent drag-selecting enemy beacons and enable selecting stealthed units while observing

[Stubbjax]

• Fixes Pulling selection box around non owned vehicle with passenger slots will yield different feedback depending on taken passenger count #119
• Fixes Impossible to select stealth units in replay #128

This change fixes several user-facing bugs:

• Enemy beacons can no longer be drag-selected
• Stealthed units can now be freely selected during replays / while observing
• Shrouded passengers / occupants of a partially visible open-contain object can no longer be drag-selected
• Singular enemy / neutral transport units containing occupants can now be drag-selected

And a few internal benefits:

• Passengers / occupants are no longer added to the selection info's DrawableList via drag-selections (e.g. selecting 10 full Battle Buses now only adds 10 objects to the draw list instead of 80)
• Fewer allocations and conditional branches

The original fix by Kris to prevent drag-selecting stealth / shrouded units was a bit of a complicated band-aid fix that also resulted in stealthed units being unselectable during replays / while observing.

[xezon]

I suspect this code here is fixing what we refer to in Generals as "box trick" aka selecting objects in the fog?

[Stubbjax]

That is correct.

[xezon]

With this condition gone, does it mean that we can now select enemy stealth units, while they are stealth? Or is that covered by isDrawableEffectivelyHidden ?

If so, then this fix needs to be different, for example allowing the observer to skip these tests.

[Stubbjax]

Absolutely not, or this change would be a regression. The original stealth selection condition is now covered by isDrawableEffectivelyHidden. The benefit to this is that as observers can see stealthed units and beacons, isDrawableEffectivelyHidden returns false for them (unless the units are in a closed container).

NO_STEALTH.mp4

OBSERVE_STEALTH.mp4

[xezon]

Looks good.

Then I expect this is a Zero Hour specific change for now?

[Stubbjax]

Looks good.

Then I expect this is a Zero Hour specific change for now?

Do you think it is best to separate the same fix for Generals, considering it has a different / larger impact there? I understood "box trick" to be a popular exploit in Generals, so it could be controversial.

[xezon]

It could be unified with

#if !RTS_GENERALS
  the fix here
#endif

then it keeps the bug in Generals. Not sure if Generals players are as protective about their exploits as Zero Hour players are.

Can be separate change.

[xezon]

Shrouded passengers / occupants of a partially visible open-contain object can no longer be drag-selected

Does this refer to the bug where players drag select a cargo unit and then can tell if it is empty or not by its selection status?

[Stubbjax]

Shrouded passengers / occupants of a partially visible open-contain object can no longer be drag-selected

Does this refer to the bug where players drag select a cargo unit and then can tell if it is empty or not by its selection status?

It refers to previously being able to select this area (a shrouded Missile Defender) and the selection propagating to the container (the Firebase). This was because the open container selection propagation logic came before the original shroud condition bug fix.

You do raise a good point though. I tested the 'detect cargo' exploit and it also appears fixed as a result of this change. This is likely because the occupants are no longer being selected as they were previously, which led to the group selection exiting early due to multiple enemy units in the selection (selectEnemies would never be set to TRUE; see the condition block below).

else if (si.newCountEnemies == 1)
{
  addToGroup = FALSE;
  si.selectEnemies = TRUE;
}

[xezon]

Are we concerned about this cargo select exploit fix? I think top level players will notice.

It refers to this report:

• Pulling selection box around non owned vehicle with passenger slots will yield different feedback depending on taken passenger count #119

[xezon]

I think it is unlikely that we will get a feedback on this exploit/bug fix from the entire community so to avoid distress I suggest we make the bug explicit and make its fix opt-in. This is stupid but we have learned over the years that some players are not yet ready for such kind of fixes and prefer to play with the bugs.

So have something like (pseudo)

if (!TheGlobalData->m_fixUnitCargoSelectionBug && selection->hasPassengers())
  doSelection()

Just so that the original exploit/bug is technically still available and then our data can opt-in the fix.

In the future we will have game setup profiles and so then the pro players who like these kinds of bugs can keep the bug in their setup and the other players that like the fixed game experience can use the fixed version. This will not be the last code exploit/bug fix that we would treat like this.

[xezon]

Thinking it further, I think we need to disable the fix for the retail compatible build anyway, otherwise players on this build have a technical disadvantage against retail game clients.

[Stubbjax]

Are we concerned about this cargo select exploit fix? I think top level players will notice.

I think it is unlikely that we will get a feedback on this exploit/bug fix from the entire community so to avoid distress I suggest we make the bug explicit and make its fix opt-in.

I'm not too concerned and I've abused it many times. Let's not overestimate the impact of this exploit. An extreme minority of top-level players might be temporarily unhappy if/when they notice its absence but I'm confident such awareness will be gradual and it'll be soon accepted and forgotten.

This is stupid but we have learned over the years that some players are not yet ready for such kind of fixes and prefer to play with the bugs.

It is very stupid. 😄 However, one of the benefits of initially maintaining retail compatibility is that we can fix these kinds of bugs without causing distress for those particular players, who have time to adapt and provide feedback, and can still play on the retail executable if they prefer.

Let's not forget, these fixes do not have to be set in stone. If we merge this and there does end up being enough pushback then we can look at explicitly reimplementing the exploit. I don't mind the idea of an opt-in EnableOriginalBugs option as I've seen in several other game restoration projects and remasters, especially once retail compatibility is dropped to further ease the transition.

Thinking it further, I think we need to disable the fix for the retail compatible build anyway, otherwise players on this build have a technical disadvantage against retail game clients.

It's worth noting that several of the fixes / improvements applied to this build already provide technical advantages over the retail game client. If anything, now is the perfect time to apply these kinds of fixes, when they are easier to accept due to the accompaniment of various advantages.

[xezon]

The main issue is that it triggers bad PR because top level players will use these kinds of fixes to discredit the whole project. And other players listen to what the top players say, so it carries some weight. That is why I think it is generally a good idea to retain a bugged variant of the game that these top players can use and the rest of the players can play the fixed version of the game without any interference. And it would be easy for both groups to access both these game variants. But for that we need to make controversial bugs/exploits individually configurable.

[Stubbjax]

The main issue is that it triggers bad PR because top level players will use these kinds of fixes to discredit the whole project. And other players listen to what the top players say, so it carries some weight. That is why I think it is generally a good idea to retain a bugged variant of the game that these top players can use and the rest of the players can play the fixed version of the game without any interference. And it would be easy for both groups to access both these game variants. But for that we need to make controversial bugs/exploits individually configurable.

I'm skeptical that there are top-level players chomping at the bit to discredit the whole project despite all of the great advancements achieved so far and on the horizon - especially over such an insignificant and rarely-used exploit. If anything, they'd surely discredit themselves by making a fuss over it.

I'm certain that nothing bad would come of this fix being merged; and reversion or amendment are always options if necessary. However, if we absolutely must remain beholden to the anticipated opinions of a few top-level players, then we can revisit this if/when such opinions have been expressed.

[xezon]

My recommendation for this is to make the bug at least compile time configurable, and enable it for RETAIL_COMPATIBLE_BUG. Retail players can use this bug to tell which terror technical or humvee or bus is empty. So I think to keep fairness, the same bug needs to be kept for our Retail compatible release.

[Mauller]

Needs replicating to generals

Needs to be configurable on compile time and in GlobalData

[xezon]

Produced by ArcticDolphin

[Stubbjax]

My recommendation for this is to make the bug at least compile time configurable, and enable it for RETAIL_COMPATIBLE_BUG.

Done.

DEJECTION.mp4

Produced by ArcticDolphin

That is rather unfortunate. A player's ability to make decisions with less information is arguably more skillful. It goes against the game's design and the core principle of reconnaissance for opponents to have such information.

We could also just show passenger info to opponents. It's effectively the same and the enhanced accessibility would diminish the viability of decoy strategies and curtail gameplay depth even further.

Needs replicating to generals

Are we also concerned about eliminating the much more egregious "box trick"? I suspect being able to select stealthed units and objects through fog must help the pros with fast decision-making to an even greater extent.

[xezon]

User AnT1_Pr0 reported that users Excal, Dominator, Boycah say this bug does not need to be preserved.

[L3-M]

AnT1_Pr0:

Take this as confirmation that the pro scene is happy to fix this bug

[Mauller]

This can be updated again to remove the explicit reintroduction of the bugged behaviour if we are going along the lines of just fixing the behaviour now.

Also wants replicating to generals.

[xezon]

This can be updated again to remove the explicit reintroduction of the bugged behaviour if we are going along the lines of just fixing the behaviour now.

For Retail compatible we can preserve the bugs that do give an advantage, because otherwise Retail game clients do have a competitive edge.

[xezon]

Also wants replicating to generals.

I assume Generals code is a bit different to Zero Hour because of the extra bugs. Perhaps treat it in a follow up change?

[Stubbjax]

For Retail compatible we can preserve the bugs that do give an advantage, because otherwise Retail game clients do have a competitive edge.

Are we going to ignore the changes so far that give an advantage?

I assume Generals code is a bit different to Zero Hour because of the extra bugs. Perhaps treat it in a follow up change?

It's actually the same fix, it just fixes a lot more as the "box trick" exploit still exists there. I can add the fix under the same RETAIL_COMPATIBLE_BUG condition if desired?

[xezon]

Are we going to ignore the changes so far that give an advantage?

Which ones are these?

It's actually the same fix, it just fixes a lot more as the "box trick" exploit still exists there. I can add the fix under the same RETAIL_COMPATIBLE_BUG condition if desired?

Yes can do.

[Stubbjax]

Are we going to ignore the changes so far that give an advantage?

Which ones are these?

Idle worker selection, building snapping, building cancellation, dead units blocking commands, etc.

[xezon]

Idle worker selection, building snapping, building cancellation, dead units blocking commands, etc.

The things you listed are minor impact. Personally I am not as concerned about the advantages of the update, albeit someone could classify this as cheating.

[xezon]

I think this change needs a rebase? Because of GameDefines.h?

[Stubbjax]

The things you listed are minor impact. Personally I am not as concerned about the advantages of the update, albeit someone could classify this as cheating.

Fair enough. I still think the collective and ever-increasing advantage - which already makes a difference in most if not all games - still outweighs the relatively minor impact of fixing this exploit.

I think this change needs a rebase? Because of GameDefines.h?

Is it blocking the merge? If it's the same change from two different sources then it should be fine.

[MrS-ibra]

This PR is causing a bug where if you put an infantry unit inside a garrison, humvee or a building for example, then try to put another infantry unit it will deselect the infantry and select the humvee instead of garrisoning it. This is also true for tunnels, if there is one unit inside the tunnel no other unit can get in. EDIT: this has been already reported