I'm ok with removing that rule.
I understand elexis reasoning and I think the cases where such blocks are code-wise useless and don't help with understanding the code are very seldom and easy to spot.

The patches fixes the bug.
But it seems it reveals a new one.
With your test plan and the patch the paired tech is greyed out and says this tech will be unlocked in city phase...

Fixed by rP23259.

Thank you for the review.
I'll commit that soon.

So I think we should commit this workaround.
Pros:

• It fixes the issue.
• It gets the correct result at least until zen3 comes out.

Cons:

• It is not the correct way to get the cache associativity from this CPUs.

In D2353#102347, @Stan wrote:

This person also runs out of memory #5645 without the fix, maybe it's the same reason.

Nice. Maybe print Market is too close to first one in red?
(not sure, if that is possible)

In D1511#99249, @Vulcan wrote:

Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/978/display/redirect

Now has a ticket at #5620

Reverting the UserReporter back to rP22771 proves it is the UserReporter

With this revision the CPU usage of 0ad on my machine is always 12 percentage points higher than before this revision.
That equals exactly one virtual core, so one thread. Guess you are badly polling somewhere.

In D2353#99093, @elexis wrote:

In D2353#99092, @Imarok wrote:

(OptimusShepard got an OOM after about 30 minutes when playing on a big map with 8 AIs that might be related to this issue.)

Reproducibly? (For example by replaying the match or starting a new match with the same settings)

No idea. Didn't got any more information.

It sounds unexpected that a magic number comparison would trigger an out-of-memory during a match later.
If the same happens without the patch (commenting out the ENSURE), then it's not the patch.
Also then it would be something in the simulation, in which case a replay and some profiling on that to figure out what happens might be useful.

Yeah, I also don't think it has anything to do with the patch, but I can't rule it out 100%.

In D2353#99090, @Woodhousezb wrote:

Great Work!
I conform that the modification on line 92 dows the trick for my ryzen 3600. Now working. The packager for Archlinux has already included this patch in the a23.1-6 version of 0ad

Quickly tested the patch. Looks like a really nice behaviour. 👍
But here are some things I noticed:

Please add a comment with some explanation and the link to the doc.

In D2353#98462, @elexis wrote:

In D2353#98374, @Imarok wrote:

I think we now had multiple Zen 2 users complaining about crashes.

Got a link?

Just see the ticket and the link in the post above.

The other question I have is whether we should actually call ENSURE in AddTLB.
It sounds better to fail more softly with a red error message?
Wouldn't object to a ticket for that (perhaps we already have a "dont use ENSURE" ticket?).

I think we should in general not use ENSURE in hardware detection but red error warnings instead.
Afaik there is nothing that critical that would justify an ENSURE

Sooner or later the magic numbers will change again I assume.

I don't think so.

Looking at the spec of that CPU(PDF page 75) it seems 0x9 is still reserved.

It sounds useful to add the links in the code, there already are some

Good idea

What does reserved mean?

That the number has no current meaning but is reservered for assigning some meaning in the future.

After this change it means in the worst case it will report a 16-way associative cache when someone just has a 8-way or a 32-way or whatever.

Reporting the correct number of not reporting would be more correct if the number is not known.
Either the magic number is correct for all caches, or there should be a wildcard and the Validate function should be changed to account for that wildcard?
(I have no clue what Im talking about)

Seems like ;P
The issue is: we currently don't know what this 0x9 means. But everyone who played 0ad and got this 0x9 had a CPU with a L3 16-way associative cache.
And to be honest: I don't even know why we need this information at all...

The one in https://wildfiregames.com/forum/index.php?/topic/27095-cachecpp43-assertion-failed-cachevalidate/ also has this 0x9 and a 16-way associative cache.
So I assume it's correct.

In D2353#98445, @elexis wrote:

It seems like you know more than the rest of us about this issue, you have many facts gathered.

I just went down the rabbit hole a bit xD
(Plus some knowledge about how a CPU works internally)

In D2353#98371, @elexis wrote:

Will the numbers not bug for other people? (As no other CPU users seem to have complained about them)

I think we now had multiple Zen 2 users complaining about crashes.

As I was "pushed" by @OptimusShepard to help him on that part, I'll answer:

In D2353#98371, @elexis wrote:

How does one determine the magic numbers that should be there?
E.g. can we look it up on some specification of the manufacturer?

