Adding to default.cfg for the purpose of documenting the option. Not offering falling back to upstream default as the current value is problematic and possibly future values may take a decade for us to be relied on. Also what should constitute and invalid value may be stuff for discussion, so conveniently avoid that.

In D4967#212056, @Itms wrote:

Hello, this looks nice and clean, thanks for the work!

I know this is caused by the successive improvements to the patch, but now that you have created a config value, there is no need anymore for a hardcoded default MTU in the C++ code. I think that you should add network.enetmtu = 1372 ; lower enet's MTU in order to account for ICMP and IPv4 headers with some VPN solutions in default.cfg, and remove the constant in the C++. Then, inside CreateHost, check if the config value exists, if it doesn't it will mean the user explicitly wanted to remove the MTU customization, just return host; with the default MTU of 1400; if the config value is here, use it.

Adds configure option network.enetmtu.
Moves header and implementation from lowlevel to network.

Haven't forgotten the package ->packet change, was just waiting for more discussion to happen before pushing out a new version of the patch, will also wait a day or two to allow people to comment on making it configurable. Given the release still has no name we aren't in that much of a hurry.

How should the config option be named, network.mtu is about as misleading as ENET_HOST_DEFAULT_MTU. Any good suggestions?

The only change that upstream patch brings is that now server can specify a lower mtu which is then used for communication in both directions. This patch here doesn't make use of this.

In D4967#211859, @phosit wrote:

Thats much better.
host (and m_Host) should be a std::unique_ptr but that would requires more change.
Propably PS_ENET_HOST_DEFAULT_MTU should be in the cpp file (an implementation detail). No strong opinion.
We use another function-naming stile. But I think it's ok to use that from enet.

Take care of review comments.

In D4967#211738, @phosit wrote:

In D4967#211735, @sera wrote:

Can't just be removed, but it will be possible to make it configurable for users if they have issues, we still should provide a sane default or implement pmtud.

I missread the patch. Your enet-patch has nothing to do with wireguard, right?

In D4967#211722, @phosit wrote:

Is it allowed to mutate ENetHost::mtu directly?
"No fields should be modified unless otherwise stated."

If we follow that there is no way to set pseudo mtu; right after creation and resetting peers after is already the most unlikely way to break in future

As for the choice of 1372, a rule of thumb I know says assume MTU 1400 and you most likely are fine, my guess this same rule is how ENET_HOST_DEFAULT_MTU = 1400 came to be. Then assume the idea was to use ENET_HOST_DEFAULT_MTU in the sense of MTU so we should deduct the ICMP/IPv4 from it.

Update copyright year and add full diff context

In D4935#210945, @phosit wrote:

In D4935#210943, @sera wrote:

base handler is a lot more obvious than erased handler, thanks.

This patch doesn't demonstrate the need for erased handlers, could you add some doxy comments to make it clear how they are supposed to fit into the "new" system?

Can't this be done via shaders? If you need an extra set of textures I'm afraid you'd end up with the demo only just like what happened with the damage textures.

In D4922#210152, @Grapjas wrote:

In D4922#210139, @sera wrote:

If you switch to class syntax and change pretty much every line anyway, I suggest to use default method signature, static and private where appropriate and drop useless "this" refs. Maybe even switch to use doxy comments.

Find this a bit hard to decypher, not that great at the terminology yet.

If you switch to class syntax and change pretty much every line anyway, I suggest to use default method signature, static and private where appropriate and drop useless "this" refs. Maybe even switch to use doxy comments.

In D4279#209437, @phosit wrote:

In D4279#209413, @sera wrote:

For me the intersection with a fixed horizontal plain seems the most attractive solution. A intersection plain height like max(sea_level, min(map_height)) would be my first choice.

That would have the same effect as raising the sea_level to just below the min(map_height).

In D4279#195318, @vladislavbelov wrote:

In D4279#195314, @phosit wrote:

the exact "field of vision".

It's possible but the problem is how to show it properly and what a player does need exactly.

Works with system premake now.

LGTM

In D4895#209054, @Stan wrote:

generated files should not be under version control.

It's not. It just gets bundled accidentally when creating the release archive.

My take:

• generated files should not be under version control.
• files under version control must not be changed by build scripts

Whatever version is in use at compile time. An upgrade to icu forces a recompile of spidermonkey. The spidermonkey package doesn't install any icu headers. Looks like a sanitized package.

Can build with --with-system-mozjs on Gentoo with or without this patch and have confirmed that with this patch mozjs-91 and nspr come after -idirafter. I also confirmed that this patch will break use of system premake.

In D3127#208890, @Stan wrote:

@sera can you try this?

Oh, already noticed by Itms :)

In rP27409#59860, @Stan wrote:

Thanks was fixed locally. Was waiting to see if we could get the other python fix to be sorted out

As requested by Stan making the patching conditional on python version being >= 3.11

