Thank you for the patch.
And sorry, that it took so much time :/

In D932#71723, @Polakrity wrote:

In D932#71681, @elexis wrote:

Wondering whether these two options shouldn't be located in the Gamesetup UI and Lobby UI part of default.cfg and options.json.

I think it's more instinctive to go to the "Sound" tab.

I tend to the same opinion.

But maybe u want duplicate these options in each tab?

Definitely not a good idea ;)

Add a option to disable the experience status bar

Fix license header year

Good patch.
The only question that remains: Should it be enabled by default?
It could be quite annoying, so I'd lean to disable it by default.
What do you think?

In D1769#71333, @elexis wrote:

Was there a reason for the == case?

Was there since leper implemented Healing in rP11536

Adding an option for this is a good idea.
Will do so as soon as i have time for it.

In D1473#70819, @wraitii wrote:

I think this should differentiate packed and unpacked siege engines. (Correct me if it already does)

It doesn't as-is but I actually disagree here, so :p . Is the triple-clicking behaviour described above in-game already?

In D1473#70814, @wraitii wrote:

This seems obviously good to me?
Unless I'm misunderstanding things this just affects the double-clicking behaviour?

Support for multiple pings. Use player color for pings. Ping is shown as increasing square. (hope someone will code something more beautiful) Limit each player to 1 ping per second.

In D1751#70444, @elexis wrote:

In CCmpMinimap.cpp:

	// Not serialized (based on renderer timing):
	// TODO: eventually ping state should be serialized and tied into simulation time, but currently lag causes too many problems
	double m_PingEndTime;

		case MT_MinimapPing:
			// This depends on the viewing player, so don't alter the synchronized simulation state
			m_IsPinging = true;

Can find some more information with blame then.

That is the minimap attack notification. (white blinking unit)

Use sim command instead of chat. Play sound. (but the wrong) Create ping entity on real map. (but the wrong)

In D1751#70415, @elexis wrote:

(I think a simulation command would be the wrong place for that.)

That's an important decision. Why is a simulation command wrong and chat right?

Whether it goes through the simulation or not, it sounds like it should end up as a GUI notification, near the other events that the GUI processes that it receives from the gamestate. So that one could also play a funky sound or display something in the 2D menus.
That would be g_NotificationsTypes instead of g_IsChatAddressee and parseChatAddressee (Maybe the others have an opinion too, but IMO parsing player command events that create local simulation entities seems really out of place from a function that is supposed to be about chat messages)
These events usually come from the GUIInterface, i.e. cmpGuiInterface.AddTimeNotification or cmpGuiInterface.PushNotification.
If you do it via chat, it won't be in the replay. If it was in the simulation, one could even do things in the simulation if that event is received (extensibility for future creativity in 0ad and mods. for example units automatically reacting towards that ping, perhaps an actual simulation entity that may influence the simulation other than the VisualActor component and Position, marking that position instead of every player creating the same local one). One could store additional data about the ping.
For example if you want a ping to be displayed for 30 seconds and people rejoin or load a savegame 1 second after such a map ping was sent, some of the players will see it for the next 29 seconds and possibly refer to that point in the chat, while the other player doesn't see that thing.

Ok, you convinced me. It'll be a sim command. (Was my first approach, but I thought such only visible thing should be better handled like chat)

Finishing to read the code, I see there is no local entity created yet.

As already stated in the TODOs in the diff description.

IMO that would be very relevant, we have a bunch of flag actors, and perhaps we can find something shiny to make it more visible without trying to find the differently colored pixel on that small thing in the bottom left corner.

The plan is to make more than just a tiny pixel in the minimap. I thought about expanding concentric circles or such.

Cosmetic changes (Year numbers, semi)

If someone had read the linting results of the patch....
(Yeah, I know input.js throws a bunch of warnings)

If nobody objects, I'll commit that soon.

In D1749#70366, @wraitii wrote:

Not to be annoying but wouldn't it be easier to say "from -X to +X, and "Normal" has no bonuses"?

In D1749#70337, @Nescio wrote:

How about the following then?

The AI's resource gathering rate and trade gain depends on the difficulty level (Sandbox −58%, Very Easy −44%, Easy −25%, Medium normal, Hard +25%, Very Hard +56%); the easiest levels also have a slower production and building rate.

Use CenterPoint()

Deduplicate code by adding two functions

Last time the values were changed in rP21256. In rP21257 the description was adapted.
So it was adapted. But it has been rounded. So not sure if we should use exact values, but I have nothing against it.
I don't find your new format that readable. I think the old had enough information.

In rP21520#32227, @elexis wrote:

If one sets that value to true when hosting a lobbied game, all players will join with foo (2) in the gamesetup and won't be able to rejoin after a disconnect!

Hmm, bad thing.
I guess just disabling that feature in the lobby is the solution.
It doesn't make any sense anyway when lobby authentication is enabled, because there can't be duplicate names.

early return

In D1742#70022, @elexis wrote:

I guess not without hacks or rewrites.

And I guess we won't know for sure without looking up what that function does.
Looks like it only needs updateUnitCommands, which is called with 2 const arguments in updateSelectionDetails and one argument which looks like we might get that easily elsewhere or again with some refactoring,
for instance moving the entStates loop to a helper function g_Selection.GetEntityStates, calling that in updateSelectionDetails and the scroll caller. (But I can explain the patch to the JIT in programming code instead of paraphrasing it I guess.)

