Implement a waiting command (#6884)

[ghost]

This is for the feature request #6884. It's a proof-of-concept for the first alternative outlined there. You can now "postpone" decisions for the current unit using key Z (since W is taken, I used Z for "zzz..."). Unit for which decisions are postponed this way will reappear on this turn later when you use "Next unit".

Thoughts: maybe better to move it from unit action area to the place just below "Next unit".

[Azzurite]

Hey there :) welcome to the project, glad to have you!

[ghost]

This commit should address all issues in the review. I will squash them later if necessary. I resolved clear-cut conversation, but left open those where I didn't follow the advises exactly.

[doublep]

Since I failed to properly separate with my main account anyway, I deleted the secondary and will just use the main instead.

[SomeTroglodyte]

Hey, welcome! I'll need time to really look at this, just two notes from reading mail:

• "Zzzz" - perfect choice!
  Just be aware that since the lwjgl3 migration localized keyboard layouts are broken, frogs will have to press "W" for a Input.Keys.Z, and krauts a "Y"... Haven't found a solution so far. Except reinventing the wheel.

• Place "just below next turn" uuhhhh - I'd prefer not. We have enough actors floating already. UnitAction's are cleanly extensible and at the moment grouped into 2 "pages" - but - there's a hidden mini-project I never finished behind this. Idea: Allow a more flexible "collection" of all possible actions, and dynamically distribute onto pages. Effect 1: Keyboard for "other" pages could work without first switching to them. Next: E.g. Workers - integrate with improvement picker in such a way that a) improvent building actions become available directly as UnitAction without going through the PickerScreen. Effect: Workers and Legionaries can now always build Roads with the "R" key. Next: Intelligent priorities, so when there's no point in the full PickerScreen the "I" action disappears in a later page or entirely and the only available improvement is moved to the first "page"... If any of that were realized, "page 2" would be the logical place because desktop players will use the "Z" key anyway.

[Azzurite]

frogs krauts

I don't think these words in this context are really appropriate anymore ;)

[SomeTroglodyte]

You're only allowed to throw stones if you're sitting inside the glass house yourself. But if you are, it's fun!

[Azzurite]

Haven't found a solution so far

How about using keyTyped instead of keyDown?^^ Gives a char instead of a key code.

[SomeTroglodyte]

How about using keyTyped instead of keyDown

Look it up in blame - that was the original implementation way back until someone with a cyrillic keyboard convinced us otherwise. Of course, back then with the lwjgl2 backend, keyDown was still layout-mapped. Now, I don't even see a way to reflect down into Gdx to get access to the mapping stuff directly...

[Azzurite]

So I looked at this again, more in-depth...

The problem I have is the initialization of the dueUnits list... doing getDueUnits() just to get the side effect of adding all units to the list seems like a clutch.

And I noticed one thing: the dueUnits list contains exactly the same units as the units field.

So how about, instead of duplicating the list, remove from beginning, add at the end, we instead change dueUnits from a MutableList<MapUnit> to just an Int with default -1 - a pointer into the unit list? (and rename to curDueUnitIdx or some such)

Then the cycleThroughDueUnits function would look something like this:

fun cycleThroughDueUnits(unitToSkip: MapUnit?): MapUnit? {
    if (units.none() || getDueUnits().none()) return null

    var curUnit: MapUnit
    do {
        if (curDueUnitIdx == -1 || curDueUnitIdx >= (units.size - 1)) {
            curDueUnitIdx = 0
        } else {
            curDueUnitIdx++
        }
        curUnit = units[curDueUnitIdx]
    } while (curUnit == unitToSkip || !curUnit.due || !curUnit.isIdle())

    return curUnit
}

and the only thing we need to do to keep the pointer in sync is a if (curDueUnitIdx != -1 && removedIdx < curDueUnitIdx) curDueUnit -= 1 instead of dueUnits.remove(mapUnit).

getDueUnits would then be side-effect-free, since you can just do

fun getDueUnits(): List<MapUnit> {
    return units.filter { it.due && it.isIdle() }
}

It changes some work/complexity/performance from keeping the lists in sync & initialized to a bit of array index logic. I think that's a fine trade-off.

It's fine either way, but it'd be cool if you could get rid of the "getDueUnits for side-effects" clutch if you keep the second list... maybe use a small helper class with add+remove helpers.

[Azzurite]

