[GEN][ZH] Fix object deselection logic in BuildAssistant::sellObject()

[Mauller]

• Relates To: [GEN][ZH] Fix game crash in AIGroup::groupSell() #1200

This PR fixes the ordering of deselection logic within the build assistants sellObject() function.

I came across this while resolving #1200 and it corrects the object handling for removing the required object from the current selected objects.

In the original ordering, the object being sold was not being removed from it's original AiGroup and put into a new AIGroup within Player::getCurrentSelectionAsAIGroup() when it was called by GameLogic::deselectObject().

This is because getCurrentSelectionAsAiGroup tests if an object is selectable before placing it in an AiGroup.

This meant that subsequent code within GameLogic::deselectObject() was not working as intended.

At the moment there should not be any user facing changes with this.

TODO

• Test against many Replays

[xezon]

In the original ordering, the object being sold was not being removed from it's original AiGroup and put into a new AIGroup within Player::getCurrentSelectionAsAIGroup() when it was called by GameLogic::deselectObject().

This is because getCurrentSelectionAsAiGroup tests if an object is selectable before placing it in an AiGroup.

This meant that subsequent code within GameLogic::deselectObject() was not working as intended.

I dont fully understand this. What do the AIGroups look like before and what do they look like afterwards?

Also if the AIGroup membership changes now, then this will mismatch with the Retail version, right?

[Mauller]

In the original ordering, the object being sold was not being removed from it's original AiGroup and put into a new AIGroup within Player::getCurrentSelectionAsAIGroup() when it was called by GameLogic::deselectObject().
This is because getCurrentSelectionAsAiGroup tests if an object is selectable before placing it in an AiGroup.
This meant that subsequent code within GameLogic::deselectObject() was not working as intended.

I dont fully understand this. What do the AIGroups look like before and what do they look like afterwards?

Also if the AIGroup membership changes now, then this will mismatch with the Retail version, right?

Before the change the object deselect is being called on remains in the current group it is a part of, if it was within one.
After the change it gets properly transfered to and then deleted from the new AIGroup that is created when updating the current selection.

This showed up in the groupSell as the first object on the list remaining in the member list on the calling AIGroup while everything else got removed. Since the other objects got transfered to the new AIGroup and set as the currently selected objects / buildings etc.

This is more about making the code function as it was intended, the way this gets called in game by players means the AIGroups were already being cleaned up during a game. And the subsequent code only functioned by chance and not by intention.

[xezon]

This needs to be tested for Retail mismatching.

[roossienb]

This needs to be tested for Retail mismatching.

I'll pick up the testing against replays

[roossienb]

Code looks good

Tested against 1,000 replays without issues.

[xezon]

I do not quite understand what this change is supposed to fix. I sold single buildings, stepped through the code, and saw no difference in behaviour with and without this change.

Does this only concern logic that would sell multiple buildings at once? If yes, how? And was this tested for Mismatch? As far as I am aware in regular play it is not possible to select and sell multiple buildings with one action.

Blocking merge until situation is clear.

[Mauller]

I do not quite understand what this change is supposed to fix. I sold single buildings, stepped through the code, and saw no difference in behaviour with and without this change.

Does this only concern logic that would sell multiple buildings at once? If yes, how? And was this tested for Mismatch? As far as I am aware in regular play it is not possible to select and sell multiple buildings with one action.

Blocking merge until situation is clear.

So the change allows the correct behaviour of object deselection to occur within GameLogic::deselectObject when deselecting a building that is being sold.

When the object is set to be unselectable, the call to Player::getCurrentSelectionAsAIGroup -> Squad::aiGroupFromSquad does not remove the object the deselection was called on from its original AIGroup.

This occurs because Squad::getLiveObjects, that is called within aiGroupFromSquad, filters based on if an object is selectable or not. If the object is not selectable it leaves the object in it's prior AIgroup. Which when using groupSell, it shows up as the object remaining in the callees AIGroup member list when it should have been removed from it.

Once the logic returns back into GameLogic::deselectObject the AIGroup it is acting on implicitly lacks the object that deselectObject was called for, so the following essentially doesn't do what it was meant to be doing.