In an earlier version of this table It said the zero values are reserved.
But @OptimusShepard reported he got 0x9 as value for the L3-Cache. And it's know, that his CPU has a 16-ways associative cache.
Looking at the spec of that CPU(PDF page 75) it seems 0x9 is still reserved.
So I'm not sure, what goes on here, but it seems 0x9 means 16-way associative in this case.

In D2353#98371, @elexis wrote:

Will the numbers not bug for other people? (As no other CPU users seem to have complained about them)

This change only affects people who got this 0x9, and with previous code they just crashed.
After this change it means in the worst case it will report a 16-way associative cache when someone just has a 8-way or a 32-way or whatever.
So I think the harm is quite limited.

In D2294#95862, @Gallaecio wrote:

I’m just looking for using the same wording. If “Rebuild” works on both strings, I’ll switch both to “Rebuild”.

Good idea to streamline that.
But I think "Rebuild" would be the better word because the whole cache file gets built again and not loaded again. However as you are our language master, if you think rebuild fits better, then go on with that.

Looks sensible to me.

thanks for the patch

Slipped through in rP19194

In D1877#78342, @elexis wrote:

One argument for the button is that it looks more streamlined

Yes, I understood your reasoning and it's not inherently wrong to use a button, especially depending on which graphics represent the control elements. I just wanted to point out that that typically checkboxes are used for binary settings and buttons for actions. If I don't use red color, it's not a contractual obligation to perform changes.

Yes, I also understood your reasoning and found the arguments for a checkbox better than the arguments for a button, so I used a checkbox instead. (It appears that also is a reduction in LOC ;) )

Fix indent

Use a checkbox instead of a toggle button

In D1877#78328, @elexis wrote:

It's a multiple choice, either it shows the stats grouped per team, or it shows the stats grouped per player.
So naturally the options are:

• A checkbox
• A dropdown
• Radio buttons (in HTML, we don't have it in Pyrogenesis GUI)

The purpose of a button to perform an action, not to trigger an option.
So a checkbox [x] Group per team seems more nearby to the purposes of GUI object types me.

Yeah, that seems like a better alternative. (One argument for the button is that it looks more streamlined.)

Remember the state of g_ShowTeamCharts when summary is closed.

player graphs:

I guess you should apply the comments of the linter.

In D1849#78009, @elexis wrote:

I think it's just a minority of mac users.

@vladislavbelov got numbers on mac users and perhaps number of mouse buttons?
Even without numbers, if we can estimate that the option is relevant to half of mac users, it sounds relevant.
Imagine you have to play on a mac and can't use right click. Your ability to use the game is strongly impacted, no?

Many mac users have normal external mice.
There will also be many that just use "two-finger clicks".
So there should only be about 2%-10% remaining. (I am no mac user, so I am just estimating)

In D1751#77632, @elexis wrote:

I'd prefer to not use the simulation as imho minimap pings shouldn't be in the simualation. They are a purely gui thing, just as chat.

Seems like I was arguing with myself. I already do the pings as simulation commands. xD Sorry for my confusion.

Rounding for INTEGER and PERCENTAGE. Seems saner to me.

In D1751#77680, @elexis wrote:

Yeah it's bad that it will be defined for all GUI objects, but I don't see this as a blocker.

rP22134, adds only a little bit of mess, not a blocker, someone else can fix it afterwards
D1781, adds only a little bit of mess, not a blocker, someone else can fix it afterwards
D1751, adds only a little bit of mess, not a blocker, someone else can fix it afterwards
Who will fix it?
Why should he fix it?
When will he fix it?
Why should the code be introduced with a known defect?
Why do I get accused of wanting to stop people from progressing if I point out anti-patterns that only add a little defect for now that can be fixed in the future by someone else?
Created #5442.

@weberkai I guess this newer version of the patch still fixes your issues?

In D1849#77631, @elexis wrote:

Sure, but I don't think that would match in the manual.

What stops us from keeping the manual in sync?

Yeah, why not.

And why is the manual not the first place to look at if the purpose of the diff is to inform mac users how to adapt their thing rather than in the source that only developers see?
If the setting is relevant to all or the majority of mac users (is it?), why would it be overkill to expose it in the GUI?

I think it's just a minority of mac users.

Make the License Bear happy ;)

In D1877#77655, @Stan wrote:

