Page MenuHomeWildfire Games

Fix gametime overlay and watermark in summary screen
Needs RevisionPublic

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

Details

Reviewers
elexis
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

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 BuildJenkins
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
elexis added a subscriber: elexis.Feb 17 2017, 4:12 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 ^^

Vulcan added a subscriber: Vulcan.Feb 17 2017, 4:49 PM

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.

Imarok updated this revision to Diff 569.Feb 21 2017, 1:46 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.

leper edited edge metadata.Feb 21 2017, 7:41 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.

leper resigned from this revision.Feb 23 2017, 1:29 AM

[...]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.)

elexis requested changes to this revision.Feb 28 2017, 11:25 AM

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)

Imarok added a comment.Mar 3 2017, 8:15 PM
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.

vladislavbelov added a comment.EditedApr 13 2017, 5:35 PM
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#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.)

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