Page MenuHomeWildfire Games

vladislavbelov (Vladislav Belov)
User

User Details

User Since
Feb 3 2017, 10:51 PM (96 w, 3 d)

Recent Activity

Fri, Dec 7

vladislavbelov added a comment to rP21943: Update client\'s default.cfg for the new muc room `arena23b`.
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.

Fri, Dec 7, 3:38 PM

Thu, Dec 6

vladislavbelov added a comment to rP21943: Update client\'s default.cfg for the new muc room `arena23b`.
In rP21943#31673, @Stan wrote:

To me people should upgrade.

Only if needed.

Thu, Dec 6, 9:25 PM
vladislavbelov updated subscribers of rP21943: Update client\'s default.cfg for the new muc room `arena23b`.
Thu, Dec 6, 7:32 PM
vladislavbelov updated subscribers of rP21943: Update client\'s default.cfg for the new muc room `arena23b`.

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

Thu, Dec 6, 7:31 PM

Thu, Nov 29

vladislavbelov added a comment to D1684: Pass callback function to PushGuiPage and unify PopGuiPage/PopGuiPageCB.
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

Thu, Nov 29, 5:21 PM
vladislavbelov added a comment to D1684: Pass callback function to PushGuiPage and unify PopGuiPage/PopGuiPageCB.
In D1684#66778, @elexis 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 :)

Thu, Nov 29, 4:09 PM
vladislavbelov added a comment to D1684: Pass callback function to PushGuiPage and unify PopGuiPage/PopGuiPageCB.

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).

Thu, Nov 29, 2:58 PM
vladislavbelov added a comment to D1683: Removes hardcoded SkyBox sizes and use infinity sky.
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.

Thu, Nov 29, 1:33 PM
elexis awarded D1683: Removes hardcoded SkyBox sizes and use infinity sky a Like token.
Thu, Nov 29, 11:02 AM
vladislavbelov added inline comments to D1683: Removes hardcoded SkyBox sizes and use infinity sky.
Thu, Nov 29, 8:42 AM
vladislavbelov added inline comments to D1683: Removes hardcoded SkyBox sizes and use infinity sky.
Thu, Nov 29, 8:19 AM
Angen awarded D1683: Removes hardcoded SkyBox sizes and use infinity sky a Like token.
Thu, Nov 29, 7:19 AM

Wed, Nov 28

vladislavbelov created D1683: Removes hardcoded SkyBox sizes and use infinity sky.
Wed, Nov 28, 11:52 PM

Sun, Nov 25

vladislavbelov added a comment to D1680: Do not hardcode screen ratio in the main menu..

Did you test it on really wide monitors/windows?

Sun, Nov 25, 12:11 PM

Sat, Nov 24

vladislavbelov added a comment to D1682: Cleanup RallyPointRenderer.

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

Sat, Nov 24, 5:40 PM

Fri, Nov 23

vladislavbelov added a comment to D1678: Fix Warnings of VS2015 .

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.

Fri, Nov 23, 9:31 AM · Windows Developers

Thu, Nov 22

vladislavbelov added a comment to D406: Add a steps param to sliders.
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.

Thu, Nov 22, 12:51 AM

Wed, Nov 21

vladislavbelov added a comment to D406: Add a steps param to sliders.
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.

Wed, Nov 21, 11:53 PM
vladislavbelov added a comment to D1668: Rely on copy-elision / return-value-optimization for lobby string getters.
In D1668#66513, @elexis 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.

Wed, Nov 21, 11:14 PM
vladislavbelov added a comment to D1023: Improves the replay list interface.
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)

Wed, Nov 21, 9:41 PM
vladislavbelov added a comment to D406: Add a steps param to sliders.
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.

Wed, Nov 21, 9:39 PM
vladislavbelov added a comment to D1515: Split CColor from Shapes.
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.

Wed, Nov 21, 9:32 PM
vladislavbelov added a comment to D1676: Embed zpl-c's version of enet library.

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

