Ah, you copied that hunk directly from JSI_Simulation::GuiInterfaceCall, that's why there is an ENSURE and the name cxSim etc.

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

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

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

In D2195#91642, @wraitii wrote:

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?

First - I don't understand why you're getting so worked up about this

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, accusing me of not understanding references, accusing me of being disingenous, making strawmans and proposing to change the coding conventions to use pointers when it can use references.

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

Fixes compiler warnings from @Stan.

Updates:

• Removed duplicate functions for getReplayMetadata to more directly call the fn from the GuiInterface.
  • Updated VisualReplay::SaveReplayMetadata to call GuiInterface directly from the C++. After the code change, I removed the (now unused) ScriptInterface argument from the method signature.
  • Replaced the single invocation of getReplayMetadata with Engine.GuiInterfaceCall("GetReplayMetadata")
• Added the registration of GetInitAttributes which should have been included in the original diff.
• Added myself to the credits

Thanks for the feedback! I added some responses inline and will be updating the diff (hopefully addressing all the remaining issues)!

In D2196#91706, @elexis wrote:

So the idea is that it should use m_ViewCamera (from rP3405) instead of g_Game->GetView().

Not quite right, m_ViewCamera is the current camera of Renderer (which can be changed during frame rendering, currently for shadow maps), but here we need exact values used for the final frame before postprocessing.

Did further testing.
Don't believe the tests.
The results of the test above depend on the order of the tests.
If I measure the old function being run 500 times, then the new function being run 500 times, I get the exact opposite result when I measure the new function first and then the old function afterwards.
Even with GC at each start.

As @Stan said, there is more discussion on the PR on GitHub. The goal of this contribution is to provide the fundamental underlying capabilities for exploring RL/ML agents in 0 AD (such as an OpenAI gym environment which can actually be implemented on top of this quite easily). I am intentionally not adding any action/observation spaces or reward as expected by OpenAI gym as these are not obvious and there are many different candidates for each of these (this is discussed more on the GitHub thread).

• Moved group data from ProductionQueue.js to Identity.js to remove duplication.
• Moved the training from seperate group entry to the entities entry.
• Added Cost to the template so that one can choose its own cost for the group.

I performed a new test, and found there is a catch.

The Falcata were committed in rP22743 I believe.

Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/444/display/redirect

(Spaces in map file names again; see D1042.)

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

Notice that the previous code explicitly converted to T& or const T& due to the previous function signatures of RecurseObject, whereas the new code expects references.
This is mostly due to template deduction: Nothing except Args&& compiles, not Args... args, nor Args&... args, nor Args const&... args, and always with the same complaint about not being able to deduce the correct template.

../../../source/gui/IGUIObject.h:249:7: note: candidate template ignored: deduced conflicting types for parameter 'Args' (<SGUIMessage &> vs. <SGUIMessage>)

@anileapen05 still interested in working on this ?

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.

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

The commit message seems incomplete to me, I don't see any proofs for "so that the 50+ users stop wondering" in the commit message nor the diff description.

rP22587 removed 21 of the if (GetGUI()) checks already, sometimes with a throw, sometimes with ENSURE, most often silent pass, so it is not only manifested in my personal brain.

Oh wait, the use case is for the simulation?
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?
From a quick look it seems that the VisualActor component determines alone what it wants to do with the actor?

you have to manually specify visible garrison prop points, while you could use the ones defined by artists in the mesh.

Oh wait, the use case is for the simulation?
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?
From a quick look it seems that the VisualActor component determines alone what it wants to do with the actor?

@Freagarach that's strange, can you try to figure out what was null and shouldn't have been ?

(There is a catch with proposing unused code for review: It can't be tested against the use cases. But code looks ok.)

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

Patch looks a lot cleaner already, doesn't it? The committed one as well.

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

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

Thanks for the feedback! I can move it to be behind a feature flag and remove the generated files.

Committed cleanups separately in rP22738.

Maybe some more insights can be found here https://github.com/0ad/0ad/pull/25

In D2205#91665, @elexis wrote:

But I do remember precisely you recommending me to add if-safeguards for other places in the code even after I mentioned that it's always given, and then you even convinced me that it's a good idea to better be safe than sorry when it comes to null-deref.

I suppose nothing was changed. You have to use safeguards if you don't have guarantees of a pointer lifetime.

Thanks guys for discussing this at length, though it got quite heated in the end. It's important that we keep exchanging even when it seems that no one will ever be right.

What about the test results from the patch, and the feasibility of machine learning in 0ad?
The example AI can move an army but not much more? What is it's incentive (how is it training?)?
What is the prospect of the feature?

In D2205#91661, @vladislavbelov wrote:

In D2205#91651, @elexis wrote:

Fix the problem at the root and everything else follows along.

I can't say that the patch solves the problem at the root. It doesn't solve a real problem of GUI existence but it adds a visual confidence that the interface try/should to guarantee that.

The one m_pGUI(pGUI) line per class is not really adding any painful duplication either, it's not replicating and modifying some logic, it's just member initalization.

GetGUI is not really adding any painful duplication as well.

I would say that is quite affordable given that the cost is a quarter line per file and the benefit is not having to worry about null anymore.

I don't remember that someone (except you) whenever had serious worries about that.

Most of the code in source/gui/ has not been touched since 2004, so the sample size is limited.
And gandhi said "Even if you are a minority of one, the truth is the truth."
It doesn't matter whether it's me, you or anyone else, it's pattern, these things are true for everyone alike.
But I do remember precisely you recommending me to add if-safeguards for other places in the code even after I mentioned that it's always given, and then you even convinced me that it's a good idea to better be safe than sorry when it comes to null-deref.
So regardless, if anyone gets a pointer, he is forced to check whether it can be null or not, but by looking for comment stating that,
whereas with a reference it's impossible to even think if it can be dangling unless one questions the validity of the entire stack. At least in this code.
So I'm afraid I can't agree to change to prefer T* over T& if we know it's alwas non-null non-dangling.

In D2205#91651, @elexis wrote:

Fix the problem at the root and everything else follows along.

I can't say that the patch solves the problem at the root. It doesn't solve a real problem of GUI existence but it adds a visual confidence that the interface try/should to guarantee that.

The patch is looking like it's going into a healthy direction.

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.

Changed NULL to nullptr