Please fix comments from rP25306.

virtual is not needed when there is override.

I don't see a point in the separation as a Manager. Your implementation contains data needed only for rendering (it doesn't manage something and doing such calculations on CPU in case someone wants changeable terrain is very expensive in comparison to GPU) and a small state that might be stored in CmpWaterManager. I'd suggest to use CmpWaterManager (I'm not even sure that we need the Manager suffix for Cmp as well) for the core state, Water (without Manager) for visual properties and WaterRenderer for rendering (like for Terrain).

Forgot to add out of view points. Will do after adding tests and refactoring: viewport bounds don't depend on terrain.

Will take a look tomorrow.

Since we don't use FFP anymore, we can remove it. ARB shaders set it in ShaderProgram.

In D3866#167481, @wraitii wrote:

Edit -> Let me know if this is a blocker for you.

If it isn't fixed now then highly likely it won't be fixed ever.

In D1751#167714, @Stan wrote:

Better?

I think yes.

In D1751#167682, @Imarok wrote:

I don't think the flare marker should have the same shape as the target marker.

Shape != style. I suggest to use the texture.

In D1751#167675, @Stan wrote:

Well the ping is unrelated to both.

EDIT: Feel free to suggest something better.

You could try to use target_marker_red_and_gold but with player colors instead of the red one.

In D1751#167663, @Stan wrote:

Two mods with the other binary files as well. Committers deciders.

In D3835#167578, @nwtour wrote:

@vladislavbelov
This commit makes scrolling in the Settings -> Language menu invisible

Thanks for the catch! Here is the fix: D3881.

In D3866#167460, @wraitii wrote:

While I agree that we ideally shouldn't have unpredictable lifetimes, this is simply not the case while we have the current 'restart' logic for mods, and so I would rather make things safe to use than unsafe as a default, so I'm keeping the validity token business

translate for short words should be with context. There is (if it wasn't fixed yet) an issue in A24b that a dropdown text (like "Medium") in options is translated incorrectly.

In D3848#167242, @wraitii wrote:

Thanks for giving this a look :)

You're always welcome!

I think the implementation is more complicated than I'd like to have. My points:

In rP25266#50844, @wraitii wrote:

Yes, my point is that
Clamp<int>(some_double, some_float, some_size_t) is just as explicit as Clamp(static_cast<int>(some_double), ...) and much more readable.

That means added type_identity is useless, since Clamp<Type> works without it (you might grep occurrences before the commit).

In rP25266#50842, @wraitii wrote:

Specifying T explicitly (Clamp<int>, Clamp<float>) seems a better solution to me when that's needed

That's a workaround. It makes all casts implicit, which isn't good. The STL clamp doesn't use std::type_identity.

In D3857#166658, @wraitii wrote:

You could make it a namespace & declare classes friend to the original type if that's more convenient, not sure. Not sure it's that useful to make the class a member variable when it's all static methods, but maybe you have some notion that state will be useful in the future

It might have a VertexBuffer in future for some kind of primitives.

In rP25229#50703, @Langbart wrote:

I have noticed some difficulties with the minimap buttons.

Please, try a newer revision, at least rP25241.

In rP25241#50693, @Freagarach wrote:

CMiniMap.cpp
../../../source/gui/ObjectTypes/CMiniMap.cpp: In function ‘void {anonymous}::CropPointsByCircle(const std::array<CVector3D, 4>&, const CVector3D&, float, std::vector<CVector3D>*)’:
../../../source/gui/ObjectTypes/CMiniMap.cpp:78:14: warning: unused variable ‘radiusSquared’ [-Wunused-variable]
  const float radiusSquared = radius * radius;
              ^~~~~~~~~~~~~

I'll fix it, thx.

That broke --without-pch.

actor.rnc shouldn’t be deleted.

In D3787#164981, @wraitii wrote:

Are you referring to the actor format? the code?

The code.

It seems or model infrastructure is overcomplicated and too coupled.

Fixes order for siblings.

Maybe combine the lowest setting with bb change about reducing variations (using the same seed)? It'd be more clear for players.