Page MenuHomeWildfire Games

Pass CGUI in GUIObject constructor, remove GetPreDefinedColor top page workaround

Authored by elexis on Sun, Jul 21, 5:08 PM.


Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
rP22663: Delete wrongful proxy CGUIManager::GetPreDefinedColor from rP7214.

While working with the CGUIManager::PushPage function in D1684, it was proposed to move code into the SGUIPage constructor.
Trying to move the LoadPage call into the ctor revealed that GetPreDefinedColor access the predefined colors (such as black or white) from the topmost page, rather than the current page.
This is a defect, because the different pages might want to load different or less files.
(The Load call still can't be moved to the ctor as it messes up the stack order, for example for the lobby login page, but that's offtopic).

There are more occurrences where top() is called instead of all GUI pages or the current GUI page, so there will be more refactoring to come.

That GUIObjects can't be constructed without knowing about the UI is already manifest in the header:

	// An object can't function stand alone
	CGUI									*m_pGUI;

and there are some tangential calls removed where SetGUI was called to cover up this defect (late init instead of ctor init).

Test Plan

Is it possible to use a constant pointer? It seemed like it was not possible due to the UpdateDrawCallCache modifying Sprites argument passed from CGUISpriteInstance::Draw passed from CGUI::DrawSprite which is a CGUI member m_Sprites.
Is a shared, weak or unique pointer advisable?
Perhaps even a reference?
Is there some function complaining about the logic change?

Diff Detail

rP 0 A.D. Public Repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Sun, Jul 21, 5:08 PM

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

Link to build:

elexis added inline comments.Sun, Jul 21, 7:02 PM
51 ↗(On Diff #9037)

this will call SetupText() without generated texts

60 ↗(On Diff #9037)

this fills in m_GeneratedTexts

72 ↗(On Diff #9037)

This case is triggered once upon init. Previously it called SetSetting and then SetGUIObject, so the early return was triggered here before as well.

Stacktrace if the ENSURE was not replaced and triggered:

#0  0x00007ffff62d0636 in waitpid () from /usr/lib/
#1  0x0000555555b52ba6 in try_gui_display_error (no_continue=false, allow_suppress=true, manual_break=true, text=0x1628 <error: Cannot access memory at address 0x1628>) at ../../../source/lib/sysdep/os/unix/unix.cpp:165
#2  sys_display_error (
    text=text@entry=0x7fffa5723000 L"Assertion failed: \"m_GeneratedTexts.size()>=1\"\r\nLocation: CText.cpp:73 (SetupText)\r\n\r\nCall stack:\r\n\r\n(0x555555b4f215) /path/to/0ad/trunk/binaries/system/pyrogenesis(+0x5fb215) [0x555555b4"..., flags=flags@entry=6) at ../../../source/lib/sysdep/os/unix/unix.cpp:215
#3  0x0000555555aef9db in CallDisplayError (flags=<optimized out>, 
    text=0x7fffa5723000 L"Assertion failed: \"m_GeneratedTexts.size()>=1\"\r\nLocation: CText.cpp:73 (SetupText)\r\n\r\nCall stack:\r\n\r\n(0x555555b4f215) /path/to/0ad/trunk/binaries/system/pyrogenesis(+0x5fb215) [0x555555b4"...) at ../../../source/lib/debug.cpp:383
#4  debug_DisplayError (description=<optimized out>, flags=<optimized out>, context=0x7fffffffb340, lastFuncToSkip=0x555556cd83e0 L"debug_OnAssertionFailure", pathname=<optimized out>, line=73, func=<optimized out>, 
    suppress=0x555555d50eb8 <CText::SetupText()::suppress__>) at ../../../source/lib/debug.cpp:474
#5  0x0000555555aefe97 in debug_OnAssertionFailure (expr=expr@entry=0x555555c4eb40 L"m_GeneratedTexts.size()>=1", suppress=suppress@entry=0x555555d50eb8 <CText::SetupText()::suppress__>, 
    file=file@entry=0x555555c4eac8 L"../../../source/gui/CText.cpp", line=line@entry=73, func=func@entry=0x555555c4eabe "SetupText") at /usr/include/c++/9.1.0/bits/basic_string.h:2300
#6  0x0000555555a91156 in CText::SetupText (this=0x555556d22b60) at ../../../source/gui/CText.cpp:73
#7  0x0000555555a8ee31 in CText::HandleMessage (this=0x555556d22b60, Message=...) at ../../../source/gui/CText.cpp:138
#8  0x0000555555ab7650 in CInternalCGUIAccessorBase::HandleMessage (message=..., pObject=0x555556d22bb0) at /usr/include/c++/9.1.0/bits/allocator.h:153
#9  GUI<bool>::SetSetting (pObject=pObject@entry=0x555556d22bb0, Setting=..., Value=Value@entry=@0x7fffffffbf17: false, SkipMessage=@0x7fffffffbf18: false) at ../../../source/gui/GUIutil.cpp:423
#10 0x0000555555a91ad4 in CText::CText (this=0x555556d22b60, guiPage=<optimized out>, __in_chrg=<optimized out>, __vtt_parm=<optimized out>) at /usr/include/c++/9.1.0/bits/allocator.h:153
#11 0x0000555555a69c76 in CText::ConstructObject (guiPage=0x555555e47310) at ../../../source/gui/CText.h:30
#12 0x0000555555a6380b in CGUI::Xeromyces_ReadObject (this=0x555555e47310, Element=..., pFile=0x7fffffffc900, pParent=0x555556d2a240, NameSubst=std::vector of length 0, capacity 1, Paths=..., nesting_depth=0)
    at ../../../source/gui/CGUI.cpp:985
