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

Link to build: https://jenkins.wildfiregames.com/job/differential/350/display/redirect

Patch is working, thx. Maybe you want to display some information to the user that you have change her/his settings?

Is it correct that by chosing high settings without saving the error occurs? Shouldn't the settings be changed after saving? But I think this is another bug.

In D1437#58740, @OptimusShepard wrote:

Is it correct that by chosing high settings without saving the error occurs? Shouldn't the settings be changed after saving? But I think this is another bug.

No, it's not the bug. You can apply new settings without saving.

Notifications probably make sense, but we need to fix options events before.

I think this patch have to be in alpha 23. It doesn't solve the problem of #4883, but it saves from his consequences.

Isn't g_Renderer.m_Options.m_Shadows also needed here?

This is a bad idea. If the shadow map fails to allocate this will disable the shadows option, preventing the user from changing the shadow quality option which depends on it, thus basically preventing them from fixing the problem and forcing them to be stuck without shadows unless they manually dig into their config file to change the setting.

In D1437#60593, @aeonios wrote:

This is a bad idea. If the shadow map fails to allocate this will disable the shadows option, preventing the user from changing the shadow quality option which depends on it, thus basically preventing them from fixing the problem and forcing them to be stuck without shadows unless they manually dig into their config file to change the setting.

So we only need to switch quality to the lowest, and the problem has gone away.

Why it's needed? Because some players may don't understand, why no shadows, but the option is turned on.

In D1437#60583, @wraitii wrote:

Isn't g_Renderer.m_Options.m_Shadows also needed here?

Because it's already done inside the SetOptionBool.

In D1437#61071, @vladislavbelov wrote:

So we only need to switch quality to the lowest, and the problem has gone away.

That's not as easy as it sounds, because it'd be difficult to do without creating a recursion hell. I'll work on that I guess.

Ugh I found another problem with this, which is that shadow map size may be set in local.cfg, in which case the shadow map quality setting does nothing. I'm honestly not sure how to work around this.

In D1437#61080, @aeonios wrote:

That's not as easy as it sounds, because it'd be difficult to do without creating a recursion hell. I'll work on that I guess.

No, it won't. I suggest to disable shadows and set the quality to Low. We don't need to try allocate shadows, if we don't have space for it. It guarantees to work.

Ugh I found another problem with this, which is that shadow map size may be set in local.cfg, in which case the shadow map quality setting does nothing. I'm honestly not sure how to work around this.

It's not a problem, local.cfg is very rarely used to configure the game. The config is needed for custom values.

In D1437#60593, @aeonios wrote:

This is a bad idea. If the shadow map fails to allocate this will disable the shadows option, preventing the user from changing the shadow quality option which depends on it, thus basically preventing them from fixing the problem and forcing them to be stuck without shadows unless they manually dig into their config file to change the setting.

It's the problem only in the game, you can always enable/disable shadows in the main menu without allocations.

Can't you just lower the quality by one notch instead ?

In D1437#61154, @Stan wrote:

Can't you just lower the quality by one notch instead ?

I can, but it can fail on the next step too. But then user may don't understand, why he set the quality to Very High, but shadows seems on Low Quality. In this case we need to show a message to user.

So possible solutions:

1. Disable shadows and set the quality to the lowest possible.
2. Try to switch step-by-step from the highest to the lowest and stop when no error.
3. Open options and say to player something like: "no memory, please select other value".
4. Calculate free memory and needed memory sizes (it's not stable, i.e. it's not supported by Intel cards, for others only by extensions).

In D1437#59336, @OptimusShepard wrote:

I think this patch have to be in alpha 23. It doesn't solve the problem of #4883, but it saves from his consequences.

This is a reasonable argument.
But we still need to check that the patch doesn't introduce other unintended consequences (which can be hard to tell for all possible platforms before a release) and that the patch is the final way to address the problem (so that we dont have to start investigating this issue from scratch).

In D1437#61071, @vladislavbelov wrote:

In D1437#60593, @aeonios wrote:

this will disable the shadows option, preventing the user from changing the shadow quality option which depends on it, thus basically preventing them from fixing the problem and forcing them to be stuck without shadows unless they manually dig into their config file to change the setting.

So we only need to switch quality to the lowest, and the problem has gone away.
Why it's needed? Because some players may don't understand, why no shadows, but the option is turned on.

I think the problem is that this GL error is not reported to the user in a way that the average user comprehends.
Or rather the problem, nor the setting change are reported at all (barring the unreadable barely noticeable openGL mainlog error).

Actually it seems like we don't know that it's the shadow map quality setting that's the underlying cause of the shadow map not being allocatable, and not something unrelated, like polygon count or some shader effect, right?
An unrelated video effect could already be consuming 90% video memory and the shadow map might want to allocate 20% more.

So perhaps one can either trigger a custom JS exception in C++ and catch it in JS, or call a GUI event that informs the JS stack when a config setting changed.
This could be caught with a translated popup message that informs the user that his graphics card has too few memory to allocate the shadow map, thus the shadow map was disabled and that the user may reduce the quality of the shadow map or other graphics settings if he wants to use shadows.

