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

[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.

[TomWerner]

Looks reasonable to me!

I confirmed the behavior I noticed earlier was fixed, and let the game run in observer mode and confirmed war was still declared. (Observer mode definitely seems to have gotten slower, we'll have to do some benchmarking at some point)

Feel free to merge once stavrosfa posts any responses!

[stavrosfa]

Observer mode definitely seems to have gotten slower

How come? Because of the extra checks in CanEnterTile()? I don't see anything else that would potentially cause any slowness, but even that seems like an unlikely culprit.

Can you give me some steps to reproduce?

[ajhalme]

@TomWerner - Do you suspect an issue in this PR, or do you mean that running the observer mode has gotten slower for a while now?

--

I didn't notice the observer mode in the codebase - handy feature! Would be neat if one could also set the Quickstart AI seed. I haven't looked at the AI code much, but I seemed to be getting different games from the same setup.

(For anyone wondering, observer mode is Ctrl+Alt+Shift+O at turn end; it prompts for how many turns to let the game run on autopilot.)

A quick online search found this excellent tutorial on Godot profiling, so I ran the profiler for both the Development branch I have (commit e0c7cb340443fe8c97a382fecdc34d1beeee41fd) and this unit-popup branch and after the default 100 turns got the results below on my trusty 10-year-old Dell XPS 9350 (Godot 4.6; Linux Mint 22.2 (~Ubuntu 24.04 & patches), Linux 6.8.0-100-generic). Nothing conclusive, it's a bit apples to oranges, but I don't think the changes I'm introducing here are making the observer mode meaningfully worse.

The results suggest that, as expected, the processing time seems to increase with the number of turns and the amount of complexity generated by the players. The visuals are posing no trouble to the engine. I was seeing 25-30x peak time spent on "process time" vs "physics frame time". I wouldn't read much into the frequency of the profiler spikes - those seemed to fluxuate a lot for both runs.

The tutorial shows how to declare custom monitors. That could be something to explore. One could also try running the engine alone in some kind of a headless mode as an interesting performance test. With good instrumentation, that could yield insights on where resources are being spent.

[TomWerner]

@TomWerner - Do you suspect an issue in this PR, or do you mean that running the observer mode has gotten slower for a while now?

Sorry, I should have been clearer - I think observer mode is slower now than it was back in say March, not anything to do with your PR. Thanks for the profiling links, that'll come in handy. We definitely can run the engine in headless mode, this test plays out the first 50 turns of a game. I bet we're just doing some overly expensive AI stuff.

[TomWerner]

(It's also possibly my poor little Chromebook was having a slow day and I was just imagining things)

[stavrosfa]

Yeah, i think the pathfinding in general got more expensive (and I am partly responsible). There is definitely room for improvement there.

Nice work @ajhalme, thanks for taking the time to profile this and share the insight, appreciate it.

Alright, is this one good to merge then? I don't know if @ajhalme can merge this themselves, but I could do it, it's just a button press really.

[ajhalme]

Good to go as far as I'm concerned. I don't see a button to merge.

[stavrosfa]

I think I just stumbled upon what might be slowing the observer mode down.

When I am in observer mode, and my units (the human units) are in the visible aera of my screen, this error is flooding the terminal continuously (which I think is writing in the Godot sink, same as it would if we wrote GD.Print("something"), hence the slowdown)

ERROR: Condition "p_texture.is_null()" is true.
at: draw_texture_rect_region (scene/main/canvas_item.cpp:855)

When I am not in observer mode I don't get this error.

Again, this is unrelated to this PR, I was testing in the Development branch

[ajhalme]

If you are on Godot 4.4, you might be missing out on better error backtracing, introduced in Godot 4.5.

On Godot 4.6 I'm getting this kind of stuff in my observer mode. Something off with HillsLayer draw?

[stavrosfa]

You are actually right! We don't load the jungleVolcanoTexture on the hills layer which was definetly causing the error I was actually talking about.
Thanks a ton!

I am running the game through Rider, so I don't have (almost) never Godot open.