Page MenuHomeWildfire Games

Graphics Improvements
Needs RevisionPublic

Authored by aeonios on May 11 2018, 8:27 PM.



This revision contains two major changes:

  1. Postproc improvements

Depth of field and bloom/hdr have been merged into a single shader called dof_bloom_color. Since the two techniques share data dependencies it was more efficient to combine them into a single shader and use defines to enable or disable one or the other independently. This also allows both techniques to be applied at the same time for a negligible increase in cost, which was previously not possible. The appearance of DOF has also been dramatically improved so that it is actually something people might want to use.

Additionally the blur shader has been modified to use a smaller and nicer looking gaussian blur instead of a box blur.

DOF has been changed from being a map option to being a user option. There are at least three reasons why I believe this is a better choice:

A. There are exactly zero maps using DOF currently, and the auto-map generator does not use it either.

B. It is not an artistically relevant effect. It is not tunable and whether it is enabled or not has no impact on the way maps look in artistic terms. It looks very consistent in terms of producing the impression of distance between different maps, based on the general scaling factor of 0ad. Conversely it would make the game look inconsistent if some maps used it and others didn't, which is undesirable.

C. It's generally a user preference whether they like the way it looks or not.

  1. Shadow map improvements

The regular box filter for PCF filtering has been replaced with a poisson disk, which looks better for small blurs than the regular box blur that was previously used. I also modified it to maintain the same blur scaling regardless of the shadow map size.

Additionally I implemented percentage-closer-soft-shadows and added a user option for enabling it. For PCSS I ultimately decided to use a regular grid for blur sampling instead of a poisson disk, because it's much cheaper on a per-sample basis and allows for much smoother blurs with fewer banding artifacts. It is however disabled for objects drawn with USE_TRANSPARENT due to the large number of fragments which such objects can produce, which leads to unplayably low framerates that rapidly approach 0 fps. It falls back to regular PCF filtering for such objects.

I removed the ultra-low and ultra-high shadow map resolution settings. Ultra-low was so bad that it wasn't worth using, and ultra-high seems to fail to allocate the textures for reasons that I was unable to determine, even after doing a thorough investigation. I know that the spring engine using SDL has been able to use 8192x shadow maps with no problem, so I really don't understand why it isn't working properly for us. If the shadow map should still fail to allocate for whatever reason, it will now properly have the renderer reload the relevant shaders to disable shadow rendering entirely. I've also fixed some issues with shadows failing to draw properly after the shadow map resolution was changed in game.

Finally I changed it to use 16 bit depth for the shadow map by default. As far as I can tell this doesn't cause any real issues with shadow quality, but on non-ancient graphics cards reducing the bit depth reduces the bandwidth, which improves performance considerably for PCSS and PCF. The bit depth can still be set manually in local.cfg for old cards which may have worse performance when using 16bit rather than 24bit depth.

Misc Changes:

  • The "draw unit silhouettes" option was moved from graphics to general. As a general rather than graphics-specific feature it really belongs there anyway, but mainly it needed to be moved out because the graphics tab was overcrowded and the options were literally spilling out over the buttons at the bottom of the window.
  • The "prefer GLSL" option has been removed (also because of crowding issues). Instead, I set default.cfg to use shaders and preferglsl= true by default. This will soon be the only option anyway, and is the de-facto condition for >95% of our users, so it seemed like the most obvious choice of options to drop.
Test Plan

Make sure it compiles and that the options do what you would expect them to when enabled, and that they are saved properly and persist between games.

Diff Detail

rP 0 A.D. Public Repository
Lint Skipped
Unit Tests Skipped
Build Status
Buildable 6075
Build 10118: Vulcan BuildJenkins

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
wraitii added inline comments.May 14 2018, 11:05 AM

Next line for both


Next line


What's the reason for reordering here (and below)?


wrong tabs or this is inconsistent with above - honestly not sure :p

This revision now requires changes to proceed.May 14 2018, 11:05 AM
Stan added inline comments.May 14 2018, 11:06 AM

It was because people keep reporting crashes about it.

vladislavbelov requested changes to this revision.May 14 2018, 2:21 PM

Not all my questions were fixed or answered.

Also, please make diffs with contexts, like described here: Because currently the diff viewer shows the Context not available. message.

aeonios updated this revision to Diff 6542.May 14 2018, 7:01 PM
aeonios marked 11 inline comments as done.

wraitii fixes.

Not all my questions were fixed or answered.

Like what specifically?

Also, please make diffs with contexts, like described here: Because currently the diff viewer shows the Context not available. message.

TortoiseUDiff does not seem to have any such option. I don't know what to do about it either.


I did this because I removed the prefer-glsl option from the in-game menu for space reasons. Setting the render path to shader probably isn't necessary though. I can revert that bit.


Wait, so it was changed how? And to prevent what crashes?


I dunno, it's possible to add the ability to tune it, but it's not intuitive and easy to screw it up. Also it'd take up at least two more slots in the options which are not actually available.

