Thanks for the patch!

Final version currently in place on Jenkins.

In D1942#80673, @wraitii wrote:

Alright, then the release code is wrong too imo. UB shouldn't be called, for one thing, but also I worked under the assumption that m_JumpPointCache.find(passClass) would always return a valid, if empty, vector. That's what the code in the Hierarchical Pathfinder does. IMO we should change this code to initialise the jump point cache with the passibility classes, instead of doing two bounds checks.

In D1942#80633, @wraitii wrote:

Mh, this is weird. I would gladly add an "ENSURE" there that there is a possibility class for the C++ code, this means we do something differently in release and debug mode and I don't like it.

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.

In D1935#80249, @Nescio wrote:

Then let's use š in Old Persian.
Also, could you create a page https://trac.wildfiregames.com/wiki/SpecificNames, so I could write down a few things there?

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.

In D1935#80241, @Nescio wrote:

The specific names are specifically non-English, so I'm not sure that style guide would be the appropiate place. I vaguely recall @Itms stating he wanted to write a page on transliterations months ago, but undoubtedly he's busy with other things.

You can upload with Planned Changes and I'll test.

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?

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!

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.

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).

This is working on my macOS VM and on Linux, and the code looks perfect. 👍

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.

@Angen Marking for you the places where the scripts are actually cut on Jenkins.

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.

In D1844#77459, @wraitii wrote:

One thing I'm curious about: there is no way for C++ code to delete a "PERMANENT" property? That seems dubious.
Looking at properties, it appears SM38 has "IGNORE_PERMANENT" attributes: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_Reference/Property_attributes
This seems like the kind of thing we should use here, and we can still make these globals permanent.

In D1850#77454, @wraitii wrote:

I'm assuming this is post SM45 ?

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.

In rP22247#33237, @Stan wrote:

I guess we only need +player on 202 that's where the warning comes from.

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.

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?

In D1754#76520, @Stan wrote:

She added: "Itms president !"

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".

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.

In D1844#76466, @elexis wrote:

But hotloading in the simulation context means something different in the GUI context. In the GUI context one can be hotloading currently but it doesn't matter whether hotloadable is set to true or false?
And some caller other than the ComponentManager may need to replace somehow constant globals without being in hotloading context.
To me it seems like a https://en.wikipedia.org/wiki/Separation_of_concerns issue if the hotloading isn't shown to be (1) universally applicable to any hotloading mechanism and (2) no other mechanism wants to trigger the code flow that hotloadable triggers.

In D1844#76445, @elexis wrote:

1. If I understand correctly, the SetGlobal hotloading mechanism is only relevant to the simulation, yes?

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.

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.