User Details
- User Since
- Dec 19 2016, 10:38 PM (282 w, 22 h)
Aug 26 2021
Here is a key for signing A26 mods.
Jun 14 2021
Mar 4 2021
I'm OK with removing JSHint. Originally the linters were supposed to be complementary, one for the style, one for the logic. But nowadays both detect both kinds of issues, and there is a risk of conflict.
Feb 12 2021
Jan 25 2021
Superseded by SM upgrades.
This change needs D3470 🙂
Nov 14 2020
The SVN change is wrong. If SVN is broken, the differential pipeline should try to build upon a working version, so you must not run svn up. If you want to change the paradigm and always build upon the latest SVN, the pipelines should be entirely rewritten.
Nov 5 2020
Very nice improvement, some major comments though ??
Oct 29 2020
Thanks a lot for the quick review!
Oct 28 2020
Address Freagarach's comments.
The code change is correct and, apparently, works under SM45 and SM52.
Yo Stan, I don't see the need for MesonBuild support, I would recommend creating a Trac ticket with "If Time Permits" urgency, a link to this diff in the Patch field, and then abandon this diff for the time being.
Sep 17 2020
Hi there! I rebased https://github.com/na-Itms/0ad/tree/spidermonkey upon this. Please test it, along SM52 ? I'll update the forum post whenever I have the chance.
Sep 5 2020
I disagree with the change for the same reasons as Angen and Freagarach. IMO the only meaningful change we should make is: as part of D11, move the files we use to a new relevant place, and delete the others (moving them to the art repo). For instance the campaign test map should become an actual test campaign within the new campaign framework.
It does look like threading Atlas adds unwanted complexity. Atlas still freezes when the simulation hangs (for instance when changing the terrain with the pathfinder upgrade, we had to figure out a way to improve performance so that Atlas wouldn't lag). So it doesn't seem like the threading is useful. If it adds bugs on top, removing it sounds sensible to me.
Aug 18 2020
Rebase
Debug tests do work, I closed #3753. The current diff is now running on Jenkins, I still have some planned work for posting build warnings as comments.
Aug 17 2020
@wraitii, @vladislavbelov, what do you think about the spaces-around-operators issue? If you agree with me, I will move forward with D2490 (see some discussion there) and try to find a way to fix this one issue with clang-format. If you both think spaces everywhere are better, I can activate this instead.
Just like the others, it would be nice to have that functionality in-engine (mod validation could even happen as part of the archivebuilder workflow!)
Aug 16 2020
The code is good and it fixes a TODO. I also tested it in game and the snapping along the sides now work ??
Aug 15 2020
Aug 11 2020
Ha sorry! I really have to test it, then ?
Nice addition. I only reviewed the math/code, didn't test yet.
I think so too. As Vlad told me yesterday, the style linter will not manage to detect enforce everything in our coding conventions (for instance proper naming of variables).
For my part if it does the trick why not. Why do we need spaces around binary operators?
You misunderstood. Sometimes we need to NOT have spaces. For instance, for calculating a 2D distance, x*x + y*y is more readable than x * x + y * y (the relevant binary operator is * in this case). However, clang-format forces the use of spaces, and there is no option to change that, as of today. The detection of missing spaces is optional in astyle.
Aug 10 2020
Proposal to use Artistic Style instead of clang-format.
Here is a working version using Coala (not enabled in .coafile here).
@irishninja, could you take a look at the issues Vlad mentions? We should have detected them before, sorry ? You can read the IRC logs for a few more details.
Slight fixes.
Post build warnings as Phabricator comments, so that they are not missed in case of a successful build.
Post lint as inline comments.
Aug 7 2020
I am testing a few Jenkins improvements on this diff, sorry for the noise!
I am testing a few Jenkins improvements on this diff, sorry for the noise!
Aug 6 2020
Adapt LicenseYearBear to the current version of the python package svn.
Can I commandeer this? ?
I am restarting the build here to pass the fixed cppcheck on this. Sorry in advance if there is a false negative..
Obviously.
Fix on Python 3.8 (sent upstream https://github.com/coala/coala-bears/pull/2976)
Fix caching in a docker container (not sent yet)
Display file name of cppcheck results
Hi Stefan, thanks for this patch. Indeed, we are merely providing a bundled version of premake, and we try to keep it as close as possible from upstream.
Aug 5 2020
Address coala warnings.
Aug 4 2020
Correct dockerfile.
Move files in a way that arcanist understands.
Wouldn't it be better to include it in RLInterface.cpp? It is when compiling that file that the inclusion is missing. I am a bit confused by the issue, just as I was confused during the review ?
Aug 3 2020
That's interesting, but IMO, if this is a C++17 feature, we shouldn't use it (yet) regardless of what the MSVC documentation says. It's not always trustworthy. Of course, if the bug is present in current Visual Studio, it is an issue. But I am totally fine with the current code after rP22617 and rP22636 ?
Aug 2 2020
Yes, the more I look at it the more I'm convinced this is the issue. I really can't do the rebase today but should be possible to see whether the crash is fixed in the upcoming days. ?
By the way, a better name for the diff and for a future commit would be Fix rooting mistake in GuiPoll{New,Historic}Messages. But it would be great to add a fixes #5655, of course ? I'll rebase the branch upon this commit so that we can test if the crash is fixed.
Ha, this makes far more sense, thank you! ?
The root of the issue might be around this part of the code, but I'm not convinced with the patch.
Aug 1 2020
Update with the version from the SM52 git migration branch.
Jul 31 2020
Thanks again for the great work ?
Jul 30 2020
I do not know the current unit motion as well as wraitii and Freagarach do, so I'm trusting Freagarach's review here ? The C++ changes do not seem problematic at first sight.
Very fun tool to play with, great job! And the code looks pretty nice, it's self-contained and easy to read.
Jul 29 2020
Use CanBarter rather than HasMarket.
Test cmpPlayer and add a unit test for cmpPlayer.SetPlayerID while I'm at it.
Jul 28 2020
Fixed Jenkins lint.
Jul 25 2020
Hey! I'm the one at fault here.
Jul 20 2020
I believe that you can also delete the SetSerializablePrototypes and IsSerializablePrototypefrom the (de)serializer, as you don't replace them by anything. They were only called by this AI code.
Hi wraitii, while I was looking into those serializable prototypes the other day, I discovered that most of this code is currently dead. I do not know if you noticed, I didn't see this mentioned above.
Jul 17 2020
Thanks for the help asterix but this is not reviewable, I just uploaded it to be able to show it to other devs. In his branch, Bellaz changed the key type slightly, which could be a possible fix but does not match upstream. I am going to try a different way based on my discussion with SM devs.