In rP21943#31690, @Itms wrote:

The consensus in that topic is the reason why I asked user1 to commit this, but if you want I can formally accept this revision.

The commit contains only few style mistakes. So the accept isn't required. My point that the commit doesn't have any linked diff/ticket or link to its conversation. My first thought was are these bots and rooms already exist.

In rP21943#31673, @Stan wrote:

To me people should upgrade.

Only if needed.

Do really need it? I thought we have pretty compatible versions. Also these *b versions aren't running on serverside, no?

In D1684#66783, @elexis wrote:

As you see, I was right, with the patch you need to change a more number of lines to add new values to the GUI stack.
The patch now forbid to use different values passed to the GUI stack, only functions.

"Only functions" is the part that makes no sense to me

In D1684#66778, @elexis wrote:

In D1684#66775, @vladislavbelov wrote:

The patch now forbid to use different values passed to the GUI stack, only functions.

I think you didn't read the patch right

I think you didn't read my comment right :)

The patch now forbid to use different values passed to the GUI stack, only functions. Before the patch someone could easily add another param (with C++/JS modifications).

In D1683#66749, @elexis wrote:

the sky is in infinity. So all clouds move together with the camera

If you mean the clouds in the sky texture, that doesn't seem like a problem.
If we want closer clouds, they should be separate from the skybox probably: #46.

Yep, clouds in the sky texture. Separate "clouds" we have only as particles. Anyway close clouds should be separate, yes.

Did you test it on really wide monitors/windows?

Why separate header? I don't think it helps much. All other components don't have headers.

The problem isn't in macros, because we use scoped block to solve the redeclaration. Probably it was missed somewhere. If you still remove macros, you need to remove scoped blocks too.

In D406#66520, @elexis wrote:

Don't see a reason why an epsilon issue could not be fixed.
You mean stepWidth = 5 given in options.json will become 4.999,

Sort of.

so that the slider position wills be 0, 4.99999.., 9.9999...

No, the slider position will be correct in C++ (with an epsilon correction), but the double > JS::Number conversion loses precision.

Then min and max can also be 4.9999 and the current patch is as affected by epsilon as the change that I request, where is the issue?

Min, max can't be 4.999, the problem is only for the return to JS value.

In D406#66518, @elexis wrote:

We can convert a JS::Number <-> double well, so is it about some epsilon / rounding phenomenon?
Getting min + i * width rounded as wished shouldn't be so hard to accomplish, regardless which implementation is introducing the epsilon.

You can check it by returning a fixed number, like 4.5. JS may get from C++ the 4.499999 value. And for JS these values are different.

In D1668#66513, @elexis wrote:

In D1668#66496, @vladislavbelov wrote:

Smart, but the (N)RVO isn't guaranteed. Only since C++17.

https://de.cppreference.com/w/cpp/language/copy_elision looks more like C++11, only some other features C++17?

Yeah, compilers started optimize these cases a long time ago. But there're many nuances.

In D1023#66466, @elexis wrote:

We also want to use the Red buttons, so all GUI pages use the same button style (although I found/find grey ones more appealing sometimes) (the patch can be considered one of multiple independent improvements of something that is free of big defects.)
(We should have a ticket and a good reference to this patch with regards to the golden bar thing)

In D406#66464, @elexis wrote:

In D406#66462, @Stan wrote:

We really need this feature for the GUI.Scale options. I think there is already a differential but if Not I can submit a patch

The patch for UI scale is uploaded but abandoned, should be reclaimed and receive an ugly workaround (message box that if not clicked after 7 seconds will reset the UI scale)

Not a workaround solution requires improvements in the our option page logic. Because we shouldn't change visible scale while a user is dragging the slider.

In D1515#66454, @elexis wrote:

I agree with the patch, it's a good cleanup. A Color entirely is not a Shape and it will leave behind cleaner code if we don't include the shapes header if we only need the color.
(I suppose one could lose a lot of time reinvestigating the completeness of includes, better don't even start).
Otherwise I suppose one could test it by compilign without precompiled headers, running every function that uses colors and then it could be committed if it still doesn't fall apart and everything is confirmed to be the same code flow as before.

Do I understand it correctly, that IPv4 won't work when IPv6 is enabled?

In D1437#66465, @aeonios wrote:

In D1437#66450, @elexis wrote:

I think the problem is that this GL error is not reported to the user in a way that the average user comprehends.
Or rather the problem, nor the setting change are reported at all (barring the unreadable barely noticeable openGL mainlog error).

I sort of fixed that in my unrelated shadow revision. Sort of. The error is still reported via the GL error text, but I made it more understandable and just had it recursively reduce the size until it allocs properly or fails completely. Also the settings GUI won't update until you close it and open it again even after the resolution has been reduced by the backend code if/when it fails. I don't think there's any way around that due to the way the settings panel works.

I can close this ticket, if you'll solve the problem in your diff.

In D1668#66468, @elexis wrote:

@vladislavbelov are most compilers really smart enough to use copy elision for strings here as widely claimed on the internet?
(Some places use one pattern, other places use the other pattern, we should make up our mind what we want, otherwise it sounds contradictory if we claim there to be good reasons for both.)

Isn't the flag needed in the premake4.lua too?

Isn't the flag needed in the premake4.lua too?

Function were declarated by CC, but used with a wrong first letter.

Can the -mmacosx-version-min=10.9 flag be replaced by a constant? To prevent those big changes to change a version.

In D1662#65759, @nani wrote:

In D1662#65757, @smiley wrote:

https://code.wildfiregames.com/source/0ad/browse/ps/trunk/source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Player/Player.cpp$525

How can it be made so CCmpRangeManager.cpp and Player.cpp share the same #define MAX_NUMBER_OF_PLAYERS 32 ?

In D1662#65755, @nani wrote:

In D1662#65754, @vladislavbelov wrote:

In D1662#65753, @nani wrote:

It could also be added a Engine.MatchGetMaxNumberOfPlayers()

That'd be awesome, because we need to avoid a code duplication.

Also, isn't there any other place with the hardcoded max number of players?

I think the rmg library has some hardcodings too.
About the xml files with <repeat> tags, the hardcoding can't be avoided unless we add Engine.MatchGetMaxNumberOfPlayers() or similar that defines <repeat n="number"> in some way before the parsing or in the parsing.

In D1662#65753, @nani wrote:

It could also be added a Engine.MatchGetMaxNumberOfPlayers()

That'd be awesome, because we need to avoid a code duplication.

Why 32 in C++ and 24 in JS?

In D1600#65632, @elexis wrote:

But the url still can be changed anyway, no?

The frontend and the backend should be kept in sync:
https://code.wildfiregames.com/source/0ad/browse/ps/trunk/source/tools/webservices/userreport/

In D1600#65204, @elexis wrote:

I can't imagine it not working, but I find myself surprised sometimes with library functions.

And that's why we require to test things before committing them.

Patch doesn't actually work for me and still allows upload with http. I could see with Wireshark that the up and download was done using HTTP, though it seems zlib encrypted (and an absence of any TLS initiation within the http cleartext data, also that would be a protocol new to me).

Actually it seems this only uses SSL/TLS for FTP and other protocols, but not HTTP!
https://curl.haxx.se/libcurl/c/CURLOPT_USE_SSL.html
https://curl.haxx.se/libcurl/c/libcurl-errors.html#CURLEUSESSLFAILED

So if it should be enforced in C++, it'd have to be a rejection if the URL starts with http:// Im afraid.
But we wanted to protect from malicious JS mods changing the URL anyway by adding protected config entries for such sensitive data.

Btw the curl UserAgent exposes possibly sensitive data
0ad libcurl/7.47.0 GnuTLS/3.4.10 zlib/1.2.8 libidn/1.32 librtmp/2.3 (http://play0ad.com/)

I'd say that this part can be better in the C++ part. Because it may be faster and has much more information about elements.

Does our spidermonkey support Uint8Array/Uint32Array? It'd be good to compare them by performance/memory usage.

I have to say the current way (1 vertex per call) to modify the terrain is pretty slow, especially if it's called from JS. So I suggest to use a range modifier, or even better - brushes. With brushes you only need to pass brush params and C++ part will complete a request as fast as possible. With brushes you're still able to edit terrain by 1 vertex: just pick the brush radius as 1.

In D1578#64459, @echotangoecho wrote:

D492

In D1608#64376, @elexis wrote:

To test if D1600 actually works, I was wondering if we can't catch the CURL error code. It might also be slightly more helpful than the nothing-received test.
Vladislav mentioned that different versions might have different error codes, but I assume we can pass on the CURL error string.
https://curl.haxx.se/libcurl/c/libcurl-errors.html

In D1606#64342, @elexis wrote:

You mean unsupported unescaped strings (such as [color="foo"][font="foo"]text[/color][/font])?

Nope, do we escape strings, not characters? I mean to prevent things like "\ ", when the escaped character does nothing.

Probably we need to report about unsupported escaped symbols.

In D1591#64287, @elexis wrote:

There is a double c-style cast in the other Hexify function of Util.cpp then:

Yes, I mentioned this some time ago.

Updates the year.

Adds the Mod.IO support.

In D1591#64047, @elexis wrote:

forgot to upload Util.cpp and don't want to revert now:

std::string Hexify(const u8* s, size_t length)
{
	std::stringstream str;
	str << std::hex;
	for (size_t i = 0; i < length; ++i)
		str << std::setfill('0') << std::setw(2) << (int)s[i];
	return str.str();
}

Could you attach your errors?

In D1588#63775, @drummyfish wrote:

I agree, but what should the default be? The Paint tool has "grass1_spring" hard-coded - should I hard-code a value the same way? It's not good, someone can use a mod where the file won't exists, but this issue can probably be left for a later dehardcoding stage, right?

It'd be good to get first item from the list.

There is many duplication of std::string(reinterpret_cast<...>...), especially for reinterpret_cast it's not very good. I suggest to add a function Hexify(const u8* data, size_t size) to fix the number of reinterpret_cast.

In D1588#63771, @drummyfish wrote:

Try it out please. A new icon for the tool would have to be made, for now I gave it the same one as the Move tool. ATM if you start the editor and select the Place tool without selecting any object first, a warning is written out in the window - should I leave it like this, or try to grey the button out? Or maybe automatically select the first item in the list? Not sure.

I think default is good, because as I mentioned above, we have the default logic for the Paint Terrain tool.

In D1588#63750, @elexis wrote:

Wait, the problem is that if you enter the object tab, select an item, then select a different tab, then select the object tab again, the placement is disabled, but the user expectation is that the placement of the already selected object is enabled.
So calling OnSelectObject when the object tab is selected fixes that without the user noticing anything, without adding any new GUI elements, right?

Nope, there are at least 2 other similar problems:

In D1588#63748, @elexis wrote:

But I can't imagine that we need new UI elements to solve this problem.
As far as I see we only need to call OnSelectObject if the user activated the ObjectSidebar tab, no?

In D1588#63746, @drummyfish wrote:

Adding a placement tool clickable icon might be a solution - could it be like this?

• Add a Place object tool button to the toolbar (alongside the other main tools).
• The Place tool would allow placing the object that's currently selected in the objects list (even if the object list tab wouldn't be active, e.g. you could have the "Map" tab active and still be able to place objects with the tool).
• If no object is currently selected in the list (after program start), simply grey the icon out? Or make it just do nothing? Don't know here.

It can be the same as the the paint tool for terrain (to be consistent). If a texture isn't selected, then a terrain would be painted with a default selected texture.

In D1588#63738, @Stan wrote:

Can't we do something like a fake click ?

It's not good. It's a workaround.

I'd only prefer for a better name, but we didn't decide it. And I don't see problems in the code. So the patch is ok for me.

In rP21850#31034, @elexis wrote:

UPD.

What's that?

I edited the comment few minits later (UPD = Update).

In rP21850#31029, @elexis wrote:

A sprintf that we have in 60 places may not be ideal but is also not a workaround.

I didn't tell about sprintf_s at all in the last sentence, but about modifying the same buffer with hardcoded offsets.

In rP21850#31027, @elexis wrote:

insecure construction of std::string

If you mentioned a hypothesis other than platform depending missing null-termination in sprintf_s that can cause the function to not return the md5 hash or behave unintentionally, then I didn't comprehend it.

If it works now it's not the reason to add a non-standard workaround. But do you agree that it was wrong to commit it without this discussion in the diff (especially without reviews)?

In rP21850#31025, @elexis wrote:

I don't see the criticism that isn't founded in personal preference of code style.
I can use a stringstream voluntarily to please your personal preference of code style and perhaps even become a subscriber to that code style if the imposition ceased.
But I won't use a stringstream to fix a defect in this revision unless the claimed defect is substantiated against the indication.

The problem is that you used the code from tests. The problem is that you didn't see difference between single write in a buffer and multiple and have insecure construction of std::string. The problem is that you didn't wait my approve or full review.

In D1583#63717, @elexis wrote:

"Reviewers: Player 1" says that the patch has been or is supposed to be reviewed by "Player 1".
So if you set yourself as the only reviewer, then that sets you the reviewer of the patch.
If you don't intend to review the entire patch, say so in advance and don't set yourself to a reviewer unless completing the list of reviewers.
If there is noone that can perform a review of the rest of the patch, then the right thing to do is to post all the comments (such a the "const") without setting oneself as a reviewer.

I don't set myself as a reviewer, it happens automatically, because I mark it as Needs Changes. I agree with your point, I will removing myself from reviewers manually in similar cases after my objections will be completed.

In D1583#63713, @elexis wrote:

There are multiple ways to set oneself as a reviewer. One is intending to comprehend, examine and participate in the feature design, software design and implementation, testing stage, future maintenance. The other is not intending to do any of that but only marking one thing one noticed as a requested change and then not noticing that this property set oneself as a performer of the mentioned tasks.

Thanks, I'll fix it after the FF.

In rP21850#31004, @elexis wrote:

0123456789ABCDEF is not only one hardcoding, it's hardcoding of 16 different pieces of information that can fail individually, can be and are abstracted by the sprintf.

It's not different 16 pieces, it's logically related pieces. You said you don't want to add hardcoded things, but you did add it. Without review. With my Needs changes. You have std::stringstream without so hardcoded numbers, but you didn't use it.

I agree with @elexis, we don't need to prefer the one type of files. The compressed version of a mod is less portable. The security reason is nothing for the open-source project, because you can just modify the code a little bit to easily extract things.

In rP21850#30995, @elexis wrote:

The code comes from the MD5 test file which is tested on a number of platforms since years.
(If there was an explanation as to when how this could break, the UserReporter sprintf may also be considered.)

static const char base16[] = "0123456789ABCDEF";

I'd consider this the hardcoding anti-pattern, so I don't really share the opion that that piece of code is nicer.

I can change it, but then for a good reason.

You have 2 implementation depended hardcoded numbers, it's more than 1. Also you get it from the test code, instead of the production code. It's not good.