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

It was because people keep reporting crashes about it.

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.

You should have a look at this file:
https://trac.wildfiregames.com/browser/ps/trunk/binaries/data/mods/mod/hwdetect/hwdetect.js

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 http://trac.wildfiregames.com 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.

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

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

If you look at https://trac.wildfiregames.com/browser/ps/trunk/binaries/data/mods/mod/hwdetect/hwdetect.js 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.

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

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 ?

Caps

@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 ?

Caps

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