In D1754#76083, @elexis wrote:

(Also am I the only one using libboost 1.69.0-2 or am I just the only one too stupid to resolve the build errors? I better get them resolved if I want to commit my own patches)

This is removing state.jpc = m_JumpPointCache[passClass].get(); when it != m_JumpPointCache.end(), no?

This looks very good to me, and it works both on SM38 and SM45.

I concur 🙂 I am committing this myself now, and lifting the concern I raised.

In D1772#75866, @elexis wrote:

Mostly I'm wondering what Itms had to say about this patch, as Stan, Itms and Tobbi had created the release candidate, but this patch was somehow not committed with the others.

I'm at work so I only have Linux, but if Stan doesn't get around to it I can test tonight.

I think that if we keep supporting 2.8 we need to build patches with it, else we will commit a lot of 3.0+ changes without noticing. I believe the overwhelming majority of distributions used by our devs and contributors ships 3+.

It's indeed planned (but not effective yet so we should fix this or we won't have autobuild). The actual measure that I should take is to put into place patch testing on the Windows autobuilder. Will post a related message on the forums later today.

Thanks for the quick comments!

Some comments for helping the reviewers ❤

In rP22204#32955, @Stan wrote:

No problem, I believe Itms will add it to the automatic scripts.

I think the patch doesn't apply because of the spaces in file names. Nescio is working on a fix for the latter (D1042), if you have some time to steal the review from me that would be a perfect "thank you" to Nescio for reviewing this one 😉

This new method should be const. But apart from that this looks good to me.

In D1772#74964, @wraitii wrote:

I don't actually care enough to understand why this is required.

In rP22184#32826, @Itms wrote:

If the math isn't trivial, it should be done somewhere, so people don't have to rely on trusting you (especially in a few years when they stumble on the code).

In rP22184#32821, @wraitii wrote:

As commented, left as an exercise to the reader. The math isn't trivial.

In rP22184#32823, @wraitii wrote:

@Itms Any idea why this failed? Compiled fine and ran on my machine.

Yes, both changes are coming in different patches, probably this weekend. This is a very early WIP, you can ignore it 🙂

Fixed by Nescio in rP22183.

Thanks for the quick fix! 👍

Hi Lancelot 🙂 Let's not change the minimum version, we worked hard enough during the rerelease to make sure we could keep 10.9 as the minimum. Apart from that, if it works for you I can accept.

While trying to add the nice improvements from rP22096 to Jenkins, I got this error:

Error parsing structures/spart_syssiton: Duplicate child node 'Classes' at Entity.pm line 45.

I am afraid using those correct typography symbols (which cannot be found easily on the majority of PC keyboards) is going to cause big issues for translators, and cause inconsistencies when developers keep using the usual ascii symbols. On the other hand, using correct typography would be nice, and we have indeed no technical limitations here.

Looks good! 👍

@bb Thanks for the remarks! Regarding the name disappearing or changing too stupidly:

• If users want to use very strange nicknames on Transifex, we can't do a lot.... It's worth noting that in Transifex the username is used by default, but you can choose to supply a different name to appear in the po files ("Nicolas Auvray" instead of "Itms" typically). So some of the changes might be from people deleting this extra information.
• If all translations of a user have been overwritten over time, they will disappear from the po file. If that is the case for all resources, we will loose trace of them. I agree that in theory we should keep them in the credits (for code or art, even if all changes from someone are superseded later, we thank them for contributing). However, we have no possibility to automatically detect if a name removal is a name change, or all translations from a user obsolete. So we would have, for instance, both Nicolas Auvray and Itms in the credits (stupid). There is also a privacy issue: if someone wants to remove their real name from the credits, they will do so on Transifex, but they won't be warned that we keep old names in the credits, and they won't know they should contact us.

