2020-10-29 08:16 CET

View Issue Details Jump to Notes ]
IDProjectCategoryView StatusLast Update
0004659Spring engineGeneralpublic2015-12-30 12:40
Reportersilentwings 
Assigned ToKloot 
PrioritynormalSeveritymajorReproducibilityalways
StatusresolvedResolutionfixed 
Product Version98.0 
Target VersionFixed in Version100.0+git 
Summary0004659: gadgetHandler miscounting with RemoveGadget
DescriptionThis is slightly complicated.

Suppose two gadgets A and B both have a (say, synced) callin C. No other gadget has (synced) callin C. Suppose C is called for A first, and that A unloads itself during C. Then C will not be called for B.

The problem is that RemoveGadget() has reduced the count of the list gadgetHandler.CsNameList by one, during the ipairs(...) loop which calls C for each gadget; with the result that the ipairs loop finishes without processing all items.

If other gadgets also share callin C the results can be quite complicated.
Steps To ReproduceDrop the two attached gadgets into the minimal A game (from FLOZi's ABC). They will demonstrate the above for the (synced) GameStart callin.

The first gadget will say "GameStart 1" and then unload itself. As a consequence GameStart will not be called for the second gadget, which will not say "GameStart 2".

If you then comment out the RemoveGadget() from within the GameStart() of the first gadget, the second will have it's GameStart() called.
TagsNo tags attached.
Checked infolog.txt for Errors
Attached Files

-Relationships
has duplicate 0001907resolvedKloot remove gadget/widget will block calls for other gadgets/widgets 
+Relationships

-Notes

~0013982

abma (administrator)

Last edited: 2015-02-03 10:50

View 2 revisions

doesn't this affect all callins?

minimal lua example, but this works:

list = { "a", "b", "c", "d"}

for i,v in ipairs(list) do
    if i == 1 then
        table.remove(list, 2)
    end
    print(i, v)
end

outputs:

1 a
2 c
3 d

(so sth. is missing here)

~0013983

silentwings (reporter)

Last edited: 2015-02-03 11:31

View 8 revisions

I haven't tested other callins, but I assume it's the same.

I think that you minimal example is wrong for the situation. The problem is that during the i==1 case of the loop, a function is called which removes the i==1 element from the table. So a correct minimal example would be:

code:

list = {"a","b"}
for i,v in ipairs(list) do
    print(i, v)
    if i == 1 then
        table.remove(list, 1)
    end
end

output:

1,a

The pair of gadgets above will do what the ticket says they do.

~0013984

abma (administrator)

Last edited: 2015-02-03 18:07

View 5 revisions

a, true, thats exactly the problem. (it should output 1,a 2,b)

hmm, only idea i currently have to fix the problem is to copy the array in the callin and then iterate over it, but this will impact speed :-|

~0013985

silentwings (reporter)

I'm not sure how to fix this without hurting speed either.

Another idea is that RemoveGadget (and RemoveCallin) cache up whats to be removed and then the cache is processed at the end of each CallinNameList (if it contains anything, which it almost always doesn't).

~0013986

silentwings (reporter)

I am amazed that this never caused problems before; would like to try and understand why not.

~0014042

Jools (reporter)

It has. People get around the issue with checking in other places, for example if TeadDied is not called you can check every 10 seconds in gameframe if team is alive.

But there are many cases where some callins are not reached, including TeamDied, GameOver and probably more

~0014473

hokomoko (developer)

Last edited: 2015-05-21 01:23

View 5 revisions

EDIT: please ignore/delete remover.lua, it's not working and has many bugs.
remover2.lua however works (mostly) as advertised.

RFC
I have a suggestion of a solution:
Instead of removing said widget, it should be possible to just replace all its callins with dummy functions returning true/false/whatever that doesn't affect the game.
Once every frame or every update, remove all widgets that need removal.

Example source as a standalone gadget is attached as remover2.lua, it handles the example two gadgets correctly

Logic can probably be incorporated into gadgetHandler and widgetHandler

~0014474

Rafal99 (reporter)

Last edited: 2015-05-22 16:02

View 2 revisions

There was quite a similar bug in Lua Unit Script framework once. I remember it was fixed by iterating the list in reverse, so even if you removed the current element, the next one to process would stil be at index i-1.

~0014475

hokomoko (developer)

Yeah, I saw the lus bug, I think that indeed may work.
I'll try to refactor the handlers and submit a pull request.

~0014476

Kloot (developer)

Iterating in reverse will *NOT* work here because the callin lists are ordered by gadget layer value and any removal may not change their ordering or leave holes.

The only proper solution is to change them from array tables to linked lists.

~0014477

hokomoko (developer)

how will removal change ordering? (assuming a gadget only removes itself)
can you give an example?

~0014478

Kloot (developer)

It's what would happen if you tried to be creative and did swap(t[i], t[#t]) to satisfy the no-holes requirement.

~0014479

hokomoko (developer)

The current code uses table.remove, hence it will work correctly if traversing in reverse (as long as we change ArrayInsert to build the lists in reverse).

~0014814

hokomoko (developer)

In Spring:1944 we currently use https://github.com/spring1944/spring1944/blob/master/LuaRules/Gadgets/remover.lua
and
https://github.com/spring1944/spring1944/blob/master/LuaUI/Widgets/remover.lua

which seem to work correctly if anyone wants to steal them.
They're slightly too hacky to be merged into the engine at the moment.

~0014818

silentwings (reporter)

Last edited: 2015-07-01 09:32

View 3 revisions

I think that a clean solution to this is something like: http://stackoverflow.com/a/12435347. I don't like how the implementation there has remove() being passed as 3rd arg on every iteration, but replacing ipairs with something saner, or maybe counting manually, seems like a proper solution to me.

+Notes

-Issue History
Date Modified Username Field Change
2015-01-30 21:15 silentwings New Issue
2015-01-30 21:15 silentwings File Added: game_start_1.lua
2015-01-30 21:15 silentwings File Added: game_start_2.lua
2015-02-03 10:46 abma Note Added: 0013982
2015-02-03 10:50 abma Note Edited: 0013982 View Revisions
2015-02-03 10:56 silentwings Note Added: 0013983
2015-02-03 10:58 silentwings Note Edited: 0013983 View Revisions
2015-02-03 11:21 silentwings Note Edited: 0013983 View Revisions
2015-02-03 11:23 silentwings Note Edited: 0013983 View Revisions
2015-02-03 11:23 silentwings Note Edited: 0013983 View Revisions
2015-02-03 11:27 silentwings Note Edited: 0013983 View Revisions
2015-02-03 11:29 silentwings Note Edited: 0013983 View Revisions
2015-02-03 11:31 silentwings Note Edited: 0013983 View Revisions
2015-02-03 18:06 abma Note Added: 0013984
2015-02-03 18:06 abma Note Edited: 0013984 View Revisions
2015-02-03 18:07 abma Note Edited: 0013984 View Revisions
2015-02-03 18:07 abma Note Edited: 0013984 View Revisions
2015-02-03 18:07 abma Note Edited: 0013984 View Revisions
2015-02-03 23:07 silentwings Note Added: 0013985
2015-02-03 23:08 silentwings Note Added: 0013986
2015-02-12 20:37 Jools Note Added: 0014042
2015-05-20 23:37 hokomoko File Added: remover.lua
2015-05-20 23:39 hokomoko Note Added: 0014473
2015-05-20 23:45 hokomoko Note Edited: 0014473 View Revisions
2015-05-21 00:43 hokomoko File Added: remover2.lua
2015-05-21 00:43 hokomoko Note Edited: 0014473 View Revisions
2015-05-21 00:44 hokomoko Note Edited: 0014473 View Revisions
2015-05-21 01:23 hokomoko Note Edited: 0014473 View Revisions
2015-05-22 15:57 Rafal99 Note Added: 0014474
2015-05-22 16:02 Rafal99 Note Edited: 0014474 View Revisions
2015-05-22 21:20 hokomoko Note Added: 0014475
2015-05-22 22:09 Kloot Note Added: 0014476
2015-05-22 22:54 hokomoko Note Added: 0014477
2015-05-22 23:12 Kloot Note Added: 0014478
2015-05-23 10:40 hokomoko Note Added: 0014479
2015-07-01 01:11 hokomoko File Deleted: remover2.lua
2015-07-01 01:11 hokomoko File Deleted: remover.lua
2015-07-01 01:13 hokomoko Note Added: 0014814
2015-07-01 09:20 silentwings Note Added: 0014818
2015-07-01 09:22 silentwings Note Edited: 0014818 View Revisions
2015-07-01 09:32 silentwings Note Edited: 0014818 View Revisions
2015-12-30 01:49 Kloot Status new => resolved
2015-12-30 01:49 Kloot Fixed in Version => 100.0+git
2015-12-30 01:49 Kloot Resolution open => fixed
2015-12-30 01:49 Kloot Assigned To => Kloot
2015-12-30 12:40 Kloot Relationship added has duplicate 0001907
+Issue History