Page MenuHomeWildfire Games

elexis (Alexander H)
User

Projects

User Details

User Since
Dec 21 2016, 3:52 PM (139 w, 1 d)

Recent Activity

Today

elexis updated the summary of D2215: Delete copy-assignment GUIUtil GetSetting variant.
Fri, Aug 23, 11:44 AM
elexis retitled D1670: Singleton g_Game and g_SoundManager from Remove references to globals to Singleton g_Game and g_SoundManager.
Fri, Aug 23, 11:02 AM
elexis added a comment to D1670: Singleton g_Game and g_SoundManager.

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.

Fri, Aug 23, 10:47 AM
elexis created D2215: Delete copy-assignment GUIUtil GetSetting variant.
Fri, Aug 23, 5:57 AM
elexis removed 1 auditor(s) for rP22695: Remove IsBoolTrue helper function from rP7289.: vladislavbelov.
Fri, Aug 23, 5:48 AM
elexis added a comment to rP22758: Fix missing actual boolean check in the commit removing the odd way to check….

Uses GetSetting variant from rP22693.

Fri, Aug 23, 5:48 AM
elexis added a comment to rP22756: Introduce IGUIObject::PlaySound to unify 19 copies of the UI sound play….

Uses GetSetting variant from rP22693.

Fri, Aug 23, 5:48 AM
elexis added a comment to rP22761: Remove hardcoded C++ fallback font from rP1074 and according TODO from rP1085….

Uses GetSetting variant from rP22693.

Fri, Aug 23, 5:47 AM
elexis committed rP22761: Remove hardcoded C++ fallback font from rP1074 and according TODO from rP1085….
Remove hardcoded C++ fallback font from rP1074 and according TODO from rP1085…
Fri, Aug 23, 5:28 AM
elexis closed D2214: Delete broken fallback font for broken fonts.
Fri, Aug 23, 5:28 AM
elexis added a comment to D2214: Delete broken fallback font for broken fonts.

Revision history:

Fri, Aug 23, 5:17 AM
elexis created D2214: Delete broken fallback font for broken fonts.
Fri, Aug 23, 4:41 AM
elexis requested verification of rP22695: Remove IsBoolTrue helper function from rP7289..

Thanks for the notification and bisect!

Fri, Aug 23, 2:42 AM
elexis committed rP22758: Fix missing actual boolean check in the commit removing the odd way to check….
Fix missing actual boolean check in the commit removing the odd way to check…
Fri, Aug 23, 2:42 AM
elexis committed rP22757: Mark some GUI functions as const, including the boolean ones from rP22744..
Mark some GUI functions as const, including the boolean ones from rP22744.
Fri, Aug 23, 1:51 AM
elexis added inline comments to rP22744: Use variadic template for RecurseObject from rP31 and move it from GUIutil to….
Fri, Aug 23, 12:46 AM
elexis committed rP22756: Introduce IGUIObject::PlaySound to unify 19 copies of the UI sound play….
Introduce IGUIObject::PlaySound to unify 19 copies of the UI sound play…
Fri, Aug 23, 12:34 AM
elexis closed D2209: Unify 19 copies of IGUIObject::PlaySound and stop copying the according CStrW.
Fri, Aug 23, 12:34 AM
elexis updated the summary of D2209: Unify 19 copies of IGUIObject::PlaySound and stop copying the according CStrW.
Fri, Aug 23, 12:15 AM
elexis added a comment to rP13521: Adds UI sounds for buttons, dropdowns, lists, and checkboxes, fixes #948.

Comment about the duplication.

Fri, Aug 23, 12:07 AM

Yesterday

elexis created D2213: Delete CSimulation2 m_MapSettings and redundant getters.
Thu, Aug 22, 11:48 PM
elexis added a comment to D2138: Tooltips for D2092/rP22754..

This patch fixes the tooltips after D2092/rP22754.

Fixes the tooltips? Did that commit break them?

Thu, Aug 22, 11:26 PM
irishninja awarded D2197: Add support for recording replay metadata when in nonvisual mode a Like token.
Thu, Aug 22, 9:29 PM
elexis created D2212: Show system_info.txt path in terminal when it's written.
Thu, Aug 22, 8:16 PM
elexis added a comment to D2197: Add support for recording replay metadata when in nonvisual mode.

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();
Thu, Aug 22, 7:28 PM
elexis added a comment to D2211: Clean CGame constructor arguments.

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