The xml file only defines the widget styles and things, and doesn't actually define any of the options themselves. It's not related nor necessary to change it.


I'm still considering this. It's entirely doable but I'm not sure if I want to expose those kinds of obscure nuts-and-bolts details to the users.


Technically they're arbitrary, but it ensures that the blur scale increases/decreases with the texture size, and any number that I plug in here will be equally arbitrary. Those just happen to be the numbers I used to tune it. If there was a way to do it without magic numbers then I would have done that instead.


This is only the case when the sun is directly overhead. I could possibly use dot(-sundir, vec3(0.0, 1.0, 0.0)) to reduce the blur when the sun is nearly directly overhead. That's possibly worth experimenting with.


This was a pain but I fixed it. Now it uses the shadow frustum's bounding box to determine proper scaling so flickering artifacts should be greatly reduced and it should no longer overblur things when the sun is nearly overhead. I also reduced the blur scaling a bit so that it's less exaggerated. Now that it's working properly there was no need for such an excessive blur anyway.


I looked into this, and AFAIK it isn't. OpenGL doesn't let you use both linear sampling and nearest sampling for the same texture, and it looks a lot worse without linear sampling. Otherwise it probably would be possible to bind the one texture to two uniform points, as long as the texture was bound to two different texture units. DirectX does let you use arbitrary sampling methods for the same texture by defining them in the shader, for DX10 at least, but openGL is very inflexible about it across all versions.


Floats aren't that arbitrary, and this works.


d'oh. That was leftovers from a different experiment I was doing.