I suppose the same OOM error can happen in other renderer places too that could equally be caught.

In D1437#61080, @aeonios wrote:

shadow map size may be set in local.cfg, in which case the shadow map quality setting does nothing

I didn't test, so I don't know if this patch handles the error gracefully with an affecting local.cfg.
If it doesn't, it would be safer to update the renderer to use shadow maps if and only if shadow maps can be used and are configured to be used; i.e. possibly don't change the configuration value, only a renderer value.

(Also, isn't disabling settings if theyre not supported by the hardware a use case of RunHardwareDetection? I guess we can't detect it as well there. (Perhaps it might be possible and make some sense to move the logic to disable the shadows to function in HWDetect.cpp and call that from ShadowMap.cpp... or not, dunno))

In D1437#66450, @elexis wrote:

I think the problem is that this GL error is not reported to the user in a way that the average user comprehends.
Or rather the problem, nor the setting change are reported at all (barring the unreadable barely noticeable openGL mainlog error).

I sort of fixed that in my unrelated shadow revision. Sort of. The error is still reported via the GL error text, but I made it more understandable and just had it recursively reduce the size until it allocs properly or fails completely. Also the settings GUI won't update until you close it and open it again even after the resolution has been reduced by the backend code if/when it fails. I don't think there's any way around that due to the way the settings panel works.

https://code.wildfiregames.com/D1486 (still needs minor tweaking to restore the 'Use GLSL' option for people who suffer crashes when it's enabled)

Actually it seems like we don't know that it's the shadow map quality setting that's the underlying cause of the shadow map not being allocatable, and not something unrelated, like polygon count or some shader effect, right?
An unrelated video effect could already be consuming 90% video memory and the shadow map might want to allocate 20% more.

I don't think that's how it's supposed to work. Allocating the shadow map is done in RAM and is only moved to the card when you actually draw the shadow map. OOM shouldn't even be possible due to virtual memory, so clearly SDL is doing something screwy. I seem to recall being able to use max sized shadow maps in the spring engine which also uses SDL, so either we're doing something wrong somewhere or else the SDL version we're using might have a bug. Alternatively it might be because we're limited to 32bits of address width.

I didn't test, so I don't know if this patch handles the error gracefully with an affecting local.cfg.

Well it's been forever since I've touched this due to the feature freeze going on for months, but I seem to remember testing it and it doesn't work. Or rather, the way it sets the shadow map size is different when using the cfg and may not support cleanly downsizing the shadow map if it fails to alloc the way you can with the in-game settings. A workaround for that should be possible but that's not something I paid much attention to.

In D1437#66465, @aeonios wrote:

In D1437#66450, @elexis wrote:

I think the problem is that this GL error is not reported to the user in a way that the average user comprehends.
Or rather the problem, nor the setting change are reported at all (barring the unreadable barely noticeable openGL mainlog error).

I sort of fixed that in my unrelated shadow revision. Sort of. The error is still reported via the GL error text, but I made it more understandable and just had it recursively reduce the size until it allocs properly or fails completely. Also the settings GUI won't update until you close it and open it again even after the resolution has been reduced by the backend code if/when it fails. I don't think there's any way around that due to the way the settings panel works.

I can close this ticket, if you'll solve the problem in your diff.

I don't think that's how it's supposed to work. Allocating the shadow map is done in RAM and is only moved to the card when you actually draw the shadow map. OOM shouldn't even be possible due to virtual memory.

It doesn't touch RAM in common case (when no mesa/software rendering/memory sharing) on a simple allocation, driver allocates the video memory directly. You can notice it when changing the option. Driver touches the RAM when the data param of glTexImage2d isn't nullptr. It'd be strange to allocate RAM in case of shadow maps (when data is nullptr). Notice, it's common, but it's not guaranteed. It's system/driver dependent.

so clearly SDL is doing something screwy.

SDL does nothing there. It only a helper for an OpenGL context creating/managing.

Well this patch is just disabling shadows if the user can't handle it because he doesn't have enough VRAM. Currently it only disables it but the user doesn't know about it.

Random thought:
How hard it would be to disable level of shadows and higher on which it errored and force one level lower?

In D1437#129985, @Angen wrote:

Random thought:
How hard it would be to disable level of shadows and higher on which it errored and force one level lower?

I already wrote a thing that does that as part of my giant graphics upgrade. I just haven't been able to work on that and got way too many complaints about every freaking thing.

In D1437#130020, @aeonios wrote:

I already wrote a thing that does that as part of my giant graphics upgrade. I just haven't been able to work on that and got way too many complaints about every freaking thing.

Unfortunately it's really hard to review big patches. Because the reviewing time grows exponentially with the size of patches. I'd suggest to split all big patches on as small as possible patches.

@aeonios,@Angen feel free to commandeer this patch as it's low priority on @vladislavbelov's todo list.

@Silier two comments above from @Stan