This needs to be above addToggleActionsAction (what a name...) because that is basically our "Next Page" button, and a "Next Page" button does not make sense in the middle of a page, but should be at the bottom.

[doublep]

Done.

[SomeTroglodyte]

more in-depth

Ah, yes, LIFO or Cursor or... How about a virtual ZIndex on the unit itself? Could be made to work... The problem with SQL cursors is they basically need a strict isolation level meaning lock preventing access or be completely immune against fetch fails, against the same record coming along repeatedly, or against missing rows. Translated - a pointer can get out of sync with the structure it's pointing into unless it's part of that structure's implementation... Some of that you have addressed. More would be 'we're using knowledge about how that collection is done now' - fine but someone thinking "Optimization! Let's use a Set instead!" would then break our cursor...

Don't get me wrong - I'm not advocating a specific approach, just throwing bones to the dogs. I'll leave the choice to "ghost", and if his solution works...

Ah, while I'm playing devil's advocate: Is the LIFO holding on to references longer than before a potential problem? I didn't really look.

[Azzurite]

"Optimization! Let's use a Set instead!" would then break our cursor

It would, but not without the compiler warning that you can't access a Set by index.

[Azzurite]

It's fine either way, but it'd be cool if you could get rid of the "getDueUnits for side-effects" clutch if you keep the second list... maybe use a small helper class with add+remove helpers.

And just so you know how it works in this project, these are all suggestions unless you get a red "Changes Requested" review. If you say "nah I don't have time for this/I want to leave it like this" we can also get this merged. This small thing is not a dealbreaker.

I just at least want to get a response to my suggestions before I approve :D

And another thing: you might've noticed we don't always keep it on-topic. Just ignore irrelevant comments.

[doublep]

Reimplemented without separate list. However, now addUnit() and removeUnit() rotate units list: it still contains what it should, just in a somewhat different order. Is that fine? If not, this can be reimplemented (yet again). I'm also not sure why it creates new lists to begin with, but oh well.

From personal observations: would be nice if "Next unit" also would be trigerred by key Z, but only if no unit is currently selected.

[SomeTroglodyte]

Sorry for the continued teasing, but - getDueUnits() was Sequence<MapUnit> and now... A materialized copy. Big difference. Then a little "Nex" where "Next" is meant - and I see no "why" to the reordering (but that's a case of 'they'll likely know why' for me).

OK, getDueUnits is bad design anyway. May not have been historically, but at the moment there's only two callers, and both only iterate to the first hit : first and any. (( And the 9-liner getNextDueUnit could be just fun getNextDueUnit() = getDueUnits().firstOrNull()?.apply { due = false } - not necessarily your business though)) So all you/we need is its firstOrNull.

Look at what getDueUnit is: units.asSequence().filter { it.due && it.isIdle() }. Now we only need firstOrNull. Which amounts to an Iterable scan (units.asSequence uses Iterable.asSequence - can't do better than to iterate on a List), stop if the predicate is fulfilled, and recognize the Iterable ending to return null. Using your nextPotentiallyDueAt, how could we "just" iterate from there to the end, roll over start from beginning, and give up where we started? Sequence.drop(n) + Sequence.take(n) would probably do it, but is drop efficient on a List? Don't know. take is. drop can be as efficient as [n]... Which for a List is? Depends on which implementation of List is used. Which is actually controlled by addUnit and removeUnit. And it's an ArrayList, promising efficient indexing, hopefully. So that scan could just be written as for loop, which would effectively do the same as the sequence terminator in the drop+take idea and run just as efficient - but you save the overhead of sequence creation.

Sorry for rambling. Not even sure I'm right. But your solution creating at least two clones where before there was none, that's true. Maybe Azzurite can explain better.

"Z"="N" if no selection: Yup, had the same idea when I played your initial version!

[Azzurite]

Looking good :) unfortunately I still have comments :D

I'm also not sure why it creates new lists to begin with

The only thing I can imagine is that there were some concurrent list modifications or some such, and the solution was to make copies instead of making it a synchronizedList? Not sure. I'd be fine with you changing it to direct modification. /shrug

[Azzurite]

is unused now, can be removed

[doublep]

Kept because it was used before. But OK, removed in the latest commit.

[Azzurite]

sequenceOf(sublist1, sublist2).flatten() (or sublist1.asSequence() + sublist2.asSequence(), whatever you prefer :D) should make this a sequence again and not materialize a new list. Otherwise this is unfortunately worse than your previous approach, as getDueUnits() would be creating a new list each time it is called :D

