That is indeed horrible.

In D5309#226621, @Stan wrote:

If we're looking at filenames with tabs shouldn't we look for any whitespace?

Well, the linked upstream commit bumps to 2.2.7 but the tag 2.2.7 doesn't contain this commit, so seems I was mislead.

Should be miniupnpc 2.2.7, right?

I prefer the approach used for the logger, ie put some profiler object on the stack that sets the g_profiler to something usable in the constructor and cleans up in the destructor. Maybe also replace the use of optional with a dummy profiler, similar to the blackhole logger.

In D5286#226446, @Dunedan wrote:

Do we all agree that it's an improvement over the current situation and should land?

In D5286#226419, @Dunedan wrote:

In D5286#225319, @sera wrote:

For instance single-player and multiplayer replay are the same, just a different default for one filter.

What about removing the replay items from the single-player and multiplayer submenus and add a single new "Game Replays" item to the top-level menu? When opened it would show the list of all games, without the pre-selected filter on the game type. That would also make replays for campaigns easier to find.
The downside of this would be that the main menu would have 9 items then, which feels to me like it's too many items when looking at it and that the "Single-player" submenu would only contain 2 items, which feels too little to warrant a separate submenu.
Does one of you have an idea how to better organize that?

In D5282#225970, @phosit wrote:

droping from 20.78 ms to 23.05 ms per simulation frame.
measured on no-visual combat demo huge.

As it doesn't improve performance. What's the goal of this diff?

In D5293#225609, @Dunedan wrote:

In D5293#225603, @sera wrote:

As for "dotted", whatever that is, is there value to support it on the c++ side?

Here is the Babel issue which caused it to be added: https://github.com/python-babel/babel/issues/148

The summary doesn't say why exactly 2.12 and looking at https://babel.pocoo.org/en/latest/changelog.html I don't see an obvious reason either. Can you please clarify?

In D5285#225479, @phosit wrote:

In D5285#225460, @sera wrote:

Went trough all, no accidental code changes spotted.

Thanks a lot

yw

CPU skinned? Is this what is commonly known as gpu skinning but the work done on the cpu? isn't that prohibitively expensive?

Went trough all, no accidental code changes spotted.

Can't figure how the comments lead to this but as it's the same as my initial take no objections from me.

In D5286#225425, @wowgetoffyourcellphone wrote:

Perhaps "Scenario Editor" -> "Atlas Scenario Editor"?

Up to northern_lights

Checked maps starting with a trough f (up to frontier), no real issue found, will continue another time. Line breaking for code I guess one could do some bike shedding here but I say your changes are ok as-is.

Didn't spot any issues

While huge it still can be reviewed after you fixed all linter warnings, so I'd say getting this one in next is fine.

I agree that the current one is a bit unfortunate just that reorganizing leads to people to have to get used to it again. So I suggest to also try to address other issues at the same time. For instance single-player and multiplayer replay are the same, just a different default for one filter. Then the multiplayer vs single-player and the lobby vs lan always felt awkward to me and I'm sure this can be improved.

Would be nice if it were an upstream tarball that gets extracted during build instead of checking in sources, similar to how sm is handled.

The global header with the wfg styling I'd only use for global icons and links like forum, web, trac, irc and so forth; don't think reusing the global header style for application headers is a good idea.

In D5258#224712, @phosit wrote:

whitespace changes

There is code to handle filesystem 2 & 3 implementation, if you want to to require boost filesystem 4 it be best to remove the handling for old versions. V4 is probably around for more then 10 minor boost versions which I'd say is old enough, tho you'd have to check which exact version would be the new minimal version anyway.

In D5256#223673, @Vantha wrote:

When the AINames are empty or don't exist, the game already throws errors before the step of choosing the names.

Guess some of those let could be const instead.

However clients have no idea which user issued which command which might be ok-ish or not, you don't need it for basic gameplay but per user stats are of reach for instance.

Thinking a bit more about player spoofing, it probably is a bad idea in the first place and extending the command protocol to have both a "user" and "player" id would probably make implementing co-op play a lot cleaner. Sure it's not the intended scope of this patch ...

Quick review

In D5222#223496, @phosit wrote:

In D5222#223495, @sera wrote:

In D5222#223494, @phosit wrote:

IMO The behavior of the server shouldn't dependent on the simulation's init-atributes. Instead the controller should explicitly control the server.

allowCheats is an init-attribute, at least that's my take.

Is that also the case if we call it allowPlayerSpoofing?

In D5222#223494, @phosit wrote:

IMO The behavior of the server shouldn't dependent on the simulation's init-atributes. Instead the controller should explicitly control the server.