#13 0x0000555555a643aa in CGUI::Xeromyces_ReadObject (this=0x555555e47310, Element=..., pFile=0x7fffffffc900, pParent=0x555556ad8ff0, NameSubst=std::vector of length 0, capacity 1, Paths=..., nesting_depth=0)
    at ../../../source/gui/CGUI.cpp:1096
#14 0x0000555555a65fcc in CGUI::Xeromyces_ReadRootObjects (this=0x555555e47310, Element=..., pFile=0x7fffffffc900, Paths=...) at ../../../source/gui/CGUI.cpp:937
#15 0x0000555555a6936d in CGUI::LoadXmlFile (this=0x555555e47310, Filename=..., Paths=...) at ../../../source/gui/CGUI.cpp:897
#16 0x0000555555a99d01 in CGUIManager::SGUIPage::LoadPage (this=0x7fffffffcf70, scriptRuntime=...) at /usr/include/c++/9.1.0/bits/shared_ptr_base.h:1020
#17 0x0000555555a9b96d in CGUIManager::SGUIPage::SGUIPage (this=0x7fffffffcf70, pageName=..., initData=std::shared_ptr<ScriptInterface::StructuredClone> (use count 4, weak count 0) = {...}, 
    scriptRuntime=std::shared_ptr<ScriptRuntime> (use count 6, weak count 0) = {...}) at /usr/include/c++/9.1.0/ext/atomicity.h:96
#18 0x0000555555a9bb70 in CGUIManager::PushPage (this=0x555556ab7910, pageName=..., initData=...) at /usr/include/c++/9.1.0/ext/atomicity.h:96
#19 0x0000555555a9c164 in CGUIManager::SwitchPage (this=0x555556ab7910, pageName=..., srcScriptInterface=srcScriptInterface@entry=0x555555db5730, initData=..., initData@entry=...) at /usr/include/c++/9.1.0/ext/atomicity.h:96
#20 0x00005555557db3aa in InitPs (setup_gui=<optimized out>, gui_page=..., srcScriptInterface=0x555555db5730, initData=...) at ../../../source/ps/GameSetup/GameSetup.cpp:516
#21 0x00005555557e2cee in InitGraphics (args=..., flags=<optimized out>, installedMods=...) at /usr/include/c++/9.1.0/bits/allocator.h:153
#22 0x00005555555fa3d3 in RunGameOrAtlas (argc=<optimized out>, argv=<optimized out>) at ../../../source/main.cpp:626
#23 0x00005555555eb06a in main (argc=1, argv=0x7fffffffe908) at ../../../source/main.cpp:675
wraitii added a reviewer: Restricted Owners Package.Thu, Aug 1, 1:39 PM
elexis updated this revision to Diff 9186.Thu, Aug 1, 5:44 PM

Rebase following rP22558.
Add the CGUI* to the CGUIColor ctor. Perhaps rP22558 can be reverted with a similar approach to the ConstructSettingType construct here (replacing FromJSVal to JSVal). But that would be slightly more error prone, since every constructor of the CColor would have to make sure not to forget the PredefinedColor parse code.
Use GetSettingPointer instead of GetSetting for colors to avoid creating temporaries.
Wanted to look into changing the ConstructSettingType pointer to a reference / copy-elision return value.

Vulcan added a comment.Thu, Aug 1, 5:48 PM

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

Link to build:

elexis updated this revision to Diff 9187.Thu, Aug 1, 6:17 PM

Change ConstructSettingType to reference instead of pointer.

Vulcan added a comment.Thu, Aug 1, 6:19 PM

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

Link to build:

elexis updated this revision to Diff 9191.Thu, Aug 1, 11:07 PM

Committed CGUI* IGUIObject ctor in rP22587, unrelated LoadPage part in rP22588 (how did that get into this diff?); rebased.

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

Link to build:

elexis added inline comments.Tue, Aug 13, 6:53 PM
41 ↗(On Diff #9191)

I should have discovered this pattern before committing the CGUIColor type in rP22558.

The type is the same, but what makes it different is the way it is initialized (test against predefined color string database).

As long as no call in the GUI that creates a color doesnt forget to test for predefined colors, one can get away with using CColor.

A different benefit of this class however is that it can ensure that one will not forget parsing the predefined-color database when parsing a string:

By deleting the inherited method, we identify the users surely, for example:

../../../source/gui/CGUI.cpp:1763:13: error: attempt to use a deleted function
        if (!color.ParseString(value))
../../../source/gui/CGUIColor.h:30:7: note: 'ParseString' has been explicitly marked deleted here
        bool ParseString(const CStr& value, int defaultAlpha = 255) = delete;

Another advantage of keeping the class is that we can mark it as NONCOPYABLE and not implementing the move semantics, so that it totally is never copied around, not even under the move umbrella. There was only one call where copy was needed and that can be done by renaming the copy assignment to something that cant be triggered implicitly.

There may be further advantages that the CGUIColor class allows when one wants to do something different for GUI and "simulation" colors.

But I won't add the CGUI pointer to the CGUIColor type for now, since there are potentially many colors, and since the pointer may be passed around as a function argument.

This revision was not accepted when it landed; it landed in state Needs Review.Tue, Aug 13, 8:00 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.