Some commands get modified by a simulation update

[elexis]

Some actions in Commands.js modify the commands. This is usually not an issue, but can cause false positives OOS in serializationtest and might fail oddly somewhere down the line.

r17324 was a fix for one such case. There are possibly others.

A fix would be making sure we detect those problems (at least when running serializationtest), and fixing the cases where it happens.

[elexis]

Fixes the bug, but should be optimized performance-wise as this patch initializes the secondary context and component manager in non-serializationtestmode too.

[historic_bruno]

Why is the command modified in the first place? That seems undesirable.

[wraitii]

I don't think your fix is the proper solution here (it would be handled better as part of a complete overhaul of the system imo, such as the RM fix).

The command change is indeed a bug in TryConstructWall where instead of locally modifying a boolean we modify the command "queued" parameter. That should be changed.

[elexis]

Fixed alternatively by r17324.

[historic_bruno]

It's an interesting thing to note however, perhaps somewhere in the docs, in case a similar bug pops up in the future.

I had a peek at TryConstructBuilding and it modifies cmd.angle for docks, so we may need to fix that as well: ps/trunk/binaries/data/mods/public/simulation/helpers/Commands.js?rev=17324#L890​ Does that cause serialization test failure?

[elexis]

Replying to historic_bruno:

we may need to fix that as well

An attack order also changes the command object: cmd.allowCapture = true;

Even if this and the dock doesn't OOS, it is just a matter of time until we will run into this issue again. We should fix the serializationtest as it's the only place where we can guarantee that the false-positive serializationtesterror won't repeat.

[historic_bruno]

Alternatively, the serialization test is working correctly and detecting broken logic, which we would only be covering up by changing it :)

[wraitii]

I agree with historic_bruno. My suggestion to fix it would be custom-checking in the serialization test that the commands don't change, and if they change, crash with an obvious message and perhaps the changed command to make debugging easier.

We also should fix the broken logic in the places you mentioned.