That all maps must be migrated more or less immediately to use the new way is a given and doesn't depend on whether to leave a compatibility layer behind.

In D5220#223420, @phosit wrote:

In D5220#223418, @sera wrote:

So what is the plan?

My plan is to

1. Learn rmgen
2. Document the flaws
3. Come up with a solution
4. Convince other devs

xD

In D5220#223417, @phosit wrote:

In D5220#223414, @sera wrote:

Also now that you drive generation from cpp why the need to call export map from js?

You mean calling g_Map.MakeExportable() from C++?
That's a good idea. Do you have a better name for MakeExportable? "finalize" or something...

*shrug, also would be more obvious an improvement if the old way is removed.

Checking the code in pyrogenesis the global object is indeed available under the name "global" in js running in pyrogenesis, my bad, I got tricked to believe you added a property global to the global object :)

Also now that you drive generation from cpp why the need to call export map from js?

global is as good a name as rootedvalue or habakuk.

why not Engine.map?

Ok, now that we have established that for sufficiently small deviation in radius this approach is acceptable let's continue getting the patch into shape.

In D5247#223303, @Feldfeld wrote:

Depending on how it's done it could have an impact for competitive gameplay.

@Baelish , not sure I follow you. To find a solution one must exist, the code you touch doesn't guarantee such at all. What the ticket suggest is to make the radius variable to some extent to increase the probability such a solution exist in the first place. Ofc there is a tradeoff that now possible minor imbalance is introduced, like one players chickens are on average one tile further away than the other players or the like. The change from random angle to iterative angle in practice should be less impactful.

Thanks for the rational, tho the idea was to update the summary at the top of this web page. The summary is what the commit message should be based on.

I did not remove old parts, so i took them as comments

In D5246#223204, @marder wrote:

at least from a simple test (just object creation and setting a value) the performance looks equal. Not sure what happens in complex scenarios tho.

I suggest to add a shaders not found error message and maybe give a hint as for the possible cause and solution, otherwise LGTM apart from the obvious linter warning.

In D5242#223125, @vladislavbelov wrote:

In D5242#223121, @sera wrote:

About atlas, wx itself uses gl but that doesn't mean you can't use vulkan for the viewport, anyway mostly a non issue and certainly low prio.

Vulkan can't access the viewport without interacting with wx.

If selecting vulkan can break the game then it shouldn't be exposed in the gui in the first place, even less be recommended. At least as long as reinstalling can't fix the issue. So before a release this needs to be addressed anyway.

Vulkan being _highly recommended_ but not the default is a contradiction, the note about drivers could be read as don't try unless you are sure it works because you will lock yourself out if not. Graceful degradation from vulkan to gl if something goes wrong is a must have.

In D5242#223095, @Stan wrote:

Which is really a pain

Well, I'd say gl is supposed to be labeled legacy. So I'd use stronger language and move vulkan to the top of the list and make it default in default.cfg

In D5240#222967, @Stan wrote:

How about using ProjectionType from source/graphics
/Camera.h ?

In D5240#222955, @Boston_Brooks wrote:

where there should be references to perspective/orthogonal.

Haven't tested so just some comments based on a quick review. First is you are switching between perspective and orthographic projection, not between 2D and 3D (ie if in orthographic mode you can still 3D navigate the viewport, right?), so names should reflect that. Secondly, if you want to log output use the logger infrastructure provided by pyrogenesis, tho I guess the printf are just for the sake of development and therefore should be removed.

we could adopt the upstream fix instead found at https://github.com/premake/premake-core/pull/2117, but this one looks ok as well.

Binary patches are a real pain to test so I skip doing that, always ;)

make the textures less blurry

The packaging tarball and the wheels could be hosted as a tarball like the mozjs tarball or bundled with it instead of committed to svn. Actually the mozjs tarball isn't pristine (subset of firefox tarball), @wraitii already runs autoconf first, so just plain updating vendored python packages doesn't seem that big of a leap anymore. Just let us name it mozjs-ps-91.13.1.tar.xz

wrt deepfreeze

In D5218#221736, @vladislavbelov wrote:

In D5218#221718, @sera wrote:

