- User Since
- Dec 19 2016, 10:38 PM (131 w, 15 h)
Sun, Jun 16
Thanks for the patch!
Sat, Jun 8
Tue, Jun 4
Final version currently in place on Jenkins.
Mon, Jun 3
Sun, Jun 2
Well well well fixes always need more fixes it seems. Dead code is the worst.
Good work! I am currently testing the pipeline on the autobuilder with the following changes.
Fri, May 31
There are no rules yet. For Latin I find the diacritics for vowels pretty, and since it's a transliteration I guess we should put them. For Sanskrit I am used to ś, sh doesn't bother me though.
You can upload with Planned Changes and I'll test.
Thu, May 30
Wed, May 29
Very nice! I'm proposing the following changes. They are either changes from the scripted pipeline syntax to declarative, or improvements that we could make to the current bat script (so, not a criticism towards your work). The only bug/puzzling thing is the absence of the repo address, which makes me think like this pipeline shouldn't work? Unless you checked the repo out manually once?
Tue, May 28
This works perfectly from me now, and the code still looks good 👍 I think the build error comes from the fact that the premake change makes the pch files invalid. I should improve Jenkins so that it performs cleanups when premake scripts are touched, but this is not trivial. I have an alternative fix in mind that I am implementing these days. Anyway, thanks for the great work on this!
May 26 2019
There is your issue. I haven't fully reviewed the rest yet.
Regardless of the future merge between runtimes and context, I think we should use persistent rooteds as little as possible, so conceptually I agree with the patch.
May 25 2019
Thanks for the fix.
Wait for the build, but yes from me.
Let's just wait for the build to finish: spoiler alert: it will warn about the copyright years 😛
I restarted the build, let's see.
Sorry, my reproduction steps were not correct. What I actually tried was adding a char that is outside the first block (ξ, i.e. U+03BE). From my research, this doesn't fail in Atlas, but in CSimulation2::GetMapSettingsString(), so this is a SpiderMonkey bug, not related to your patch. It should be fixed later, I'll try to have a look as part of the SM update.
What Stan said, unless the complete type is actually very long (not on desktop, I can't check).
May 23 2019
This is working on my macOS VM and on Linux, and the code looks perfect. 👍
May 21 2019
Very nice, thanks for the work 👍
With three small improvements/cleanups, this looks good to me. You can fix them and then commit. 👍
@wraitii Would you have time to test this on a mac?
Thanks again for the patch, now I finally have some time to dedicate to it!
Ah actually yes, I agree with elexis. EntityExists is better. Something bugged me about the name and I couldn't pinpoint what.
I just did some quick profiling on the patch. I don't see any difference between patched and unpatched versions on "Combat demo (huge)", so the perf gain is probably minimal. It's not bad to have it in though, and the new method might be useful in the future.
May 20 2019
@Angen Marking for you the places where the scripts are actually cut on Jenkins.
May 19 2019
If you have five minutes I really think you could be interested in that code, especially the comments in c44465f2a483, and how it actually not ignores the permanent property in general but only in a very specific case.
May 8 2019
Ha I was testing and I can't accept now, but it's fine for me 👍
Yeah as long as we don't need to break out of the loop, forEach is fine. I don't see a real need for it here, but if it's more to your tastes, why not. It's just lengthening the verification of the fix a bit, because it makes it less trivial 😊
I agree that this looks nice at first sight. I just followed Philip's concept regarding clearance, and then did some tuning to reduce the number of stuck units... This should be tested a lot but I don't see any conceptual issues with this.
Hi! Thanks for the input @Nescio. The DD is still a very experimental WIP but if you want to collaborate with Dok you are most welcome. It is located here on Phabricator: rDD. I am adding you to the group which can see differentials related to the DD.
As detected by the linter, you should use return and not continue in a function. The current code doesn't even parse.
May 7 2019
I think it would be better to revert to the original for style like Polakrity suggests. for...in is better suited for dictionaries, and adding a + everywhere player is used in that function sounds like a pain and people might make the mistake again.
May 2 2019
Apr 28 2019
UGH why can't it display the test results. Back to you.
Ah of course. I thought the (3) was about this patch specifically.
For tens of months before being banned, on a regular basis, unhelpful comments in the fashion of "there is a defect, I'm not helping you further" were made. Even if leper was allowed to answer here, he wouldn't (except if he has changed since then).
Sorry, which 3?
Well I agreed with all points of this patch, and I just looked for other possible uses of the new SendEventToAll and found none that relied on this workaround. It's possible that the code can be used in another place that we didn't see (in which case we'd only loose the opportunity to fix it sooner), but it seems incorrect that the patch "misses something".
Apr 27 2019
This is the patch rebased and with the second arguments renamed to data.
I understand the feeling, but unfortunately we can do nothing except keeping developing the game with everything that we have and that we can actually access. Your work is certainly not crap (quite the opposite), your contributions and the ones from fellow WFG members are enough to keep creating 0 A.D. 🙂 Chin up! 💪
This is the version of the patch where I make all globals hotloadable. I also reverted currentlyHotloading to replace in order to make the script interface agnostic of the hotloading concept.
@bb Could you create a ticket for that? It sounds like we should indeed take a look at this and make the use of OwnershipChanged over Destroy consistent, and put explicit comments where one is needed over the other. 👍
Thanks for performing the changes and sorry for not going back on this immediately. I agree with the merging of the functions, because we indeed often use OwnershipChanged for gameplay stuff whereas Destroy is seen as a rather low-level message. Additionally it's always better to associate miraging code with ownership concepts.
Thanks for clarifying the report 😉 I'm fixing it on Transifex.
Thanks a lot for the input! I pondered a long time with the question of whether SetGlobal should get this or that parameter while writing the patch. Based on the comments I'll remove one of the parameters, which will make the patch a bit more simple.
Apr 25 2019
This is removing state.jpc = m_JumpPointCache[passClass].get(); when it != m_JumpPointCache.end(), no?
Apr 24 2019
This looks very good to me, and it works both on SM38 and SM45.
Apr 23 2019
I concur 🙂 I am committing this myself now, and lifting the concern I raised.