#3647 closed defect (fixed)
[PATCH] Some commands get modified by a simulation update
Reported by: | elexis | Owned by: | leper |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 20 |
Component: | Core engine | Keywords: | patch |
Cc: | Patch: |
Description (last modified by )
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.
Attachments (1)
Change History (15)
by , 8 years ago
Attachment: | clone_commands_for_serializationtest_v1.patch added |
---|
comment:2 by , 8 years ago
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.
follow-up: 5 comment:4 by , 8 years ago
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?
comment:5 by , 8 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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.
comment:6 by , 8 years ago
Alternatively, the serialization test is working correctly and detecting broken logic, which we would only be covering up by changing it :)
comment:7 by , 8 years ago
Description: | modified (diff) |
---|---|
Keywords: | patch removed |
Priority: | Must Have → Should Have |
Summary: | [PATCH] Serializationtest error when placing walls → Some commands get modified by a simulation update |
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.
comment:10 by , 8 years ago
Keywords: | patch added |
---|---|
Summary: | Some commands get modified by a simulation update → [PATCH] Some commands get modified by a simulation update |
Two more places were identified where the command object was modified: 1) Non-visual replays: #3751. 2) The cheat "back to the future":
ERROR: JavaScript error: simulation/helpers/Cheat.js line 88 TypeError: "parameter" is read-only Cheat@simulation/helpers/Cheat.js:88:3 g_Commands.cheat@simulation/helpers/Commands.js:66:3 ProcessCommand@simulation/helpers/Commands.js:37:3 ERROR: Failed to call ProcessCommand() global script function
comment:13 by , 7 years ago
Description: | modified (diff) |
---|
Fixes the bug, but should be optimized performance-wise as this patch initializes the secondary context and component manager in non-serializationtestmode too.