Hmm ok, it looks doable. (Had to think quite long about possible corner cases.)
But the question is if that is worth the effort, since updateSelectionDetails is far more performance heavy than displayMultiple or displaySingle. (factor 4-10 with a mean of 5)

(Also you updated the wrong revision proposal D1460)

(Uh, f***. Seems like I messed up my scripts...)

Revert the upload, was the wrong differential

early return

In D1742#69973, @elexis wrote:

updateSelectionDetails sounds like it updates everything, could we get away with updating only the relevant part?

I guess not without hacks or rewrites.

Also, the regular batch training, does that have the same issue and could receive the same fix? (Perhaps a common method would make sense, I didn't check)

See my comment above.

Perhaps early return.

If wanted...

In rP21497#30285, @elexis wrote:

Also incrasing the batchsize should not be possible if one can't afford that (or at least the current resources).

That would require a quite big rewrite...
Also what about that use case:
User has batch size 9 and batch count 3. They increase the batch size to 10, but have too less resources for that. So they decrease the batch count.
This won't be possible anymore. (With sane numbers this might make sense, especially cause you then might have the correct batch size set for the next unit production.) (To be honest, I'm not sure, if that is an applicable usecase ^^)

Done for the batchkey (⇧) in rP19028 by elexis.

In rP21497#31791, @elexis wrote:

A second concern is that the display of the batch count is only updated once per turn, whereas the value is updated instantly.
In mulitplayer this means at least 0.5 seconds (up to to multiple seconds in a laggy game) delay until the user sees the currently selected batchcount!

In D1460#69897, @elexis wrote:

About the performance:
Was that a visual or nonvisual replay?

Just as you proposed. So nonvisual.

Does PostMessage skip sending messages if there are no subscribers?
If so, the performance would correlate with the number of entities that have bars currently displayed (either selected units or all units if tab pressed), (and a test without visible bars would not measure the slowest part of the diff).

Not sure how our simulation determines if there is a subscriber, but all StatusBars get initialized as soon as the entity of that unit is created.

options.json:
As the experience information is a bit verbose, shouldn't there be an options.json entry (similar to the range overlays and extended tooltips)?

Hmm, yeah could be needed. But on the other side we shouldn't create too much options ;)

I've run a replay of "Combat Demo(Huge)":

In D1460#68737, @wraitii wrote:

I think the simplest would be to measure the total runtime of a non-visual replay with and without the patch (for example using time on linux).

Seems like I can't reproduce that bug anymore.
I also don't have any working VS available.
So if anyone wants, please take it over. I cannot work on this further. :(

In D844#69173, @wraitii wrote:

Code looks good and does what it says on the tin. Will let up for a few days as I kinda dislike committing code without an intended use, particularly GUI codes I'd like to know if this will be used sometimes soon.

@wraitii @vladislavbelov any news on that? ;)

In D1481#68809, @wraitii wrote:

So what about setting a bool m_correctAlcInit to false when no device is connected and check for that after constructing CSoundManager instead of catching an exception?

An un-recoverable error in the constructor is _the_ use case for an exception and the above would be a really ugly workaround imo.

That!

In D1481#68794, @elexis wrote:

The ctor -> throw -> catch is still an anti-pattern, because by definition code should be written so that it doesn't trigger errors unless an unexpected event occurs. But this seems to be an expected event.

We don't expect the user not to connect a sound device. But we use the exception to handle that unexpected possibility.

An alternative would also be to not catch this error and have the program exit horribly, but informing the user what to do (configuring some sound device thing),
or fixing the code to not run into this error and pick the possibly other sounddevice that works.

The program shouldn't exit, because it's no fatal error. We can't pick an other device, because if the exception is triggered no device at all is connected.

So what about setting a bool m_correctAlcInit to false when no device is connected and check for that after constructing CSoundManager instead of catching an exception?

In D1481#68698, @elexis wrote:

So the question is: How can the code detect a missing soundcard if sound is enabled prior to creating the SoundManager?
Is alcOpenDevice really the only way to test?

The issue is that newbies interpret a "Replay" button as "Play the a new match with the same settings and seed".
This button is the only button with that caption we have. The "Start Replay" button is imho not that ambigous, also because it has an other context.

In D1460#59918, @elexis wrote:

So it would be good to at least check once that we don't add a big performance regression unwittingly.

Any idea on how to do that? I've never done such thing ;)

In D1481#67038, @elexis wrote:

I didn't read the SoundManager file yet, but in general not creating a component (or passing a dummy component) if the --nosound argument is passed sounds nicer than creating a soundmanager that is determined to be unused and error out and then trying to catch and hide the result?

In D1485#68670, @Stan wrote:

Shouldn't it be "Watch replay" with the second caps ?

If nobody objects, I'll commit that soon.

Assuming you the renames are correct and the proposed changes are made.

Accept if proposed changes are followed.

In D406#66521, @vladislavbelov wrote:

the double > JS::Number conversion loses precision.

I think that is why we have FixedPoint number type in C++. (I guess it's mainly use in simulation)

In D1649#66370, @elexis wrote:

In D1649#66186, @Imarok wrote:

Besides the comments: Looks good.
Could reproduce and fixes. 👍

Anything more that needs review?

Nice 👍
We should probably also rename the file itself.