In D4886#208818, @Stan wrote:

Well it's still an Entity so you might have PlayerEntity.

Wouldn't this warrant it's own player schema?

Some enums I can't associate with a lookup table, so those name variants I'd rather see dropped. No strong opinion tho.

The names NetModerationTables and LookupTable are supposed to match and I don't like either name in particular. NetModeration.h feels better to me.

In D4876#208456, @vladislavbelov wrote:

In D4876#208455, @sera wrote:

No shader compiler is basically the same as no vulkan support

I believe it's incorrect logic to think about Vulkan. Also not all players want to build everything from sources (it takes noticeable amount of time for shaders). We pack all prepared data inside the 0ad-data package.

In D4876#208447, @vladislavbelov wrote:

In D4876#208429, @sera wrote:

To build sdl with vulkan support you'd need some vulkan build environment and why should such an environment not include a shader compiler?

Because it contradicts to design behind Vulkan.

Sure you can get away without but if sdl without vulkan is a bug then not having a shader compiler can be considered one as well.

You missed my point. I don't have any objection to SDL without Vulkan. The problem is that in your example SDL is compiled without Vulkan only because of no shader compiler available.

if enabled requiring a shader compiler would be fair game.

I'm definitely against requiring shader compiler to run a Vulkan. I don't want to bother players to care about shader compilation, only when they want to (for those who wants I'm glad to provide an option).

In D4876#208408, @vladislavbelov wrote:

In D4876#208407, @sera wrote:

Having it precompiled in an external mod 0ad-spirv for now works for me short term for the sake of getting the ball rolling.

0ad-spirv is only for SVN development. For releases SPIR-V shaders will be included in 0ad-data.

In D4876#208406, @vladislavbelov wrote:

In D4876#208405, @sera wrote:

Just like a c++ compiler or rust compiler a shader compiler can be treated as external dependency, no need for shipping our own build thereof. Making vulkan support in 0ad optional sounds good though, then those who can't be bothered can simply stick to gl only.

I plan to have Vulkan for all platforms with precompiled shaders, but built-in support for compiling shaders is currently missing (--with-shaderc flag might be added in the future). You'd need to install glslc/spirv-reflect manually and then run a script.

In D4876#208403, @vladislavbelov wrote:

In D4876#208402, @sera wrote:

googles shaderc is far from 200mb, closer to 200kb.

You're forgetting about Windows and probably macOS. On windows only shaderc_shared.dll is 11MB. But we also store .pdb and .lib.

In D4876#208388, @Stan wrote:

In D4876#208346, @sera wrote:

In D4876#208333, @vladislavbelov wrote:

NOTE: Shaders should be in the mod mod.

I'd prefer to include those shaders only in final builds, because they're binaries and might be changed.

I'd prefer to include those shaders in source form only and compile them to spir-v during build.

Problem is it requires an extra dependency on all platforms which might not be available. Which means we would then be required to add the soure of that ands that's 200mb: glslc/spirv-reflect via Vulkan SDK or similar.
the script is python (and will be integrated in a separate patch) It also should be run if modders change things so it's not related to build directly..

In D4876#208333, @vladislavbelov wrote:

NOTE: Shaders should be in the mod mod.

I'd prefer to include those shaders only in final builds, because they're binaries and might be changed.

In D4847#207206, @Grapjas wrote:

Data (word list) and code needs to be split.

Why?

Quick look only.

Data (word list) and code needs to be split. Also it might make sense to support multiple data files to support "language packs" via mods that can individually be enabled / shipped. The reason for this being a word may be perfectly fine in one language but not another, secondly easier maintenance by people actually speaking a certain language and lastly performance. Performance might not be critical in lobby but we should be careful for in-game.

The overload for wchar is proprietary, so likely only available with msvc. Needs some ifdef here. Or better yet move to <filesystem>.

It represents the fact that Rome was one of the most populous cities in the ancient world.

Go ahead

"Ralph Sennhauser <ralph.sennhauser@gmail.com>", if nick is required "sera" is fine.

Do you need to call parse multiple times? If not why do you need the method?

If so, what should I use instead of debug_printf()?

You won't need the readme.txt anymore if you have support for help, worst case just run "pyrogenesis -help > readme.txt" as a postbuild command. So no duplication required.

IMHO the right approach is to change CmdLineArgs from an minimalist class to a proper command line parser where you register available options, does validation and has a help string for each.

It'd be great but who knows how long it will take for someone to go out and make it.

Just write it as you like and get it committed to /binaries/data/mods/public, doesn't have to be a complete or perfect plan, a rundown of possibilities will already do, anything else I don't see ever having success indeed.

making the addition of it to that file out of scope for this revision

I suppose the bigger problem is that there isn't really a design plan/idea for how the civs should play and how many bonuses, unique techs a civ should have

same, with context