Page MenuHomeWildfire Games

Stop copying color every draw call for every GUI object using colors ffs
AbandonedPublic

Authored by elexis on Aug 19 2019, 2:22 PM.

Details

Summary

Importantly avoid color copies for rendering Draw calls in GUIRenderer::UpdateDrawCallCache, CButton::Draw, CChart::DrawAxes, CDropDown::Draw, COList::DrawList, refs #1984, rP1518, rP22637, rP22638.

Also avoid color copies during XML loading in CGUI::Xeromyces_ReadImage, CGUI::Xeromyces_ReadEffects, COList::HandleAdditionalChildren.
Add CGUI::HasPreDefinedColor and mark m_PreDefinedColors, CGUI::GetPreDefinedColor, IGUIButtonBehavior::ChooseColor() as const for consistency with the other "databases", refs rP22637.
Mark CGUIColor as NONCOPYABLE to add compiler errors if there is an unexplicit copy, refs rP22637.
Explicit and ugly copy in CGUI::Xeromyces_ReadColor and CGUIColor::ParseString until refactoring can remove these.
Deregister copying <CGUIColor>GetSetting functions, refs rP1518.
Uses the const ref GetSetting from rP22693.

Test Plan

The two remaining copies are ugly. Before one rushes to the argument that not adding the NONCOPYABLE and just copying beautifully is preferable, having this CGUIColor noncopyable also means that accidental copies are avoided in the future (also I seem to remember that some future patch wanted that to be noncopyable). The noncopyable property (just like the class) can be removed later if refactoring allows for that and if there are no further reasons to have it that way.
Notice that CColor is a struct having four floats, so a reference will be faster. (Could have been four floats and a pointer to CGUI, but I dodged that one).

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8924
Build 14645: Vulcan BuildJenkins
Build 14644: arc lint + arc unit

Event Timeline

elexis created this revision.Aug 19 2019, 2:22 PM
Stan awarded a token.Aug 19 2019, 2:36 PM

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

Linter detected issues:
Executing section Source...

source/gui/CGUI.h
|  27| #include·"GUIbase.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classGUITooltip{' is invalid C code. Use --std or --language to configure the language.

source/gui/GUIRenderer.h
|  33| namespace·GUIRenderer
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceGUIRenderer{' is invalid C code. Use --std or --language to configure the language.

source/graphics/Color.h
|  75| »   »   »   static_cast<u8>(r·*·255.f),
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'C++ cast <...' is invalid C code. Use --std or --language to configure the language.

source/gui/IGUIButtonBehavior.h
|  49| class·IGUIButtonBehavior·:·virtual·public·IGUIObject
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classIGUIButtonBehavior:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/427/display/redirect

Stan added a subscriber: Stan.Aug 19 2019, 2:38 PM

Any performance boost from this ?

Yes, but probably not that much. I didn't see an FPS difference that exceeds the measurement error and I didn't write benchmark code for this.

In D2193#91263, @elexis wrote:

Yes, but probably not that much. I didn't see an FPS difference that exceeds the measurement error and I didn't write benchmark code for this.

congrats

Stan added a comment.EditedAug 19 2019, 3:08 PM
In D2193#91263, @elexis wrote:

Yes, but probably not that much. I didn't see an FPS difference that exceeds the measurement error and I didn't write benchmark code for this.

Well too bad.

Forgot to close it ? (Missing the differential revision) in rP22694

Well thats unfortunate, I'll ask Itms, or whomever @phabadmin is on occasion.

Stan added a comment.Aug 19 2019, 3:37 PM

Since its your revision you can close it

It can only be closed if it is accepted. And in case you want to accept something, remember territory pull.

Stan added a comment.Aug 19 2019, 4:02 PM
In D2193#91271, @elexis wrote:

It can only be closed if it is accepted. And in case you want to accept something, remember territory pull.

Right.

Itms added a subscriber: Itms.Aug 19 2019, 4:45 PM
In D2193#91271, @elexis wrote:

It can only be closed if it is accepted.

No, the author is supposed to be able to close it. I am also unable to bypass this with the admin account. I have opened a bug report upstream: https://discourse.phabricator-community.org/t/cannot-manually-close-revisions/3035.

It might be just a web UI bug, in which case you can try arc close-revision.

elexis abandoned this revision.Aug 19 2019, 7:01 PM

Usage Exception: Revision D2193 can not be closed. You can only close revisions which have been 'accepted'.

Still enforced serverside (tested by removing the check in /usr/share/php/arcanist/src/workflow/ArcanistCloseRevisionWorkflow.php)

ERR-CONDUIT-CORE: Validation errors:
You can not close this revision because it has not been accepted. Revisions must be accepted before they can be closed.

I'll take this out of the review queue the hard way until then - committed in rP22694.