How does it handle changing teams during the game ?

Tiny style fix

Hide the Toggle button if no teams

direct return

Fix comments.

No idea, but does it make sense to test against that behaviour?

Cache replay directory path

Way better. Can't see any performance loss compared to svn anymore. 👍

In D1851#76602, @Freagarach wrote:

In D1851#76566, @Imarok wrote:

Yeah, that doesn't always have a meaning ;)

I know, but since I had an idea about how to do it, I thought I could just share it with the community and perhaps get some feedback in the process. It is not that I *expect* this to come into the main game, I think it is merely a start ;) And perhaps a help to others.
It would be a shame if bad ideas get lost ;)

In rP19674#33135, @weberkai wrote:

Hi!
Good news!
The Differential D1852 fix the issue!
Awesome work guys! Congratulations!
Thank you very much!

In D14#76593, @wraitii wrote:

In D14#76586, @Imarok wrote:

Hmm, seems like it takes far longer to load a map.
Just quickly tested with Corinthian Istmus (4):
Svn took about 7 seconds.
With D14 it took about 18 seconds and I got a notice from my OS that it seems like the program has hung up and if I want to go on or quit the program.

That's weird. Is it stuck at 100%? The only thing I can see taking more time is the thread creation, but that really shouldn't add 9 seconds

Yeah, sorry. I measured the time I was stuck at 100%.

Hmm, seems like it takes far longer to load a map.
Just quickly tested with Corinthian Istmus (4):
Svn stuck about 7 seconds at 100%.
With D14 it stuck about 18 seconds at 100% and I got a notice from my OS that it seems like the program has hung up and if I want to go on or quit the program.

In D1751#76309, @elexis wrote:

Perhaps draw vs. display can be made consistent.

Yeah, sure.

There is a conflicting name MT_MinimapPing in components/CCmpMinimap.cpp.
There should only be one minimap ping, so one of the two should be renamed.

I know. I planned to rename the MT_MinimapPing to MT_MinimapBlink

So in the current revision it works this way:

1. GUI -> send simulation command and displayMapPing locally
2. Commands.js -> record to replay and broadcast GUI message
3. messages.js -> displayMapPing -> GetGUIObjectByName("minimap").ping = true and GUIInterface drawTargetMarker;

yes

I mentioned GetGUIObjectByName("minimap").ping(position, template); is also possible by editing the JSInterface_IGUIObject.cpp function (this file should be split into one file per GUI Object type, since the new function will be added to every GUI Object type).
But not having to hardcode the GUI objects seems preferable, then people can add a second minimap if they'd like to, or rename it.

That was what I looked for. Thank you ;)
I'd prefer to take that way. Yeah it's bad that it will be defined for all GUI objects, but I don't see this as a blocker. We can still use two minimaps as long as they both use the MiniMap class.
I'd prefer to not use the simulation as imho minimap pings shouldn't be in the simualation. They are a purely gui thing, just as chat.

In D1849#76530, @elexis wrote:

Where would you add that to inform the users?

In the files that inform users about hotkeys? Which are the same as:

Which two other places are traditionally kept in sync with this file?

A third place was mentioned in the comments too.

Fix years

In D1851#76554, @Freagarach wrote:

Also I saw the ticket

Yeah, that doesn't always have a meaning ;)

See D1852

@elexis should GetDirectoryName really called that name? Isn't GetDirectoryPath more appropriate?

Picking hidden gaia entities really sounds like cheating. ;)
Also do we really need scouting? With a few shift+clicks on the minimap a unit can be ordered to scout the whole map.

In rP19674#33124, @elexis wrote:

@Imarok wanna fix this, have it tested by @weberkai and approved by vladislav and me?

In D1849#76384, @elexis wrote:

In D1849#76361, @Imarok wrote:

In D1849#76307, @elexis wrote:

Which two other places are traditionally kept in sync with this file?
Also, how many mac players will look at default.cfg?

To my knowledge this is the only place where macmouse is mentioned. It's neither in the manual nor on the hotkeys wikipage as it is no hotkey.

So the intention is just to inform developers who modify default.cfg, but not to inform users with a mac mouse that their hotkeys might need to be changed?

In D1849#76307, @elexis wrote:

Which two other places are traditionally kept in sync with this file?
Also, how many mac players will look at default.cfg?

Imho the large bar looks to big for the small fishing boat