- User Since
- Dec 19 2016, 10:38 PM (122 w, 3 d)
This is removing state.jpc = m_JumpPointCache[passClass].get(); when it != m_JumpPointCache.end(), no?
Wed, Apr 24
This looks very good to me, and it works both on SM38 and SM45.
Tue, Apr 23
I concur 🙂 I am committing this myself now, and lifting the concern I raised.
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.
Mon, Apr 22
Thanks for the quick comments!
Some comments for helping the reviewers ❤
Sat, Apr 20
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 😉
Thu, Apr 18
Tue, Apr 16
This new method should be const. But apart from that this looks good to me.
Mon, Apr 15
Sat, Apr 13
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.
Wed, Apr 10
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.
Tue, Apr 9
Sun, Apr 7
@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?
Thu, Apr 4
If you want go ahead! Else I can commit it in an hour or two
Wed, Apr 3
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.
Tue, Apr 2
Oof this one is old. Many sorries for the awful queue 🙁
Some inline details.
Mar 27 2019
Mar 17 2019
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.
Mar 16 2019
Works for me, thanks for this patch!
Mar 10 2019
Feb 25 2019
It's a nice script, but is it useful now that the linter detects those wrong license dates? What would be the use case?
Feb 24 2019
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 🙂
Feb 7 2019
Jan 15 2019
@elexis I sent you a forum PM, please let me know if you want to use a different place for discussion.
Jan 13 2019
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.
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.
Jan 12 2019
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
Jan 6 2019
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).
Jan 5 2019
Jan 4 2019
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.