But if it is a sequence, all getDueUnits().any() and such calls should short-circuit nicely.

[Azzurite]

also NexDue -> NextDue

[doublep]

Renamed.

[Azzurite]

your solution creating at least two clones where before there was none

1 clone, as sublist is only a view on the list. The clone that happens is actually the +. But see my review comment. If it's changed to sequenceOf(sublist1, sublist2).flatten() there's only constant time difference to the previous implementation, which is perfectly fine.

[SomeTroglodyte]

can imagine is that there were some concurrent list modifications

As commented on the units field.

The clone that happens is actually the +.

Wasn't counting the sublist - cuz then a filter gets called on the plus result... right?

[Azzurite]

Wasn't counting the sublist - cuz then a filter gets called on the plus result... right?

I mean then in total it's actually 4, because getDueUnits is used 3 times + 1 filter.

[doublep]

I think you guys are busy with wrong things. Who cares about performance of a max-50 items (in reality even fewer) list in user interface? It would matter if it was 1000+ items, or called used thousands of times per second. The only maybe performance-relevant place are addUnit() and removeUnit() here: I don't know how AI works, maybe it performs some lookahead for "what could possibly happen next" and invokes these many (hundreds, thousands, etc.) times.

Your "if it is a sequence, all getDueUnits().any() and such calls should short-circuit nicely" etc. made it necessary for addUnit() and removeUnit() to create new lists. I have verified that reusing lists, i.e. just adding/removing items does cause concurrent modification exceptions somewhere in AI code, probably when unit list gets modified while some iteration is still going on. At this point I'm not even sure if it's worth it and if working with plain stupid materialized lists wouldn't have been a tad faster and also easier for users. Also, keep in mind that sequences (I'm presuming this corresponds to Streams in Java, I have never used Kotlin before) do have some overhead, and using them for 10-20-30-item-lists all the time might even have negative effect on performance (which in any case is too small to matter here).

And the 9-liner getNextDueUnit could be just fun getNextDueUnit() = getDueUnits().firstOrNull()?.apply { due = false }

Actually, no, it also updates nextPotentiallyDueAt and possibly skips a unit. But yes, it could be simpler, by indirectly looping multiple times, e.g. to find index of the found unit in the list. But then again, make up your mind. Do you strive for maximum performance even where it hardly matters, or for simplicity and readability? In the latter case the function could be reimplemented, yes. I, personally, think the main goal should be functionality, with simplicity/readability taking the second place. Performance is usually not to be bothered about unless something really turns out to be too slow.

Anyway, I think I'm done with this. Do with it what you want: merge as-is, improve, refactor or throw away. I'm willing to do only functionality fixes/improvements and fixes for real problems from now on, not these peanuts.

[Azzurite]

I think you guys are busy with wrong things. Who cares about performance of a max-50 items (in reality even fewer) list in user interface?
...
Anyway, I think I'm done with this. Do with it what you want: merge as-is, improve, refactor or throw away. I'm willing to do only functionality fixes/improvements and fixes for real problems from now on, not these peanuts.

I tend to agree with that. Sometimes me + SomeTroglodyte lose ourselves in "pointless" discussion, but we're having fun doing it. But why do you only say that now? From what you wrote here, it seems like you thought that from the very beginning, but didn't say it, and let yourself get annoyed having to "fix" things that you didn't think were necessary. If you thought that, you should've just said so and we could have gotten this merged, because do you remember this comment of mine:

And just so you know how it works in this project, these are all suggestions unless you get a red "Changes Requested" review. If you say "nah I don't have time for this/I want to leave it like this" we can also get this merged

This was always an option. None of the things we said were ever requirements to get this merged. The requirement to get this merged is that it works, which it did from the very beginning. We just had some suggestions that all were optional and you could've said "no, I don't think this is worth it" to.

[SomeTroglodyte]

In a medium-sized game, 2 turns amounted to 709 calls of the cloning code (the one that was a straight copy before and now splices and resorts) averaging 26.5µs per call.
Same turns, master, tried to do the same steps. 713 / 7.0µs. 14ms penalty distributed over a few minutes. And most of that could be shaved off by using the old code if !isPlayer().

My apologies for my role in how the communications degenerated from constructive to emotionally charged.