Thu, Aug 22, 7:10 PM
elexis updated the summary of D2211: Clean CGame constructor arguments.
Thu, Aug 22, 5:33 PM
elexis added inline comments to rP19645: Add a -autostart-nonvisual option. Patch by sacha_vrand. Fixes #4577..
Thu, Aug 22, 5:32 PM
elexis added a comment to rP17689: Don't create replays without commands in case of running non-visual replay or….

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.

Thu, Aug 22, 5:29 PM
elexis added inline comments to rP18613: Always save the replay metadata (summary screen info) when ending the….
Thu, Aug 22, 5:16 PM
elexis updated the diff for D2211: Clean CGame constructor arguments.

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

Thu, Aug 22, 5:04 PM
elexis added inline comments to rP22432: Adds a possibility to disable saving of replay in autostart mode..
Thu, Aug 22, 5:02 PM
elexis added inline comments to D2211: Clean CGame constructor arguments.
Thu, Aug 22, 4:37 PM
elexis updated the diff for D2211: Clean CGame constructor arguments.

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

Thu, Aug 22, 4:32 PM
elexis added a comment to D2211: Clean CGame constructor arguments.

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.

Thu, Aug 22, 4:30 PM
elexis created D2211: Clean CGame constructor arguments.
Thu, Aug 22, 3:43 PM
elexis commandeered D2197: Add support for recording replay metadata when in nonvisual mode.
Thu, Aug 22, 12:58 PM
elexis added a comment to D2197: Add support for recording replay metadata when in nonvisual mode.

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

Thu, Aug 22, 12:26 PM
elexis accepted D2197: Add support for recording replay metadata when in nonvisual mode.

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

Thu, Aug 22, 3:54 AM
elexis updated the summary of D2210: Don't show target markers for observers pretending to send commands.
Thu, Aug 22, 3:37 AM
elexis created D2210: Don't show target markers for observers pretending to send commands.
Thu, Aug 22, 3:37 AM
elexis updated the Trac tickets for D2197: Add support for recording replay metadata when in nonvisual mode.
Thu, Aug 22, 12:15 AM

Wed, Aug 21

elexis added inline comments to D2209: Unify 19 copies of IGUIObject::PlaySound and stop copying the according CStrW.
Wed, Aug 21, 11:26 PM
elexis created D2209: Unify 19 copies of IGUIObject::PlaySound and stop copying the according CStrW.
Wed, Aug 21, 11:19 PM
elexis added a comment to D2195: Cleanup Camera and CGameView.
In D2195#91641, @elexis 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.

Wed, Aug 21, 11:05 PM
elexis awarded D2197: Add support for recording replay metadata when in nonvisual mode a Like token.
Wed, Aug 21, 10:06 PM
elexis added inline comments to D2127: ScriptInterface CreateObject performance improvement.
Wed, Aug 21, 9:54 PM
elexis added inline comments to rP22680: Improve performance of ScriptInterface::CreateObject from rP22528 / D2080 by….
Wed, Aug 21, 9:48 PM
elexis added inline comments to D2127: ScriptInterface CreateObject performance improvement.
Wed, Aug 21, 9:48 PM
elexis added a comment to D2128: Use CreateObject in EngineScriptConversions.

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.

Wed, Aug 21, 7:15 PM
elexis updated the test plan for D2127: ScriptInterface CreateObject performance improvement.
Wed, Aug 21, 4:06 PM
elexis added a comment to D2128: Use CreateObject in EngineScriptConversions.

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

Wed, Aug 21, 4:04 PM
elexis committed rP22744: Use variadic template for RecurseObject from rP31 and move it from GUIutil to….
Use variadic template for RecurseObject from rP31 and move it from GUIutil to…
Wed, Aug 21, 3:22 PM
elexis closed D2204: Use variadic template for RecurseObject and move it from GUIutil to IGUIObject.
Wed, Aug 21, 3:22 PM
elexis added inline comments to rP141: Major updates.
Wed, Aug 21, 3:20 PM
elexis added a comment to D2204: Use variadic template for RecurseObject and move it from GUIutil to IGUIObject.

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

Wed, Aug 21, 2:27 PM
elexis committed rP22741: Use CGUI& instead of CGUI* so that the 50+ users stop wondering whether or not….
Use CGUI& instead of CGUI* so that the 50+ users stop wondering whether or not…
Wed, Aug 21, 12:12 PM
elexis closed D2205: Use CGUI& instead of CGUI*.
Wed, Aug 21, 12:12 PM
elexis added a comment to D2205: Use CGUI& instead of CGUI*.

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.