Wed, Nov 21, 9:29 PM
vladislavbelov added a comment to D1437: Disable shadows if we can't create a shadow map.
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.

Wed, Nov 21, 9:22 PM
vladislavbelov added a comment to D1668: Rely on copy-elision / return-value-optimization for lobby string getters.
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.)

Wed, Nov 21, 8:43 PM

Mon, Nov 19

vladislavbelov added a comment to D1676: Embed zpl-c's version of enet library.

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

Mon, Nov 19, 2:30 PM
vladislavbelov updated subscribers of D1676: Embed zpl-c's version of enet library.

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

Mon, Nov 19, 2:30 PM

Sun, Nov 18

vladislavbelov added inline comments to D1672: Cleanup of SkyManager.
Sun, Nov 18, 1:08 AM

Wed, Nov 14

vladislavbelov added inline comments to D1667: Lobby ScriptConversion to remove stanza property name hardcoding and duplication.
Wed, Nov 14, 5:32 PM

Mon, Nov 12

vladislavbelov added inline comments to D1672: Cleanup of SkyManager.
Mon, Nov 12, 8:54 PM
vladislavbelov added inline comments to D1672: Cleanup of SkyManager.
Mon, Nov 12, 5:09 PM

Nov 10 2018

vladislavbelov added inline comments to D1670: Remove references to globals.
Nov 10 2018, 11:35 AM
vladislavbelov added inline comments to D1670: Remove references to globals.
Nov 10 2018, 10:37 AM

Nov 9 2018

vladislavbelov created D1672: Cleanup of SkyManager.
Nov 9 2018, 10:10 PM
vladislavbelov added a comment to D1670: Remove references to globals.

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

Nov 9 2018, 7:42 PM
vladislavbelov added a comment to D1669: Remove hardcoded minimum OSX version in premake5.

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

Nov 9 2018, 7:37 PM

Nov 1 2018

vladislavbelov added a comment to D1662: Define the maximum number of players pyrogenesis can handle in a match..
In D1662#65759, @nani wrote:
In D1662#65757, @smiley wrote:

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

Nov 1 2018, 2:43 PM
vladislavbelov added a comment to D1662: Define the maximum number of players pyrogenesis can handle in a match..
In D1662#65755, @nani 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.

Nov 1 2018, 1:44 PM
vladislavbelov added a comment to D1662: Define the maximum number of players pyrogenesis can handle in a match..
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.

Nov 1 2018, 1:26 PM
vladislavbelov requested changes to D1662: Define the maximum number of players pyrogenesis can handle in a match..

Why 32 in C++ and 24 in JS?

Nov 1 2018, 11:59 AM

Oct 30 2018

vladislavbelov added inline comments to D1642: Allow to manipulate the heightmap in simulation.
Oct 30 2018, 2:44 PM
vladislavbelov added a comment to D1600: Forces SSL for the UserReport.
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/

Oct 30 2018, 1:38 PM
vladislavbelov added inline comments to D1642: Allow to manipulate the heightmap in simulation.
Oct 30 2018, 12:44 PM
vladislavbelov added inline comments to D1621: Removes active values from UserReport.
Oct 30 2018, 11:02 AM

Oct 29 2018

vladislavbelov added a comment to D1600: Forces SSL for the 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/)

Oct 29 2018, 5:01 PM
vladislavbelov added inline comments to D1621: Removes active values from UserReport.
Oct 29 2018, 5:00 PM
vladislavbelov added inline comments to rP21723: Define and associate .pyromod filetype with pyrogenesis on Linux and Windows..
Oct 29 2018, 11:44 AM

Oct 28 2018

vladislavbelov added a comment to D1650: GridBrowser GUI addon.

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.

Oct 28 2018, 10:48 AM
vladislavbelov added a comment to D1637: BoolArray array wrapper..

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

Oct 28 2018, 10:29 AM
vladislavbelov added a comment to D1642: Allow to manipulate the heightmap in simulation.

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.

Oct 28 2018, 9:40 AM

