Which version of boost starts support it?

I suppose we won't loose distribution here, at least we won't notice it.

The WELL512 usage was introduces in rP10446 along a bug fixing. So the only question is: would be enough to have the mt19937 to prevent the bug.

mt19937 seems ok for me, but I'm worrying about performance. Could you test how many calls of the UploadPropertiesAndPlay function we have on the biggest map with biggest number of units per second?

In D1821#74573, @Stan wrote:

That's a test so it doesn't matter much.

Actually we run all tests on our server for each diff, so it does matters if it significantly slower.

What's about performance?

In D1584#74541, @Stan wrote:

Maybe we should use boost after all https://stackoverflow.com/questions/20998470/random-numbers-c11-vs-boost

@Angen thank you for the patch!

I tested the patch and it works. Since the problem is reported by cppcheck (probably by some compiler too) I don't see a reason to refactor the whole file, because it's a bit other problem.

In D1584#74336, @Stan wrote:

Does setting the seed reset the rng ?

In D1584#74323, @Stan wrote:

Can't really make it static since we always want to generate the same number for a given Actor seed.

In D1584#74317, @Stan wrote:

https://pastebin.com/mFhKWCAu

In D1584#74312, @Stan wrote:

1000000 Samples

Could you attach the source?

In D1584#74289, @Stan wrote:

How should I test it ? I'm not really confident with profiling :)

But! mt19937 could be a bit slower of faster than the previous solution for many sounds, could you test it?

I tested the patch, it works well. We already use mt19937 but from boost. I don't see any strong reason to use the boost one. Maybe someone wants to replace them by the std:: version in future. By https://trac.wildfiregames.com/wiki/CppSupport all supported compilers support <random>.

In D1816#74161, @Angen wrote:

clamp has int as parameters (32 bits), std::max returns only 8 bits so might not bee needed. But I am not sure about that.

Clamp is a template function, so a compiler deduce a type from its arguments.

The model works now without any error, but the berries looks too polygonized. Did you smooth normals?

In D1799#74092, @Itms wrote:

If you want go ahead! Else I can commit it in an hour or two

In D1799#74058, @Itms wrote:

Still good after testing! The text improvement is a good idea yeah 🙂

In D1799#74045, @Itms wrote:

Do you want me to test on Windows @vladislavbelov?

As you want, I just wanted you check the text :)

The C++ part is clear for me.

In D1809#73950, @elexis wrote:

Then add it to CC.

CC is to allow people to chose between multiple syntactically and semantically correct solutions.
It's not a convention that we avoid bugs.

But CC helps to make less mistakes.

In D1809#73924, @elexis wrote:

This commit is not a cleanup, but a bugfix with some additional cleanup.
So it makes sense to add a comment so that people don't reintroduce the bug.

Then add it to CC.

In D1809#73904, @elexis wrote:

But even better might be adding the comment to the code itself, so that people stubmle upon it while reading the code.

About what? It's too common thing and it's related to many places in the code. So I'd prefer to add comments to our CC.

I got an error on the African Plains map:

ERROR: Could not load mesh 'art/meshes/gaia/berry_bush_02.dae'
ERROR: CObjectEntry::BuildVariation(): Model art/meshes/gaia/berry_bush_02.dae failed to load

I suppose you plan changes here.

In D1493#70804, @Stan wrote:

@vladislavbelov Any comment on this patch ?

Did you test it? There is a chance that some values on edges may lead to a strange behaviour.

Could you create a patch with context: https://trac.wildfiregames.com/wiki/SubmittingPatches#Makingsomechanges? It helps a lot when you're reviewing, thank you. Currently it's harder to me to review a logic without context.

In D1809#73829, @elexis wrote:

Tests?

I plan to add it later. I have a big queue. But there is no logic change.

In D1804#73657, @elexis wrote:

I'm sure we can find a wrong formula that can give the right results too in a blackbox test.

It's true, our water shader has a lot of things to improve.

@Itms any thoughts?

In D1571#73534, @elexis wrote:

If you ask if WFGs policy allows for unreviewed commits, then I wonder why you ask, after you saw me committing hundreds of unreviewed patches and always arguing for that freedom, other people committing unreviewed patches this release, and last but not least the lengthy forum discussions to reinsure this freedom, last one at https://wildfiregames.com/forum/index.php?/topic/25169-post-a23-commit-frenzy/. One of my arguments is that granting a developer access to commit patches of people who can't be trusted (external contributors) implies trusting the developer to decide over the quality of a contribution singlehandedly. Another argument is that there is a big difference in quality between reviews and peer reviews. If the author knows the code in question much better than anyone else, the reviews won't be very effective nor efficient. On the other hand reviews cost time for the reviewer - if the reviewer wants to do a good job, it costs similarly much time as compared to writing the patch.
So it depends on the specific patch, on the active members and their knowledge of the affected code whether and how much a review benefits, and it depends on good taste and luck to see which patches would be better proposed first and which ones are simple and solid enough to go in sooner than later.

Yeah, I understand your point. My point is that everyone mistakes, and waiting for more review sometimes decreases a mistake chance. But sometimes it takes too much time.

In D1571#73526, @elexis wrote:

If you want to commit this patch to continue working on greater features that depend on this, and if you have done your best to ensure the correctness, I would suggest to take that decision to commit into your hands.

Edit: You can add "Comments By:" if you can't post "Reviewed By:"

In rP22137#32613, @elexis wrote:

Reviewed By has the use case that one can grep it, but I was more refering to Reviewed By: <comitter>, where <comitter> is the person that committed it.

Oh, I missed that part, sorry. I understood it incorrectly.

In rP22137#32598, @elexis wrote:

(Reviewed by: <committer> always seemed redundant to me.

I think a possible use-case of the Reviewed By is when someone made a review remotely from phabricator, like via IRC/voice chat/live (for example on conference).

@wraitii @elexis Removed m_Smoothness getter until we have something finished with config.

Fixes Stan's notes.

The patch looks good to me.

In D1753#72433, @Itms wrote:

It's a nice script, but is it useful now that the linter detects those wrong license dates? What would be the use case?

In D1698#72244, @Stan wrote:

It's still using the 2017 toolset so yes.

In D1745#72226, @elexis wrote:

So you're asking for AddIcons instead of AddIcon.

Actually no, though AddIcons is better than AddIcon. I think these functions are dangerous currently.

In D1698#72240, @Stan wrote:

the goal is to drop support eventually and move on to 2015 2017

It's true, but we didn't drop it yet.

In D1698#72235, @Stan wrote:

Errors for debug build are here https://trac.wildfiregames.com/ticket/4460

Ok, it should be linked then (I added).

In D1698#72233, @Stan wrote:

Updating Fcollada is not that important. Nevertheless though our debug build is broken on windows because of it and the game doesn't work with Fcollada compiled with vs2015 instant default.

So I thought we might as well take all the fixes from the other forks over the internet.

Do we have to update our fcollada? Especially if it has useful changes.

In D1745#72176, @elexis wrote:

So the numbers show us that displaying 250 new textures means that 250 more textures are processed, but that's not surprising, is it?

Of course.

Currently few of my comments should be fixed anyway, so I add a "Request changes". But also I want to discuss about a method to pass the ping event to the C++.

What's about wraitii suggestion:

If not, I would recommend passing an enum VFS::SORTED or something instead of true or false since C++ doesn't have named arguments

It makes sense to use a name. It'd be easier to read such code.

As we discussed XML is a pretty specific case and it may have some problems. But I can suggest to add a more common parameter, like an ignore mask. Example:

... --ignore-mask="*test.js|*.xmb"

But it also requires a motivation.