View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0004659 | Spring engine | General | public | 2015-01-30 21:15 | 2015-12-30 12:40 |
| Reporter | silentwings | Assigned To | Kloot | ||
| Priority | normal | Severity | major | Reproducibility | always |
| Status | resolved | Resolution | fixed | ||
| Product Version | 98.0 | ||||
| Fixed in Version | 100.0+git | ||||
| Summary | 0004659: gadgetHandler miscounting with RemoveGadget | ||||
| Description | This 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 Reproduce | Drop 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. | ||||
| Tags | No tags attached. | ||||
| Attached Files | |||||
| Checked infolog.txt for Errors | |||||
|
|
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) |
|
|
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. |
|
|
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 :-| |
|
|
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). |
|
|
I am amazed that this never caused problems before; would like to try and understand why not. |
|
|
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 |
|
|
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 |
|
|
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. |
|
|
Yeah, I saw the lus bug, I think that indeed may work. I'll try to refactor the handlers and submit a pull request. |
|
|
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. |
|
|
how will removal change ordering? (assuming a gadget only removes itself) can you give an example? |
|
|
It's what would happen if you tried to be creative and did swap(t[i], t[#t]) to satisfy the no-holes requirement. |
|
|
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). |
|
|
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. |
|
|
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. |
| 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 | |
| 2015-02-03 10:56 | silentwings | Note Added: 0013983 | |
| 2015-02-03 10:58 | silentwings | Note Edited: 0013983 | |
| 2015-02-03 11:21 | silentwings | Note Edited: 0013983 | |
| 2015-02-03 11:23 | silentwings | Note Edited: 0013983 | |
| 2015-02-03 11:23 | silentwings | Note Edited: 0013983 | |
| 2015-02-03 11:27 | silentwings | Note Edited: 0013983 | |
| 2015-02-03 11:29 | silentwings | Note Edited: 0013983 | |
| 2015-02-03 11:31 | silentwings | Note Edited: 0013983 | |
| 2015-02-03 18:06 | abma | Note Added: 0013984 | |
| 2015-02-03 18:06 | abma | Note Edited: 0013984 | |
| 2015-02-03 18:07 | abma | Note Edited: 0013984 | |
| 2015-02-03 18:07 | abma | Note Edited: 0013984 | |
| 2015-02-03 18:07 | abma | Note Edited: 0013984 | |
| 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 | |
| 2015-05-21 00:43 | hokomoko | File Added: remover2.lua | |
| 2015-05-21 00:43 | hokomoko | Note Edited: 0014473 | |
| 2015-05-21 00:44 | hokomoko | Note Edited: 0014473 | |
| 2015-05-21 01:23 | hokomoko | Note Edited: 0014473 | |
| 2015-05-22 15:57 | Rafal99 | Note Added: 0014474 | |
| 2015-05-22 16:02 | Rafal99 | Note Edited: 0014474 | |
| 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 | |
| 2015-07-01 09:32 | silentwings | Note Edited: 0014818 | |
| 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 |