EDIT: correction, it's everything else that's using the wrong tabs. :(


fair enough.


it's necessary because of the way it's read from maps may produce inconsistent capitalization, or cause it to try to read something that doesn't exist. There's a mess of hardcoding no matter what. The only way to avoid it would be to dump full responsibility for postproc onto javascript and export all the openGL functions to allow it to call the shaders, then the mods could do whatever they wanted with it. The way atlas handles it is suboptimal as well, but there isn't really a clean way around it. This merely ensures that nothing breaks regardless of what kind of garbage input it receives.


No, this is only called on initialization and when the user changes options, or in the case of atlas when the map options change. Doing it this way just removes redundant code from the if-then-else block and makes it cleaner and easier to separate dof from hdr when compiling the shader. Performance wise nothing changes.


Ok I see how that might be confusing.


Fair enough.


Improving early-z rejection. Doing so reduces the number of fragments generated and thus the number of wasted shadow samples.


Ah crap, this was leftovers from some other experiments I was doing. Needs to be reverted.


Actually no, because opaque models will add to the depth buffer, which will later trigger early z rejection for the terrain drawn under it, reducing fragment load and improving performance. Rendering terrain before the skybox is a similar optimization. Sadly very few models actually use full opacity rendering, afaik only gaia critters do pretty much. I did this to try to reduce unnecessary shadow samples, which is important for making PCSS usable.


Fair enough.


Oh, it used to defer recreating the shadow textures/framebuffers until the following frame when options changed, but that caused it to do all kinds of screwy things and basically fail to render at all. I made it recreate the textures immediately which fixed the issue, but forgot to delete the commented out bit.


Well, I'll reimplement that for now, but the real problem for low-angle shots is that the shadow map pixels all end up on faraway objects which makes shadows close to the camera look like crap. The only way to really fix it is to use lightspace perspective shadow mapping, which I haven't implemented yet and which I'm not totally confident that I can do. It's honestly not that complicated but it requires some matrix math that I'm not sure entirely how it works or how to do it with the libraries that we're using.


I think you're tripping.


There was a comment that asked if it was necessary, and it wasn't so I removed it. When you bind a framebuffer with GL_FRAMEBUFFER_EXT it binds to both read and draw. Note that when I copy the shadow map for PCSS I also use bind framebuffer but instead of GL_FRAMEBUFFER_EXT I use:

pglBindFramebufferEXT(GL_READ_FRAMEBUFFER, m->Framebuffer);
pglBindFramebufferEXT(GL_DRAW_FRAMEBUFFER, m->Copybuffer);

instead. You only need to bind read/draw if you're actually going to read/draw something, which at that point in the code we're only constructing the buffers but not actually using them, so we don't need to.

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

Link to build:

fabio added a subscriber: fabio.May 16 2018, 11:11 AM

You should also have a look at this file, where some options are dynamically enabled based on detected hardware:


D86 also has some cleanups for hwdetect.js that are related and worth checking.

elexis added inline comments.May 16 2018, 11:49 AM

I guess aeonios already uses svn and notice that the current code says preferglsl = false and with svn blame we see the last changes from rP15077 / rP15075 / rP15074.
While we had GLSL crashes fixed in a23 (see release blockers on a23 milestone on nothing changed the default value since then.
And disabling it by default if some people experience a crash is not really an improvement because it still crashes if people tick the checkbox without being informed that its GLSL.
Disabling GLSL if a certain hardware + driver combination was identified as fabio mentioned is possible and good practice.

Are there actually any unfixed crashes reported? (I guess it's hard to decide with so many incomprehensible reports). I suspect we won't receive too many bugreports about people experiencing issues if we enable it by default. (Also getting more valid bugreports can be considered benefitial too and might allow extension of hwdetect.js.
(But I agree with Vladislav that its more auditable to split changes of existing defaults in a separate patch.)


Don't see the need to change the scale, this reduces the freedom to chose.
Also the string change seems bad, we made sure that the previous string was as clear as possible in stating when it may not work.

I'd rather recommend adding an "This can crash with low-end graphics card. Are you sure?" message box if the tooltip is considered insufficient.


According to rP20738, macros in shaders are not indented.

Whatever you're going to do with whitespace, do not change it in a patch that introduces a feature and if still wanted, upload a patch that only changes whitespace separately.


Indeed the convention is to not indent macros, so keep as is.

Stan added a comment.May 16 2018, 11:57 AM

I don't think the GLSL crash was fixed. Only the alt+tab one was.

wraitii added inline comments.May 16 2018, 12:34 PM

Ok, so no changes here and I'll open a diff to re-enable it.

Ah, #4734. 6 different callstacks where at least some of them are only triggered if GLSL is enabled? Then maybe, dunno. Need better reports and investigation.

Stan added a comment.May 16 2018, 1:28 PM

Need someone that can reproduce it mostly.

fabio added inline comments.May 16 2018, 2:22 PM

preferglsl = true doens't work with OpenGL 1 GPU, so it is wrong changing it here, which applies everywhere.

If you look at GLSL it is already enabled when OpenGL >= 3.

If you may want to enable it also for OpenGL >= 2 it should still be done in hwdetect.js.

wraitii added inline comments.May 16 2018, 2:24 PM

Ah, right, that's probably what I had in mind then. No action needed. Thanks for the input.

aeonios updated this revision to Diff 6561.May 17 2018, 11:12 PM
aeonios marked 27 inline comments as done.
  • Removed all indentation for macros.
  • Reverted default.cfg to preferglsl=false
  • Reduced blur scaling for PCSS from 1.25 to 1.0
  • Removed all warnings from shadow quality options
  • Implemented a proper workaround in shadowmanager so that if the shadow texture fails to allocate it will iteratively reduce the quality until it either succeeds or runs out of room for reduction, in which case it will disable shadows and the shadows option in user cfg. This behavior should be safe even if the shadow map size is defined in local.cfg.

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

Link to build:

Stan added a comment.Jun 4 2018, 11:16 AM

Some comments to keep you waiting :P


Comments seem to be above in this part of the file.


"Show units' outlines behind buildings." Maybe


Probably not, unless we want non obvious bug reports.


"Depth of field"


Maybe slow instead of expensive


Not sure for those types of files, but we use indent with two spaces everywhere for XML files.


Add a newline. Apparently it's pretty bad in Unix text editors.


Any way to reduce the number of computations by reducing the number of time each value is used ?


Nuke that last line


Nuke the empty lines


Still this is bad practise, and the reason we have fixed in our code. Also, it might only work on your platform and bug randomly on another :)


Wrong indent. Also we should use brackets on a new line everywhere, It seems you didn't do it anywhere in the above files.


Comments start with caps


Comments start with caps :)


Same here


There too


GPU_Skinning is an experimental feature not present in the game options. I don't know how good the implementation is, might want to look at it if you have some time :)


Same thing about the float, if you decide to check for close equality :)


Is there some kind of strcmp that can be case insensitive ?

Also doesn't it feel weird that default is lowercase None as a cap and DOF is full caps?


Any reason this was moved ?




@wraitii @vladislavbelov In the coding conventions should switch case with multiple statements have brackets ? We seem to do that consistently on message handling code.


Any reason for this ? :)


Any reason for this ? :)


I would have put a static_cast<float> but I don't think it's even needed. I'll let @wraitii and @vladislavbelov argue about it :P


Might want to use newlines.
Also is there a constant somewhere for those strings ?




This looks a lot like the code in rendermodifiers, can it be a common function

Stan requested changes to this revision.Jan 23 2019, 11:29 AM

First of all let me apologize for the amount of time this has been put back.

I'm requesting changes to this revision.

  • All the copyright years need to be bumped in the cpp file.
  • @vladislavbelov said he won't review the patch in the current state because it's too big. I know it sucks but can you:
    • Split the cleanup patch into multiple easy to review sub patches ?
    • Remove the unneeded whitespaces changes likely due to your editor
    • Move every little bug fix into a separate revision
    • You can set revision dependency to make all your patches linked together.

Add a final new line. *Unix users raging in the back*


Whitespace, nothing was touched below.


Sounds like a good candidate for a separate patch.

if (m_Options.m_ShadowPCF)
    m->globalContext.Add(!m_Options.m_ShadowPCSS ? str_USE_SHADOW_PCF : str_USE_SHADOW_PCSS, str_1);
This revision now requires changes to proceed.Jan 23 2019, 11:29 AM

@vladislavbelov would you commandeer it in near future or is it obsolete by now?