if (group) {
    Bool emptied = group->remove(obj);

The code only worked by happenstance as the original AIGroup the building belonged to would get deleted in the command handling function that GroupSell was being called on in this case. And if other objects were part of the selection they would be in the new AIGroup to be set as the current selection.

It also removes the object ID for the building being sold from the current selection cache.

This change is more about making sure the code is doing what it was meant to so that when we make future changes we don't come across unexpected behaviour.

[xezon]

Can you perhaps give repro steps how the wrong behavior can be triggered?

I stepped through the code on single object selling and the behaviour was the exact same.

[Mauller]

Can you perhaps give repro steps how the wrong behavior can be triggered?

I stepped through the code on single object selling and the behaviour was the exact same.

Just sell a building, breakpoint inside the GameLogic::deselectObject just before the call to Player::getCurrentSelectionAsAIGroup then check back up the call stack and check the status of m_memberlist within the call to groupSell it should contain the building being sold.

In the incorrect behavour, the moment you step over Player::getCurrentSelectionAsAIGroup the building will still be in the m_memberlist and the AIGroup returned from Player::getCurrentSelectionAsAIGroup will be empty or destroyed.

in the corrected behaviour, the object should be removed from the m_memberlist and be contained in the AIGroup returned from Player::getCurrentSelectionAsAIGroup, the call Bool emptied = group->remove(obj); further on within GameLogic::deselectObject will then remove the object and set emptied to true.

Externally the behaviour appears to be the same since beyond the deselect function the object being sold is no longer in the AIGroup that overwrites the current selection, but from what i remember the player cached selection can still contain the ID of the building that was being sold. you can see the cached selection if you go down the rabbit hole of what is going on within Player::getCurrentSelectionAsAIGroup.

[xezon]

In the incorrect behavour, the moment you step over Player::getCurrentSelectionAsAIGroup the building will still be in the m_memberlist and the AIGroup returned from Player::getCurrentSelectionAsAIGroup will be empty or destroyed.

in the corrected behaviour, the object should be removed from the m_memberlist and be contained in the AIGroup returned from Player::getCurrentSelectionAsAIGroup, the call Bool emptied = group->remove(obj); further on within GameLogic::deselectObject will then remove the object and set emptied to true.

I tested this and cannot confirm this observation. m_memberlist is 1 and new group is 0 in both scenarios.

If what you described was true, then I would expect retail mismatch, because any difference in AIGroup lifetime will cause mismatch.

[Mauller]

In the incorrect behavour, the moment you step over Player::getCurrentSelectionAsAIGroup the building will still be in the m_memberlist and the AIGroup returned from Player::getCurrentSelectionAsAIGroup will be empty or destroyed.
in the corrected behaviour, the object should be removed from the m_memberlist and be contained in the AIGroup returned from Player::getCurrentSelectionAsAIGroup, the call Bool emptied = group->remove(obj); further on within GameLogic::deselectObject will then remove the object and set emptied to true.

I tested this and cannot confirm this observation. m_memberlist is 1 and new group is 0 in both scenarios.

If what you described was true, then I would expect retail mismatch, because any difference in AIGroup lifetime will cause mismatch.

I think it's memory manager shenanigans, if you run without the memory manager you see the behaviour and the game will crash due to the original group being destroyed and not overwritten yet.

I always tend to run under debug and without the memory manager since it catches far more problems.

Mismatching also won't be an issue in this instance since the groups are updated and destroyed within the same frame under both scenarios.

[xezon]

I think it's memory manager shenanigans, if you run without the memory manager you see the behaviour and the game will crash due to the original group being destroyed and not overwritten yet.

I always tend to run under debug and without the memory manager since it catches far more problems.

Mismatching also won't be an issue in this instance since the groups are updated and destroyed within the same frame under both scenarios.

I have tested this with Null Game Memory and the behavior is still the same. There is no crash on Sell either. Can you please test again and verify that your initial observation is correct? If it is, I suggest we do a screen sharing and you show me. Because I cannot reproduce this issue at all.