Aug 27 2018

vladislavbelov added inline comments to D1621: Removes active values from UserReport.
Aug 27 2018, 12:08 PM

Aug 26 2018

vladislavbelov added inline comments to D1621: Removes active values from UserReport.
Aug 26 2018, 2:12 PM
vladislavbelov updated the summary of D1621: Removes active values from UserReport.
Aug 26 2018, 3:38 AM
vladislavbelov created D1621: Removes active values from UserReport.
Aug 26 2018, 3:36 AM

Aug 25 2018

vladislavbelov added inline comments to rP21868: Write UserReport data to local logfiles, so that users can review the personal….
Aug 25 2018, 11:18 PM

Aug 17 2018

vladislavbelov added a comment to D1578: Basic refactoring of FSM.
Aug 17 2018, 11:01 PM

Aug 15 2018

vladislavbelov raised a concern with rP15576: Commit coastal waves when activating fancy effects, and incidentally completely….
Aug 15 2018, 10:53 PM
vladislavbelov raised a concern with rP12300: ao/parallax/normal/specular/emissive mapping; windy trees; time manager; render….
Aug 15 2018, 10:48 PM
vladislavbelov raised a concern with rP10704: Rendering marker lines between buildings and rally points.
Aug 15 2018, 10:02 PM

Aug 10 2018

vladislavbelov added a comment to D1608: Show a more helpful message upon an empty response from mod.io.
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

Aug 10 2018, 11:19 PM

Aug 8 2018

vladislavbelov accepted rP21850: Persist the lobby Terms Of Use and Terms Of Service checkbox if the logged in….
Aug 8 2018, 10:25 PM

Aug 7 2018

vladislavbelov added a comment to D1606: Escape the backslash within the in-game manual.
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.

Aug 7 2018, 1:59 PM
vladislavbelov added a comment to D1606: Escape the backslash within the in-game manual.

Probably we need to report about unsupported escaped symbols.

Aug 7 2018, 10:44 AM

Aug 6 2018

vladislavbelov accepted D1591: Consistently use Hexify to convert u8* to hex strings.
Aug 6 2018, 10:41 PM

Aug 5 2018

vladislavbelov added a comment to D1591: Consistently use Hexify to convert u8* to hex strings.
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.

Aug 5 2018, 8:16 PM

Jul 29 2018

vladislavbelov updated the diff for D1600: Forces SSL for the UserReport.

Updates the year.

Jul 29 2018, 11:29 PM
vladislavbelov updated the diff for D1600: Forces SSL for the UserReport.

Adds the Mod.IO support.

Jul 29 2018, 11:27 PM
vladislavbelov added inline comments to D1600: Forces SSL for the UserReport.
Jul 29 2018, 11:22 PM
vladislavbelov updated the diff for D1600: Forces SSL for the UserReport.
Jul 29 2018, 11:21 PM
vladislavbelov created D1600: Forces SSL for the UserReport.
Jul 29 2018, 11:20 PM
vladislavbelov accepted D1597: Remove unused exploitable JS SubmitUserReport function.
Jul 29 2018, 9:04 PM
vladislavbelov added a comment to D1591: Consistently use Hexify to convert u8* to hex strings.
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();
}
Jul 29 2018, 4:33 PM

Jul 7 2018

vladislavbelov added inline comments to D1492: Abort instead of throwing an error when dealing with empty areas..
Jul 7 2018, 2:51 AM

Jul 6 2018

vladislavbelov added inline comments to D1492: Abort instead of throwing an error when dealing with empty areas..
Jul 6 2018, 4:46 PM

Jul 5 2018

vladislavbelov added a comment to D1593: Patches required to work with latest clang on FreeBSD.

Could you attach your errors?

Jul 5 2018, 9:17 PM

Jun 29 2018

vladislavbelov added a comment to D1588: [Atlas] Allow returning to current selection to place objects .

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.

Jun 29 2018, 8:02 PM
vladislavbelov added a comment to D1591: Consistently use Hexify to convert u8* to hex strings.

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.

