Thanks a lot for the quick review!

Not all rotation is done in unitMotion, only on the first turn, when m_TurnTime is enabled

I was wondering whether the turn rate needs to be in cmpPosition at all when we would do all rotation in cmpUnitMotion?

Tested, worked as advertised. It looks kinda strange to have all trees look exactly the same, but that is the point of this patch ;)
Textuals look good.

• Works as advertised.
• Code looks clean (nitpick can be ignored when desired).
• Event-based is a good step here.

Successful build - Chance fights ever on the side of the prudent.

Successful build - Chance fights ever on the side of the prudent.

comments

Address Freagarach's comments.

Some more testing reveals that in _some_ cases 2 also has the same limitation

Any idea why? It's a bit annoying the minimum vision range is apparently 4.

• make it 4, per @Freagarach

Some more testing reveals that in _some_ cases 2 also has the same limitation as 1. You can make it 3 (probably always correct) or 4 (seems logical with the tilesize).

The code change is correct and, apparently, works under SM45 and SM52.

The correct solution to the issue you highlighted would be reverting rP22802 or at least removing the <ProductionQueue disable=""/> line from the brit_wonder.xml template. However, that's strictly outside the scope of this patch, and ought to be done regardless whether or not this one is committed.

Yes, but any civ except Britons can get the +10 (albeit in a different form) by upgrading that tech. Though I do get your point. I just wanted to note this to whomever will review/commit it.

Yo Stan, I don't see the need for MesonBuild support, I would recommend creating a Trac ticket with "If Time Permits" urgency, a link to this diff in the Patch field, and then abandon this diff for the time being.

Situation prior to this patch:
Any civ build a wonder, get +10 pop cap.
Situation after this patch:
Any civ build a wonder.

But after this patch the Britons don't get the first aura anymore.

Hi @wraitii! I will need this to perform the SM 52->60 migration (see #1389510 on Bugzilla). The ideal route would be to include this under SM52, but if SM60 adds nice code to handle proxy privates, I am fine with doing this as part of the 52->60 code fixes.

I disagree. It's a waste of resources to log data you won't read. If you have a replay it's better than having a bunch of random logs.

Successful build - Chance fights ever on the side of the prudent.

In D2951#133562, @Freagarach wrote:

I mean that the Britons don't get structures/wonder_pop_1-aura when they constructed their wonder.

Actually they did; they just can't research the “Glorious Expansion” technology, which is problematic, but a problem introduced by rP22802, not by this patch.

• make it 2, per @Freagarach

(the advantage of this patch is one needs to change only the number in a single file)

Actually it does matter. The whole point of logs is logging things. Even if they're hidden in game, the errors and warnings still ought to be sent to the logs.

Thx for the review :) Will make other patches if I find more.

Which is why it's on by default . But as reported recently in the forums some people just don't want to see them at all. Keeping them in the file could be am option. But since those are overwritten anyway it doesnt matter.

Nice addition.

But still, a vision of 1 with the knowledge we have now is a bug. So I'd say change all so they have at least a vision of 2.

I mean that the Britons don't get structures/wonder_pop_1-aura when they constructed their wonder.

Isn't the whole point of errors (and warnings to a lesser extend) to point out that something needs fixing? If one would allow to disable them people would disable them for one reason and don't get _any_ for other reasons. If we still want to disable them, then log them at least.

I agree to not spam by debug message. One might add a similar line under a development define. Maybe there are other not really needed debug messages.

The goal of this patch is not save them at all.

This nukes the messages, as in there are not rendered nor saved.

They should still be saved (in the interestinglog.html), just not displayed in a game session.

If I understand your request, you want more of them to be displayed at once?

When I'm just playing, I think at most 10 lines of messages should be enough, at least for me. But I can imagine some who are actively breaking (modifying) things might want more to see more than 20.

In D3049#133545, @Nescio wrote:

Right now 20 lines of error and warning messages are displayed. It would be nice if there was a setting for that too, some people might want to see e.g. 42 lines, others perhaps just 5.

Right now 20 lines of error and warning messages are displayed. It would be nice if there was a setting for that too, some people might want to see e.g. 42 lines, others perhaps just 5.