ERROR: Failed to compile shader 'shaders/glsl/compute_downscale.cs':
0:27(31): error: no function with name 'texture2D'
0:27(2): error: no matching function for call to `imageStore(image2D, ivec2, error)'; candidates are:

Should be fixed with the last update. To reproduce (I wasn't able locally) you need to choose OpenGL backend (restart after that), enable post processing, set scale to 200%.

But might have been using opengl, maybe related to enabling postprocessing or some part thereof.

ERROR: Failed to compile shader 'shaders/glsl/compute_downscale.cs':
0:27(31): error: no function with name 'texture2D'
0:27(2): error: no matching function for call to `imageStore(image2D, ivec2, error)'; candidates are:
0:27(2): error:    void imageStore(image1D, int, vec4)
0:27(2): error:    void imageStore(image2D, ivec2, vec4)
0:27(2): error:    void imageStore(image3D, ivec3, vec4)
0:27(2): error:    void imageStore(image2DRect, ivec2, vec4)
0:27(2): error:    void imageStore(imageCube, ivec3, vec4)
0:27(2): error:    void imageStore(imageBuffer, int, vec4)
0:27(2): error:    void imageStore(image1DArray, ivec2, vec4)
0:27(2): error:    void imageStore(image2DArray, ivec3, vec4)
0:27(2): error:    void imageStore(imageCubeArray, ivec3, vec4)
0:27(2): error:    void imageStore(image2DMS, ivec2, int, vec4)
0:27(2): error:    void imageStore(image2DMSArray, ivec3, int, vec4)
0:27(2): error:    void imageStore(iimage1D, int, ivec4)
0:27(2): error:    void imageStore(iimage2D, ivec2, ivec4)
0:27(2): error:    void imageStore(iimage3D, ivec3, ivec4)
0:27(2): error:    void imageStore(iimage2DRect, ivec2, ivec4)
0:27(2): error:    void imageStore(iimageCube, ivec3, ivec4)
0:27(2): error:    void imageStore(iimageBuffer, int, ivec4)
0:27(2): error:    void imageStore(iimage1DArray, ivec2, ivec4)
0:27(2): error:    void imageStore(iimage2DArray, ivec3, ivec4)
0:27(2): error:    void imageStore(iimageCubeArray, ivec3, ivec4)
0:27(2): error:    void imageStore(iimage2DMS, ivec2, int, ivec4)
0:27(2): error:    void imageStore(iimage2DMSArray, ivec3, int, ivec4)
0:27(2): error:    void imageStore(uimage1D, int, uvec4)
0:27(2): error:    void imageStore(uimage2D, ivec2, uvec4)
0:27(2): error:    void imageStore(uimage3D, ivec3, uvec4)
0:27(2): error:    void imageStore(uimage2DRect, ivec2, uvec4)
0:27(2): error:    void imageStore(uimageCube, ivec3, uvec4)
0:27(2): error:    void imageStore(uimageBuffer, int, uvec4)
0:27(2): error:    void imageStore(uimage1DArray, ivec2, uvec4)
0:27(2): error:    void imageStore(uimage2DArray, ivec3, uvec4)
0:27(2): error:    void imageStore(uimageCubeArray, ivec3, uvec4)
0:27(2): error:    void imageStore(uimage2DMS, ivec2, int, uvec4)
0:27(2): error:    void imageStore(uimage2DMSArray, ivec3, int, uvec4)

After fiddling a while with this new feature and checking the console after I saw the above, but can't figure out how to reproduce it :)

LGTM if you fix the linter warnings.

How is it supposed to interact with gui.scale?

Should be var -> let, const

In D5189#220745, @phosit wrote:

In D5189#220712, @sera wrote:

Seems not easy to describe that function, no wonder the original comment was lousy ...

Propably I should describe it more vivid. Like "The predicate shall return wheiter to expand the wave around that tile." and "The first argument is where the wave came from".

Seems not easy to describe that function, no wonder the original comment was lousy ...

I generally like it and agree with you.

In D5167#219996, @phosit wrote:

The logger (file) should not know how it's tested. Code only used in tests should be in a test specivic file.

In D5167#219914, @phosit wrote:

In D5167#219802, @sera wrote:

Also might actually be nice to use an StdoutLogger [...] using the same mechanism.

Yes but it's in conflict with the guideline of using initialization over assignement.
Now g_Logger could be a std::reference_wrapper when we implement your idea that's not possible anymore.

In D5167#219874, @phosit wrote:

@sera I named it EngineLogger instead of DefaultLogger ;)

In D5167#219779, @phosit wrote:

In D5167#219748, @sera wrote:

About the patch I'd appreciate if there wasn't the asymmetry in default logger vs TestLogger and TestStdoutLogger, all should be handled the same.

What do you mean by that exactly: "One is declared inside the CLogger and the other in the global namespace", "One is placed inside the object the other on the heap" or "One uses the default constructor and the other provide a stream."? (I guess it's not the last one.)