Jun 29 2018, 7:56 PM
vladislavbelov awarded D1591: Consistently use Hexify to convert u8* to hex strings a Like token.
Jun 29 2018, 7:43 PM

Jun 28 2018

vladislavbelov awarded D1590: 0 A.D. Empires Ascendant Multiplayer Lobby Privacy Policy a Like token.
Jun 28 2018, 2:50 PM

Jun 26 2018

vladislavbelov added a comment to D1588: [Atlas] Allow returning to current selection to place objects .

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.

Jun 26 2018, 8:13 PM
vladislavbelov added a comment to D1588: [Atlas] Allow returning to current selection to place objects .
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:

Jun 26 2018, 2:20 PM
vladislavbelov added a comment to D1588: [Atlas] Allow returning to current selection to place objects .
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?

Jun 26 2018, 12:54 PM
vladislavbelov added a comment to D1588: [Atlas] Allow returning to current selection to place objects .

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.

Jun 26 2018, 10:45 AM

Jun 25 2018

vladislavbelov added a reviewer for D1588: [Atlas] Allow returning to current selection to place objects : Restricted Owners Package.
Jun 25 2018, 8:12 PM
vladislavbelov added a comment to D1588: [Atlas] Allow returning to current selection to place objects .
In D1588#63738, @Stan wrote:

Can't we do something like a fake click ?

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

Jun 25 2018, 8:09 PM

Jun 24 2018

vladislavbelov accepted D1583: JSInterface TextureExists function, fixes map specific biome previews.

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.

Jun 24 2018, 6:30 PM
vladislavbelov added a comment to rP21850: Persist the lobby Terms Of Use and Terms Of Service checkbox if the logged in….

UPD.

What's that?

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

Jun 24 2018, 12:12 PM

Jun 23 2018

vladislavbelov updated subscribers of rP21850: Persist the lobby Terms Of Use and Terms Of Service checkbox if the logged in….

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.

Jun 23 2018, 9:49 PM
vladislavbelov added a comment to rP21850: Persist the lobby Terms Of Use and Terms Of Service checkbox if the logged in….

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)?

Jun 23 2018, 7:56 PM
vladislavbelov added a comment to rP21850: Persist the lobby Terms Of Use and Terms Of Service checkbox if the logged in….

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.

Jun 23 2018, 6:40 PM
vladislavbelov added a comment to D1583: JSInterface TextureExists function, fixes map specific biome previews.
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.

Jun 23 2018, 6:31 PM
vladislavbelov added a comment to D1583: JSInterface TextureExists function, fixes map specific biome previews.
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.

Jun 23 2018, 5:34 PM

Jun 22 2018

vladislavbelov added a comment to rP21612: A little cleanup of the PatchRData.cpp..

Thanks, I'll fix it after the FF.

Jun 22 2018, 6:55 PM
vladislavbelov raised a concern with rP21850: Persist the lobby Terms Of Use and Terms Of Service checkbox if the logged in….

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.

Jun 22 2018, 6:32 PM
vladislavbelov added a comment to D1586: Allow mod packagers to not bundle XML files.

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.

Jun 22 2018, 5:30 PM
vladislavbelov added inline comments to rP21850: Persist the lobby Terms Of Use and Terms Of Service checkbox if the logged in….
Jun 22 2018, 5:12 PM
vladislavbelov raised a concern with rP21850: Persist the lobby Terms Of Use and Terms Of Service checkbox if the logged in….

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.

Jun 22 2018, 4:42 PM
vladislavbelov added inline comments to D1584: Seed random sounds.
Jun 22 2018, 9:15 AM
vladislavbelov added inline comments to D1584: Seed random sounds.
Jun 22 2018, 1:46 AM

Jun 21 2018

vladislavbelov added inline comments to rP21850: Persist the lobby Terms Of Use and Terms Of Service checkbox if the logged in….
Jun 21 2018, 9:08 PM