Sorry I was wrong - I thought the missing Session destructor fixed it, but the Jingle class also needs one. Too much nesting!

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 doesnt change semantics, does it? Only syntax.

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

Yeah, sorry about the confusion. The tooltips are not working as supposed to be, hence broken. But for now they're not ;)
I'll update the name of the patch.

In D2138#92011, @elexis wrote:

This patch fixes the tooltips after D2092/rP22754.

Fixes the tooltips? Did that commit break them?

Successful build - Chance fights ever on the side of the prudent.

Uses GetSetting variant from rP22693.

Uses GetSetting variant from rP22693.

Uses GetSetting variant from rP22693.

Revision history:

Successful build - Chance fights ever on the side of the prudent.

Thanks for the notification and bisect!

The commit broke button hover (in session) and minimap camera dragging (at least on Windows). As far as I see, we shouldn't reset states for visible objects.

Comment about the duplication.

Build failure - The Moirai have given mortals hearts that can endure.

This patch fixes the tooltips after D2092/rP22754.

Fixes the tooltips? Did that commit break them?

Successful build - Chance fights ever on the side of the prudent.

Rebased.

Build failure - The Moirai have given mortals hearts that can endure.

• Reverted cost calculation.
• Finish the spawning
  • Ugly but working. Feedback appreciated!

Successful build - Chance fights ever on the side of the prudent.

In D2081#91977, @wraitii wrote:

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

You're welcome!
B.t.w. I have already a patch for the new tooltips but it ought to be updated.

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.

I don't have a clean working copy, so I can't test soon.

So the important part is this, this way the ugly monster condition that copies the CGame constructor argument logic doesn't have to be replicated, kept in sync, and then bugfixes multiple times without ever becoming evidently correct, unless doing it this way:

Index: source/ps/Game.cpp
===================================================================
--- source/ps/Game.cpp	(revision 22752)
+++ source/ps/Game.cpp	(working copy)
@@ -100,10 +100,13 @@ CGame::~CGame()
 {
 	// Again, the in-game call tree is going to be different to the main menu one.
 	if (CProfileManager::IsInitialised())
 		g_Profiler.StructuralReset();

Successful build - Chance fights ever on the side of the prudent.

Also address Stan's comments.

Successful build - Chance fights ever on the side of the prudent.

Notice that there are 36 occurrences of CRenderer::IsInitialised() after this patch (and 30 before), so it's not a new thing to test for this at all.
It just shows that we made up some random concepts in our mind ("visual mode") that doesn't really exist as something different from "renderering enabled".

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.

Fixed rP16727, the commit is a good example why default arguments should be avoided. The commit added the default argument because the other argument was a default one too, from rP7653.

Successful build - Chance fights ever on the side of the prudent.

Use Singleton method CRenderer::IsInitialised() instead of adding a Renderer boolean getter and doing something that crashes everything immediately all the time.

Successful build - Chance fights ever on the side of the prudent.

Test whether the renderer is open instead of passing this information around under different names.

I don't mind much what our personal opinions are, it matters what the effect on future readers is that have never seen or met us.
So we need to stop arguing with saying "I like X", since it hides the argument.
A disadvantage of enum flags is that they define a new data structure and that they require parsing that is sometimes done uglily using switch and even loops.
But in this case those are only two booleans, two bits, so it should be possible to get away with one arg & const each and it provides a readable name.
So I'm afraid the flags are preferable in this single instance even if you had the opposite opinion or if flags are worse in other cases.

Successful build - Chance fights ever on the side of the prudent.

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

I will be updating the diff with the most recent fixes.

It seems you didn't see the inline comments and the patch?
JSI_Simulation::GetInitAttributes is out of place in ComponentManager, and furthermore it can be deleted :-)

Made stuff useful for training optional, so that the cost can also be calculated based on the individual entites to spawn. I think however that that is not viable, since one can deduce from the amount of resources subtracted which entities are to spawn. So I guess I'll have to revert this change when I find the time again to make Cost obligatory again.

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?

Cleaned up the updates to the SaveReplayMetadata function (mentioned in the prior reviews). I also made the argument for the fn const since it is read-only.

I will be updating the diff with the most recent fixes. Dumb question: when I update the diff, does it add the changes to the existing diff or overwrite it with the most recent diff? I usually use git and GitHub and am still getting used to Phabricator...