Wed, Aug 21, 12:11 PM
elexis added a comment to D1989: Allow accessing prop points in the simulation..

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?

Wed, Aug 21, 11:39 AM
elexis added inline comments to rP22694: Stop copying color every draw call for every GUI object using colors..
Wed, Aug 21, 11:16 AM
elexis added a comment to D2207: Adds a function to pick entities with obstructions on screen.

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

Wed, Aug 21, 10:57 AM
elexis added a comment to D2196: Fixes values of clip planes in PostProcManager.

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

Wed, Aug 21, 12:32 AM
elexis added inline comments to D2206: Put CGUI BaseObject / CGUIDummyObject on the stack.
Wed, Aug 21, 12:13 AM

Tue, Aug 20

elexis added inline comments to D2206: Put CGUI BaseObject / CGUIDummyObject on the stack.
Tue, Aug 20, 11:55 PM
elexis created D2206: Put CGUI BaseObject / CGUIDummyObject on the stack.
Tue, Aug 20, 11:45 PM
elexis retitled D2142: Remove JSInterface_IGUIObject duplication from Remove JSInterface_IGUIObject duplication / remove JS_THIS_OBJECT to Remove JSInterface_IGUIObject duplication.
Tue, Aug 20, 11:38 PM
elexis added a comment to D2199: Add RPC interface for Reinforcement Learning.

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?

Tue, Aug 20, 11:20 PM
elexis added inline comments to D2204: Use variadic template for RecurseObject and move it from GUIutil to IGUIObject.
Tue, Aug 20, 11:15 PM
elexis added a comment to D2205: Use CGUI& instead of CGUI*.
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.

Tue, Aug 20, 11:13 PM
elexis added a comment to D2197: Add support for recording replay metadata when in nonvisual mode.

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

Tue, Aug 20, 10:55 PM
elexis added a comment to D2205: Use CGUI& instead of CGUI*.
In D2205#91647, @elexis wrote:

If you get a reference you don't even have the thought "Do I need to check for null", whereas it's the first thought if you receive a pointer.
Even more it makes it impossible to check if the reference is null, so you don't have half the code with null checks and the other code without and some other code with ENSURE.
IMO GetGUI isn't adding anything

It answers to the "Do I need to check for null" question as it returns a reference. I'm not talking about how it stores GUI inside.

Tue, Aug 20, 10:35 PM
elexis added a comment to D2205: Use CGUI& instead of CGUI*.

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.

Tue, Aug 20, 10:25 PM
elexis added a comment to D2205: Use CGUI& instead of CGUI*.
In D2205#91624, @elexis wrote:

I still didn't claim that this guarantees the lifetime of the CGUI object.
I did and do claim that the lifetime is guaranteed by the way the code is designed and written, namely that GUIObjects are created inside a CGUI and that the CGUI destroys its objects before it is destroyed and that CGUI objects are never moved or to be moved from one GUI to another.

It's not the objection to your patch, it's just a note that reference isn't safe as it may seem.

I know, it's not a shared_ptr.

Tue, Aug 20, 10:11 PM
elexis added a comment to D2205: Use CGUI& instead of CGUI*.

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?

What other code?

The code that you have posted??

Tue, Aug 20, 9:54 PM
elexis added a comment to D2195: Cleanup Camera and CGameView.

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

Tue, Aug 20, 9:38 PM
elexis added a comment to rP22638: Use NONCOPYABLE for most GUI classes and structs to have the compiler indicate….

We usually put NONCOPYABLE in a private section.

Tue, Aug 20, 9:31 PM
elexis added a comment to D2205: Use CGUI& instead of CGUI*.

Why you want to add 54 nullability checks is your problem

So you're making this a personal issue, okay, I'm always happy to go down that road.

Tue, Aug 20, 9:29 PM
elexis abandoned D2125: VS2013 Grid.h memset fallback.

Why not to use std::fill? Does it produce the warning?

Tue, Aug 20, 9:07 PM
elexis abandoned D1699: Remove obsolete JSI_GUIMouse, JSI_IGUIObject::construct, JS_THIS_OBJECT and cleanup JS GUI interface.

All I remember is that these should be JS_FN instead of JS_FS. Haven't looked at the rest.

Tue, Aug 20, 8:56 PM
elexis added a comment to D2205: Use CGUI& instead of CGUI*.

I'd suggest to replace all m_pGUI by GetGUI(), which can return whatever you want. Fo ex:

