In D1670#92067, @elexis wrote:

It doesnt change semantics, does it? Only syntax.

Mh, yes.

I don't believe this change is worth committing as it stands, as Vlad said this only changes semantics (arguably making them worse since the new code is longer).
It's ever so slightly annoying for svn-blame to change so many lines, and to me the cost vs win calculation is negative here.

This needs a new rebase.

Thanks for the split, much easier to be confident that we aren't introducing weird bugs now :)

Thanks for taking a jab at this :)

In D2138#92011, @elexis wrote:

This patch fixes the tooltips after D2092/rP22754.

Fixes the tooltips? Did that commit break them?

Done in D2092. Thanks for the patch which gave me the idea for that one :)

@bb: I've committed rP22754, making "Damage" and "Capture" not based on the attack names, allowing you to drop that from this diff.
I think you'll find my final decision OK with regards to how templates get written.
If not, feel free to discuss this by PM or on a forum thread or through the audit feature or something.

Also address Stan's comments.

Allow calling an arbitrary method per damage type. This means we can give and remove status effects neatly, or have attacks that actually heal, or anything really.

I would prefer using enum-flags to make these parameters more readable then.

On the whole, I think this is a minor feature that's useful for performance reasons, and we shouldn't worry too much about future-proofing these kind of things, which are easy to change, and generally used in specific use-cases (see also the shore-dock code thingy).

In D2195#91852, @vladislavbelov wrote:

@wraitii any thoughts about updated patch? Is it acceptable?

Because you have wasted several hours of my lifetime, telling me that my patch is bad because it changes code without you understanding the point of it

I think I've perfectly understood your patch and I still think it's bad (or rather, not worth committing). I have to guess at this point that you believe this to be so unthinkable that you must assume I've misunderstood your patch to reconcile that with reality somehow?
You still haven't acknowledged this remark:

In D1989#91742, @elexis wrote:

That sounds like it might be problematic, because the simulation is supposed to be independent of the graphics data.
So not sure if this is right then.
Are there other precedences where the simulation state is dependent on actors?

I think projectiles start at the first ammo point, which is defined in the actors, not in templates. That seems to make sense to me - so that's a strong precedent that prop-points are part of the simulation.

I give you
void foo(T* foo)
void foo(T& foo)
Now you tell me, for which of the two functions do you wonder whether you receive a null argument?

Well, only the first, but you actually get no guarantee that foo exists with either. That's all I'm saying.
Indeed, you weren't arguing for that, indeed what you argued was that:

This patch now changes the pointer to a reference, so that it is absolutely clear to everyone that we do not need to check whether the CGUI page exists or not.

And as I've said, this feels like the smallest gain. I don't think it's worth changing so many lines and introducing a member reference (which has the highest likelihood of dangling, among references), over.

Yes, that property is true for the pointer as well, which is the reason why I wrote the patch to change the syntax to better reflect the semantics.

In D2195#91641, @elexis wrote:

In D2195#91636, @wraitii wrote:

In D2195#91617, @vladislavbelov wrote:

@elexis In my opinion yes to mostly all your So this should *. I think it's more clear. But currently I don't see a strong reason to modify the code only for reference <> pointer transformation.

So for T* it's obvious that it can be modified and for T& it is not obvious that it can be modified?

Thanks for the strawman, how often more do you want to tell me again that some other pointer or reference can crash that is not the code that we talk about?
How often will you tell me more that some other code that is not the code we speak about can crash?

In D2195#91617, @vladislavbelov wrote:

@elexis In my opinion yes to mostly all your So this should *. I think it's more clear. But currently I don't see a strong reason to modify the code only for reference <> pointer transformation.

Sorry, but you don't get it.
Either you decide for a paradigm where CGUI is always allocated by design, and thus chose a reference,[...]
or you decide for a design where CGUI is optional and thus chose a pointer.

No actually I don't have to pick a "paradigm" here. Both the reference and the pointer code are completely valid and functional. Why you want to add 54 nullability checks is your problem, you're changing perfectlyy working code

It's tricky to keep calm if people say that your patch is wrong without having read the affected code or brought fourth a single argument specific to the patch.

I'd agree with vlad that non-const references are probably trickier than non-const pointers. Const references are preferable to pointers however.

I have a vague memory of @vladislavbelov doing something similar, I'll try and find out.

Hey, thanks for contributing this on Phabricator :)

Looks alright, but calling it a 'refactoring' is a bit misleading imo, I'd avoid it in the commit message :)

Fixed remark above.

In D2188#91103, @Stan wrote:

What's the difference between this and a status effect ?

Add Promotion and ResourceGatherer to Transform.js, and rewrite the Promote test to be about promotion, not about Transform.js.

Do it in CanvasJS and D3 instead, Highcharts was too slow.

Merged back in D274

Optimised after some profiling. I'm now rather confident that this is generally as fast as SVN, and much faster for global auras.

so, I think it's better to just detach() and let it live. It'll either clean up on its own or get cleaned up by the OS when exiting, and that's likely fine.

Cleanly exit to avoid hanging in upnp Discovery.

Fix test issue, fix remarks.

So what's the "new" stack model that this patch introduces exactly?
Is the current GUI Model that we can only have one "axis", so pushing 5 times from the same pages results in:

• Main -> Page1 -> Page2 -> Page3

Instead of the (more expected):

Main -> Page 1
     -> Page 2
     -> Page 3

?

Add <condition_variable> header.

Fix debug. RC.

Clean up some of the weirdness.

Merged back into D274

m_MapSize ought to be signed but that sounded like it'd change many more lines.

Ran some profiling, and I'm not actually sure this speeds things up. But it's still better behaviour as there are some artefacts with the existing code on e.g. Polynesia near the edges, and it's more readable imo so I'll commit anyways.

Abandoning since won't get committed.

emplace_back instead of push_back to avoid the braces, and init the move inline.

Make ordering consistent:

• PlayerChanged is sent
• Entities are changed owner from invalid to player
• player ent is destroyed

Working version, worked around the fact thats SDL semaphores keep a count of how many times they were asked to wake up.

Clean up formatting.

Cleaned up.

Add include, RC

Feels like this should be the default to me to be honest

As an FYI: I confirm this is working on my machine, but don't expect all 4 cores to be 100% CPU, pathfinding is only a small part of it.

Bring back the early returns;

Left a change in D274 incorrectly.

Un-nuke the commas.

Ran some more profiling, this seems comparable with svn. I think it's a tad slower as there's slightly more indirection, but that ought to be irrelevant in cost. MultiKeyMap could be switched to C++ I guess for best performance.

Fix formatting

Rebased for commit.

Ran a few rejointests, all passing, which indicates that the serialisation keeps the deterministic non-linear order correctly, so this is good to go.

Not sure what was missing or too much, but it's fixed now.

Both concerns fixed.