Standardise TemporaryPopup usage, add invalid navigation popups

[ajhalme]

Following up on discussion in #871, I've had a go at adding popup notifications to invalid navigation by "non-combat units" as the siege units are referred to in the original game.

I cleaned up the popup label render so that the text offset calculation is done in one location instead at every call site. I added text centering and some transparency to the label, to make the popup a bit nicer and more like the original. (I made a clone of the StyleBox for tooltips, which work better without transparency.)

Implementing the actual invalid navigation detection proved a bit more challenging than I expected. The method CanEnterTile(..) is used in different contexts to check for slightly different things, which made adding side effects a bit tricky. A naive implementation would make it so that the pathing and AI calculations would also raise popups. I ended up generalizing CanEnterTile(..) with an additional parameter to enable raising warnings when navigating from the move(..) context. The whole CanEnterTile(..) pattern probably needs a bit more thought - passing in boolean flags to change the behaviour of the function is a bit of a code smell.

Finally, while adjusting TemporaryPopup usages, I noticed that the existing MsgShowTemporaryPopup call at MaybeAwardForestClearingShields(..) was being called even during non-human turns, resulting in seemingly pointless popup renders somewhere out of camera, deep in other player's territory. I've changed this so shield value popups are rendered only for human players. I want to say that this change had an observable impact on next turn load performance, but I didn't measure.

Screenshots

Original game

Existing popup mechanism

Revised popup mechanism

[TomWerner]

Thanks for the contribution!

I tested this out with the "quick start" game and walked my worker down 9ish tiles to the aztecs. When I tried to attack their city or the worker a war declaration popup was shown, and I accepted it. The popups you added then showed up correctly, which was cool!

However I wasn't able to get them to show up again after that. Similarly if I declared war first then tried attacking with my worker the popup didn't show. Is there something wrong with the probing logic?

[stavrosfa]

Thanks for this!

A few things we can consider for the future that are beyond the scope of this PR.

1. When I click multiple times I get multiple overlapping pop ups, without really being able to see them, except for the top one. Maybe we can only keep one pop up per tile at any given time.
2. We should not be able to declare war with workers or artillery units, or any non-attacking unit.
3. When we raise these pop ups, we should be focusing the camera on that tile and staying there until the pop up goes away, otherwise we are missing on what's happening if it's not directly in our visible area. This is more of an animation issue and how we handle the unit's actions, etc, but perhaps this piece can be implemented by itself at some point

[stavrosfa]

Doesn't this and the next method make more sense as Tile (extension?) methods instead of being in MapUnit?

[ajhalme]

Yes, I was thinking about pushing this inside Tile because the method relies on Tile types internal state. But then Tile would have to know about Player types ability to be at peace (or not) with other players. In a way this logic kind of belongs in both...and neither.

An extension method would work here, but I didn't see that pattern used that much.

But really, I feel MapUnit is exactly the type where Tile and Player meet, so having the logic right here is quite nice. With a private method, this logic composition doesn't need to go further than this file.

I spotted a few cases where a few of these local helpers might make some of the nested logic easier to read. Building out some public extension methods could work as well.

[stavrosfa]

but I didn't see that pattern used that much.

Don't let that stop you, if you think something is better/cleaner this way, go ahead.

What you have is not bad in any way, I am just thinking that the Tile is the object that has the property HasHostileUnits or HasHostileCity and it makes sense in my head and if I was trying to see if this API exists so I can use it in my code, I would seach for it in Tile.
If it's an extension you can also read this like, tile.HasHostileUnits(playerTryingToMove) over the simpler HasHostileUnits(tile, playerTryingToMove)

Also, I have something in what I am working on similar to HasHostileUnits, and I see now that I have it in Tile, so what I am trying to say is, think in terms of what could be useful for other use cases rather than this specific thing you are working on.

You can leave this as is for now, I am not trying to get you to rewrite anything 😃

[ajhalme]

Thanks both for taking a look.

I wasn't able to get them to show up again after that. Similarly if I declared war first then tried attacking with my worker the popup didn't show. Is there something wrong with the probing logic?

I think I got this issue fixed now. This was due to the various calling contexts of CanEnterTile and the behaviour differences during the GoTo planning and Move execution. With the contexts now explicitly laid out in TileProbe, I could see that I should be returning true even for non-combat units, when calling CanEnterTile from contexts that aren't Move.

The context-dependent logic of CanEnterTile is rather mind-bending and could maybe benefit from a rethink, but maybe the TileProbe variants make it a bit more manageable and extensible now.

When I click multiple times I get multiple overlapping pop ups, without really being able to see them, except for the top one. Maybe we can only keep one pop up per tile at any given time.

The original game has an elegant solution for this: the popups are in a stack that builds upwards as new popups get added in. A popup stack in either the tile or unit object might work nicely. Could be a nice first task for someone.

We should not be able to declare war with workers or artillery units, or any non-attacking unit.

Yeah, should be fairly straightforward. Maybe as part of a broader cleanup pass of MapUnit.

When we raise these pop ups, we should be focusing the camera on that tile and staying there until the pop up goes away, otherwise we are missing on what's happening if it's not directly in our visible area. This is more of an animation issue and how we handle the unit's actions, etc, but perhaps this piece can be implemented by itself at some point.

Hmm, maybe. Could also be quite jarring if you have a bunch of these in a row. I have noticed a few times when fortifying a unit that the move animations for other units run first and the unit in focus / middle of the screen is in a kind of a limbo waiting for the other units.

I wonder if there are many "positive" popups like shields from cutting down a forest, or is it mostly just from attempted illegal actions? If it's mostly "negatives", those probably occur when the user is instructing a unit in the visible section of the map, so any centering might be disorienting.

I do recall seeing this kind of centering happening quite regularly in the original games.

[stavrosfa]

I was thinking about the positive ones to be honest, rather than the negative, which as you say are mostly triggered when the bad action is happening in the visible area of the map.
There are stuff like, "We Love the King Day", or "The Statue of Zeus produced an Ancient Cavalry" etc, plus all other future pop ups we can implement that are not in the original game.

The whole centering of the camera could also be a preference setting, so if I want to see them I can turn it on, and if you don't want to see it you could turn it off.