Looks neater. I don't think I had a particularly good reason to use recursion here.

Think this looks fine.

I didn't realise we had this feature... I'm in favour, random should be random.

TryToConnectWithXmpp -> TryToConnectViaLobby to match the SetupConnection call (and because XMPP is basically the lobby client).

GetCameraDirection -> GetCameraOrientation for consistency with C++

@edoput How do you want to appear in the credits?

In D4682#199519, @Stan wrote:

Oh my bad then. Just thought it'd be nice if someday we decide to have proper dependency injection.

Seems like you could fix a few listing problems while at it, but this gets a +1 for being a good idea.

Per https://trac.wildfiregames.com/wiki/Coding_Conventions, we have Prefer global variables over singletons, because then they're not trying to hide their ugliness. It's definitely something I kind of like, because singletons by design make it annoying to have two of something, and that can occasionally be useful.

In D4679#199184, @Freagarach wrote:

That sounds like good play then? And reckless play from the defender? There are walls to fend off these kind of raids?

Think this is generally a fairly good first use case, but I believe you could write simpler code, particularly since this bit doesn't really require us to optimise it to death.

In D4676#199132, @chrstgtr wrote:

Maybe this is too simple, but why don’t we just switch default unit behavior to destroy instead of capture and adjust health and control metrics to reflect how we want them? [...] It seems like it an all around win.

I'll make a minor tweak to avoid double-messages but I think this looks good

Lint

This should fix that case as well.

Thanks Stan

Probably won't have time to fix it until Monday FYI

In D4681#199032, @vladislavbelov wrote:

It doesn't fix the ticket, so the ticket must not be closed. But the fix might be committed as soon as accepted until we have a proper solution.

I've written about this fairly extensively, and I think the main problem with capturing is that it's boring as hell. Your units don't even fight. I actually think the only legitimate use is sneak-capture. We need a complete revamp of the 'capture' logic to represent breaking-in, defender vs attacker losses, and control of the building as a non-abstract concept IMO for it to work.

default in the cpp

In D4675#198841, @phosit wrote:

The = defaultwas ok but not in the header file.

Add a specific test for multi-space words. It is indeed not collapsed.
I've de-activated the 'ignore space' logic in those cases, kind of arbitrarily. It seemed more coherent.

Remove the TaskManager destructor change, merging once green.

In D4673#198809, @jprahman wrote:

Looking at the details, the ip and port values inferred in the three conditional blocks are not used in the actual connection attempt. But rather those two values are only used to establish a Stun tunnel.
For that reason, I'm not convinced that QueryConnectionDetails() would be semantically accurate for the name. Instead, something like SetupStun() would more accurately reflect the intent.

You're right, I forgot how this bit worked.

In D3877#198820, @jprahman wrote:

Hey @wraitii are you planning on cleaning up and landing these changes soon? Asking since I'd like to pull in changes with the AddRecurrentTask API and can either wait for this to land, or put up a diff with an updated version of TimerThread :)

In D4672#198721, @marder wrote:

generally I think not _everything_ needs to be a setting, especially when looking at the complex setup that we have already, but since there is no consensus about what the right way is, I guess that might be ok.

I need to get some more time to actually think about the implications here, but I think this looks generally workable.

Hey, thanks for the patch. This does simplify the control-flow, particularly the last block that is kind of awkward.
I think however it would be more natural to have a QueryConnectionDetails function that encapsulates the ip/port finding logic, and call that (which can return immediately if g_xmppClient is not there), then simply setup the connection in the flow. That method should probably be a class method of CNetClient.

In D4662#198715, @vladislavbelov wrote:

What happens if there multiple spaces in the end? It needs at least tests.

Only remove the 'player defeated' plot twist.

*L" " to L' '

Thanks for fixing this Stan, didn't have time

I realise this is getting somewhat bikesheddy, but a more complete response:

General high level notes:

I need this information in game to set a default rotation during the game. The current "reset camera" feature resets the camera rotation to the default values present in the config but these values are not available in game for the user to set.

Unrelated, but this sounds like you could just read the config values?

In D4660#198512, @edoput wrote:

Should I open another differential with only that changes?

In D2938#198471, @asterix wrote:

Will there be an update on this, since it was committed for a minimap?

It sounds like this is just reporting those warnings, yeah, so I think you can go ahead

I ran into an actual bug while updating the tests following rP26889, gonna add tests in the fix for that instead.

In D4661#198340, @marder wrote:

hm actually, what about a diplomacy with no last man standing?
Imagine you're playing with a large wonder holding time and you decide to add a new ally after building a wonder. Doesn't it seem unfair that you get punished for doing so?

Mh, yes. The scenario I had in mind is the reverse: you are allied to some guy, then you remove that alliance at the last second, and you instawin because the timer isn't reset.

The tests now show an asymmetry between the left & right align, which I don't think was there before. Need to investigate.

In rP21441#46499, @Freagarach wrote:

Has a topic or a ticket for this been made? For I could not find it.

There is some related code in D3507.

In D4653#198209, @Stan wrote:

How about the projectile code ?

In D4653#198191, @Stan wrote:

That still sounds like something that should be tested. I wonder if the speed loss going through JS interfaces would not be mitigated by having a very fast component.

Yes, that's a bit of a question.

(This is exactly why I push for D3886)

In D4653#198028, @Stan wrote:

So it's not worthwile at all?

Recap of a talk with Mozilla SM devs today on their Matrix chat:

• JS-WASM & JS-C++ interop can ultimately be expected to have about the same 'cost'. A dev said that the large cost was that IonCompiled code needed to dump registers in a GC-aware manner, in case the C++/Wasm call triggered a GC.
• C++ <-> WASM interop is currently not implemented, and would have to go through JS, which I think makes the whole idea too inefficient right now. Though implementing c++ <-> wasm direct interop might be relatively easy to implement.
• Given that we're gonna need the above in any case for the engine, this sounds like a somewhat complex path to optimisations.

In D4530#197931, @vladislavbelov wrote:

In D4530#197910, @wraitii wrote:

God this code is a mess.

You're talking like other code is significantly better x)

God this code is a mess.

This one should work

Still learning lua

More debug