😀

Successful build - Chance fights ever on the side of the prudent.

Use the SoundManager Instead

In D3025#133510, @Stan wrote:

It's at startup, so I'd argue it does not matter ? I can remove the other occurence, but then someone might switch the order in the future.

Actually it does matter, since player doesn't what's going on yet (no logo, no progress bar, etc).

Successful build - Chance fights ever on the side of the prudent.

It's at startup, so I'd argue it does not matter ? I can remove the other occurence, but then someone might switch the order in the future.

Now you call it twice, but we don't need that. Especially in case there is a chance that OpenAL might do something slow.

Successful build - Chance fights ever on the side of the prudent.

In D3048#133499, @Stan wrote:

Ah I see. why do we need such a shader at all?

In D3039#133500, @bb wrote:

@return ratio between the value of the slider and its actual size in the GUI value is clearly wrong, whether you call it misleading or whatelse, misleading comments are just as much telling lies about the code.

I suppose it means m_MaxValue - m_MinValue, but I don't mind anyway.

Simple version

In D3047#133479, @vladislavbelov wrote:

Does getrandom have some performance expectations? Is it similar to /dev/urandom?

In D3039#133487, @vladislavbelov wrote:

GetSliderRatio did exactly what the comment said, you might think that the value is the misleading name though.

@return ratio between the value of the slider and its actual size in the GUI value is clearly wrong, whether you call it misleading or whatelse, misleading comments are just as much telling lies about the code.

Ah I see. why do we need such a shader at all?

no will to relook at. Eyebolled through all commends in the unitAI, found a few, otherwise mehing

GetSliderRatio did exactly what the comment said, you might think that the value is the misleading name though.

Build failure - The Moirai have given mortals hearts that can endure.

In D3046#133450, @Stan wrote:

Wasn't it used for trailers ?

In D3048#133469, @Stan wrote:

Why do we need this intermediary step? You just committed something that prevents using it. Also it adds a bunch of useless files no? You could just overwrite the existing ones if you want to go through this. Shouldn't they be in mod mod instead ?

Currently shader pipeline uses the dummy fixed program to draw some stuff. I removed that dependency to be able to remove FFP.

Does getrandom have some performance expectations? Is it similar to /dev/urandom?

Successful build - Chance fights ever on the side of the prudent.

add landscape as it has similar issue, notice for unknown map, if biome map would exists, it would need to be unknown_{biome} and would be
currently displayed for any type of landscape if biome would be switched, now with this diff the landscape stays selected

Using them on different units won't help sadly. The problem will remain. What we must work towards are visually distinctive patterns in a moderated quantity. Eg avoiding just color variation when possible, and avoiding having 15+ textures by units.

In rP24107#44706, @vladislavbelov wrote:

For me the changes seems misleading. The commit adds many very similiar textures (they have different small details that are visible only with close view) that adds a lot more variation, that means a lot more state changes, that means another place slows down the game.

Why do we need this intermediary step? You just committed something that prevents using it. Also it adds a bunch of useless files no? You could just overwrite the existing ones if you want to go through this. Shouldn't they be in mod mod instead ?

Meh, my svn did something wrong with my description. It should be "Deny to run the game with Fixed rendering pipeline".

For me the changes seems misleading. The commit adds many very similiar textures (they have different small details that are visible only with close view) that adds a lot more variation, that means a lot more state changes, that means another place slows down the game.

In D3032#133358, @wraitii wrote:

I'm wondering if this can't be fixed by using template literals: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals

Don't really see the relevance

(though I don't really understand why you need it, if you could provide an example - haven't figured it out really from the diff before either)

See revision summary for example. More generally, the current script sees any string between the brackets after translate even if they are not really translatable strings, but rather strings used for array/object/inlined function construction or (as the example) for accessing array/object values.

Wasn't it used for trailers ?

In D3046#133442, @Angen wrote:

Think about this as engine (pyrogenesis) feature not 0ad feature

I don't see any way how it can be useful in the current state, too sharp moves, no configuration. At least I'd suggest to move it to a separate camera controller, because its behaviour completely different.