Paths

Table of Contentst

Fix gametime overlay and watermark in summary screen
ClosedPublic
Actions

Authored by Imarok on Feb 17 2017, 4:05 PM.

Details

Reviewers

wraitii
elexis

Commits

rP23996: Fix gametime overlay and watermark in summary screen

Summary

The gametime overlay is not drawn correctly in summary:

And pressing the screenshot.watermark hotkey in the summary does not show the watermark, but throws some warnings.
To fix this we need to include the needed sprites in page_summary.xml and move <include>common/global.xml</include> to the end of the include list. (Then it will be drawn last)

This change has a long history:
rP8234 introduced the summary screen: the sprites were included and the include for global.xml was at the end
rP16384 removed the sprites
rP16385 included the sprite by reverting rP16384
rP16391 removed sprites and moved include for global.xml at the wrong place
rP16406 corrected it
rP16501 removed the sprites and moved include for global.xml at the wrong place

The real underlying issue is a bug in the renderer as vladislav and wraitii noticed. The corresponding ticket is: #4719

Test Plan

Open the summary screen ingame and look at the gametime overlay.
Open the summary screen and press Alt + K to show the watermark.

Diff Detail

Repository

rP 0 A.D. Public Repository

Branch

http://svn.wildfiregames.com/public/ps/trunk

Lint

Lint OK

Unit

No Unit Test Coverage

Build Status

Buildable 558
Build 885: Vulcan Build	Jenkins
Build 884: arc lint + arc unit

Event Timeline

Imarok created this revision.Feb 17 2017, 4:05 PM

Imarok edited the summary of this revision. (Show Details)Feb 17 2017, 4:09 PM

Include order shouldn't determine draw order ideally, though keeping track of z values across global objects is a pita too

In D148#5191, @elexis wrote:

Include order shouldn't determine draw order ideally, though keeping track of z values across global objects is a pita too

Sure. It shouldn't, but it does ^^

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/366/ for more details.

Harbormaster completed remote builds in B537: Diff 542.Feb 17 2017, 4:49 PM

Imarok added a reviewer: • leper.Feb 20 2017, 3:52 PM

Checked all gui pages and where needed added the includes and the comment to prevent someone breaking it later

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/382/ for more details.

Harbormaster completed remote builds in B558: Diff 569.Feb 21 2017, 2:30 PM

Why not just specify a high z value for the object in global.xml?

In D148#5824, @leper wrote:

Why not just specify a high z value for the object in global.xml?

The z value is already high enough, but the transparency only works, if the object is drawn at last.

[...]the transparency only works, if the object is drawn at last.

That sounds awfully non-straight-forward. And those include order comments will be ignored once someone adds a new page (or more likely just copied over and then ignored). You should also check if there are occurences of this in the mod mod.

So why is the transparency bugged like that? (That said I don't really care about such gui issues, so I do strongly suggest that you find someone else to review this.)

That what leper said

This revision now requires changes to proceed.Feb 28 2017, 11:25 AM

There are no occurences in mod mod.

In D148#6630, @Imarok wrote:

There are no occurences in mod mod.

Mostly investigating why the engine is messed up instead of covering this up

In D148#6631, @elexis wrote:

In D148#6630, @Imarok wrote:

There are no occurences in mod mod.

Mostly investigating why the engine is messed up instead of covering this up

I think thats a general OpenGL bug.
(To fix that, we'd have to sort all objects by their z value and draw them in this order)

In D148#6632, @Imarok wrote:

(To fix that, we'd have to sort all objects by their z value and draw them in this order)

This is probably done in the simulation renderer, but not done in the GUI renderer

In D148#6632, @Imarok wrote:

I think thats a general OpenGL bug.
(To fix that, we'd have to sort all objects by their z value and draw them in this order)

I looks like more design failure, because we always should draw opaque data as is, but transparent in a sorted order.

In our case, i.e. function for sprites (background, images, etc): void GUIRenderer::Draw(DrawCalls& Calls, float Z). It doesn't sort by Z-value, just draw as is. So we should fix it.

The proper way to fix this would be to centralise rendering of the GUI sprites, instead of delegating it to each of them. Which would also allow optimising for texture and stuff.

In D148#12680, @wraitii wrote:

The proper way to fix this would be to centralise rendering of the GUI sprites, instead of delegating it to each of them. Which would also allow optimising for texture and stuff.

Yeah, we shouldn't draw an element when we only found it, we should collect them (like render submit), and draw after all data is collected. Also it allows us to render opaque/transparent elements per 1! draw call each group.

Since noone is willing to rewrite source/gui/, we can at least make this harmless workaround consistent, have the uglyness documented in the code and write a ticket that people stumble upon when searching for this comment on trac.

binaries/data/mods/public/gui/page_credits.xml
7	Don't include this uselessly.

In D148#31076, @elexis wrote:

Since noone is willing to rewrite source/gui/, we can at least make this harmless workaround consistent, have the uglyness documented in the code and write a ticket that people stumble upon when searching for this comment on trac.

I'm planning to change the GUI rendering, as I discussed it with wraitii.

I don't think that we need other hacks.

In D148#31078, @vladislavbelov wrote:

In D148#31076, @elexis wrote:

Since noone is willing to rewrite source/gui/....

I'm planning to change the GUI rendering, as I discussed it with wraitii.
I don't think that we need other hacks.

Sounds good. (In that case we can do the converse here and restore alphabetical order for that <include>common/global.xml</include> line.)

elexis mentioned this in rP18361: Implement buttons for up to 10 heroes. Patch by Clockwork-Muse, fixes #3000..Sep 5 2017, 11:12 AM

As there is no progress with the render thing, can we just add this as temporary workaround?

elexis resigned from this revision.Dec 12 2017, 8:18 PM

elexis mentioned this in rP22957: Remove GUI object setting values z="0", size="0 0 100% 100%", hidden="false" in….Sep 21 2019, 7:59 PM

This revision now requires review to proceed.Sep 21 2019, 7:59 PM

elexis mentioned this in rP23117: TimeNotificationOverlay and Ambient class, refs #5387..Oct 30 2019, 3:39 PM

elexis mentioned this in D2412: Lobby class implementation.Nov 20 2019, 11:24 AM

@vladislavbelov any news regarding that issue? ;)

Planning to at least investigate this, given that the gui rewrite isn't coming very soon indeed.

Tested.
So indeed the issue is that this sprite has a partially transparent background, and thus the Z-buffer is updated and we run into trouble.

Updating the GUI to fetch all sprites and render them in order seems complex and this only affects that for now.

I have one alternate proposition:

How about you make the FPS counter have a completely transparent background (doesn't matter too much if it's not seen)?
The in-game time counter could be made a "first-class citizen", since it's specific to in-game.

Then you wouldn't have to hack with the order of things.

In D148#125904, @wraitii wrote:

I have one alternate proposition:

How about you make the FPS counter have a completely transparent background (doesn't matter too much if it's not seen)?

The in-game time counter could be made a "first-class citizen", since it's specific to in-game.

Then you wouldn't have to hack with the order of things.

Hmm, not that happy about this solution.
With a fully transparent background the white font gets unreadable in some situations. (e.g. main menu)

In that case, I think this is the best we can do for now.

This revision is now accepted and ready to land.Aug 1 2020, 5:52 PM

Imarok edited the summary of this revision. (Show Details)Aug 1 2020, 6:20 PM

Imarok added inline comments.Aug 1 2020, 7:21 PM