CGUI& GetGUI()
{
    ENSURE(m_pGUI);
    return *m_pGUI;
}
// or
CGUI& GetGUI()
{
    ENSURE(IsGUIStillValid(m_pGUI));
    return m_pGUI;
}
Tue, Aug 20, 8:54 PM
elexis added a comment to D2205: Use CGUI& instead of CGUI*.

As far as I understand the patch the changes only shows to readers that the m_pGUI member can be used without the safeguard.

Tue, Aug 20, 8:49 PM
elexis added inline comments to D2195: Cleanup Camera and CGameView.
Tue, Aug 20, 8:21 PM
elexis added a comment to D2205: Use CGUI& instead of CGUI*.

I've read your summary and I understand why you want to change it so.

Evidently you dont understand why I want to change it if you say that it doesn't change anything and even more so if you say that it introduces something that it is already the case.

Tue, Aug 20, 8:14 PM
elexis added inline comments to D2195: Cleanup Camera and CGameView.
Tue, Aug 20, 7:55 PM
elexis added inline comments to D2195: Cleanup Camera and CGameView.
Tue, Aug 20, 7:48 PM
elexis added a comment to D2205: Use CGUI& instead of CGUI*.
In D2205#91525, @elexis wrote:

From my experience such kind of changes doesn't really improve a code and usually adds some difficulty.

Using a reference removes the difficulty here.

I didn't find storying of reference in gui objects before.

The point is, if you operate a CGUI* then you are not sure if it's allocated or not. But if it always is, then adding safeguards is only adding more code and saving from something that never happens.
The reference expresses that the CGUI is always allocated during its lifetime, thus it is making it even impossible to test if (m_pGUI) or if (pGUI).
The m_pGUI pointer exists since rP74.
The commit mentioned in the summary changed it so that CGUI is allocated on init rather than some cycles later (which is even making it faster, since the value is directly assigned to the correct pointer instead of to nullpointer and then to something else), aside from making it easier for all the users to consider it.
If you look at that commit referenced in the summary, you will see the previous cases in which it was set.
SetGUI was called on AddScrollBar, on CGUIDummyObject creation, for every child object created in AddObject, and prior to RegisterScriptHandler.

Tue, Aug 20, 7:47 PM
elexis updated the summary of D2198: Fixes bug with disabled fog on water.
Tue, Aug 20, 6:26 PM
elexis added inline comments to D2195: Cleanup Camera and CGameView.
Tue, Aug 20, 6:25 PM
elexis added a comment to D2205: Use CGUI& instead of CGUI*.

we do not need to check whether the CGUI page exists or not.

Reference doesn't guarantee that.

Nor did I claim that its the reference guaranteeting that.

Tue, Aug 20, 5:46 PM
elexis added a comment to rP22723: Aspis Remake..

Why it would mark this as bad while others cavalry_javelinist_x_m use the same variant textures? in any case i copied from cavalry_javelinist_a to _b and see if it fixes it.

Check the attached logfile, it's marked for many files

Tue, Aug 20, 5:28 PM
elexis created D2205: Use CGUI& instead of CGUI*.
Tue, Aug 20, 5:10 PM
elexis added a comment to rP22723: Aspis Remake..

Also unrelated to this commit, but the XML validation script spams 1 error and 1 warning for many files, in case someone reading this wants to do a mass change:

Tue, Aug 20, 4:55 PM
elexis added a comment to rP22723: Aspis Remake..

Jenkins says:

mismatched tag at line 11, column 6, byte 498 at /usr/lib/x86_64-linux-gnu/perl5/5.24/XML/Parser.pm line 187.

Tue, Aug 20, 4:51 PM
elexis created D2204: Use variadic template for RecurseObject and move it from GUIutil to IGUIObject.
Tue, Aug 20, 3:55 PM
elexis added inline comments to D2196: Fixes values of clip planes in PostProcManager.
Tue, Aug 20, 3:32 PM
elexis added a comment to D2196: Fixes values of clip planes in PostProcManager.

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

I can't say that it's a bug fix or a cleanup.

Tue, Aug 20, 2:26 PM
elexis added inline comments to D2196: Fixes values of clip planes in PostProcManager.
Tue, Aug 20, 2:13 PM
elexis committed rP22720: Erase unused variable following rP22695, clean two stray includes from rP1507..
Erase unused variable following rP22695, clean two stray includes from rP1507.
Tue, Aug 20, 12:51 PM
elexis closed D2203: Remove unused argument following rP22695 and cleanup two stray includes.
Tue, Aug 20, 12:51 PM