This looks good to me now. Can somebody test on XCode (I don't have it on my VMs) and commit if it works as expected?

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

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

The message looks good to me 👍 Maybe use https in the first URL for consistency, but that's a nitpick.

Oof this one is old. Many sorries for the awful queue 🙁

In D1741#73409, @Stan wrote:

@Itms any concerns with this patch ?

Some inline details.

Oops I forgot to accept first. Seems like I can't now :(

The patch works for me! And I agree with the implementation. May I suggest that, instead of adding those two new booleans, you make the local dir variable in RotateTick an attribute of the class? You can rename it m_RotationDirection and modify it directly in OnKeyOverride. That would make the patch shorter and the code cleaner.

Works for me, thanks for this patch!

Related to D1495, so you might want to take a look @Angen.

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

Looks good to me, just fix the Frankenstein sentence in the readme before committing 🙂

Some small improvements needed, sorry...

Finally tested it 😅 It works on the Jenkins slave, you can commit it!

I'm letting Vlad handle this patch after the revamp of the feedback page 🙂 I'm still subscribed here.

Just tested it and it still seems to work pretty well 🙂

In D1042#71075, @Nescio wrote:

Still to be decided:
1 what to do with the scenarios/Height Map *.png files
2 what to do with the three Azure Coast scenarios (identical maps, different player numbers)
3 what to do with the two Flight_demo scenarios (different maps, similar function)
4 what to do with the two Sandbox - Ptolemies scenarios (different maps, similar function; all other factions have only one sandbox)
5 how to differentiate similary named skirmishes, e.g. Corinthian Isthmus (different maps, similar names)

How about the following?

• 2, 3, 4: keep only one version, delete the others
• 5: rename them to _small and _large

@elexis I sent you a forum PM, please let me know if you want to use a different place for discussion.

It would be nice to have something that detects such infinite loops and creates JS errors instead of engine crashes.

I'm rebasing this one, and I'll try to include the binaries for Windows in the diff.

In rP22023#32208, @Angen wrote:

@Itms no logs no dump files, no errors. Just crash.

That's wild! This commit should not be able to create a segfault, so it probably just reveals a bug in the engine. The commands file should be enough to find the bug and fix it.

I think _p or m_p would be acceptable names since it's one letter.
This might conflict with D1395, which I should prioritize.
The other warnings shall be fixed by upgrading Boost and/or deleting old unneeded dlls.

Okay, you're right. Go ahead and thanks for the patch!

I think we do not need to include boost::system because we don't use anything from it, but we need to link it because boost::filesystem depends on it. I think only the build-osx change should be kept, as bootstrap is clever enough to build the boost::system lib while now excluding it from distributed includes.

This doesn't work for me. Linking of pyrogenesis fails with undefined references to boost::system stuff. I will retry tomorrow with a clean checkout.

The patch looks good and it works for me too! However, I tested building Atlas and got the following warnings:

• C4458 in AtlasObjectImpl.cpp line 292 and in AtlasUI/Object.cpp:547
• C4456 in MapDialog.cpp:173 and in ScenarioEditor.cpp:742

Addressed comments above, and tested the JSPROP_PERMANENT thing.

Ah right, sorry. But I'd be interested in knowing how much of the cases are actually ent == INVALID_ENTITY. It doesn't sound like there are a lot of cases where an entity would be deleted several times?

I really think all the things that happen after this early return are inexpensive. In particular, when posting messages, if the entity is actually fully destroyed, no component subscribed to the message will be found, so there won't be a lot of overhead. If it's not actually fully destroyed, the patch is wrong (I do not think that's the case, but I might be wrong).

In D14#69171, @Kuba386 wrote:

Well, button said "resign as reviewer". I've only intended to cancel my old review that is not correct anymore.
I'm still working on this.

I think this is a good change and I share the opinion that removing the "always entering"/"not always entering" distinction would help tracking down some subtle bugs.

I could not send this commit on Phabricator for automated testing, so please report here if any bug related to that is found.

I do not know why I thought we were using alpha10 of premake5: we are using alpha 12. The fix above was included in alpha13, that is correct.

This still works for me, with the same results. I didn't test a mod but the cli flag seems to work for the public mod as advertised.