Add missing button shadows in dialogs

[a1exsh]

Relates to #4349

[a1exsh]

@ihhub can you please trigger that check again: it built OK, but just failed to upload the artifacts.

[a1exsh]

Original:

Changed:

[Branikolog]

Hi, @a1exsh
Looks fantastic! :)
But I've noticed, that the original central button has its shadow (which is a part of image) merged with the frame darkening of the frame. Is it possible to get rid of bright line (between shadow and the frame darkening) here?

[a1exsh]

@Branikolog good catch. I think we can fix that by extending the shadow by just one more pixel (currently it is -4,4).

[Branikolog]

@a1exsh
Also, it feels like shadow to the left contrasts more with the background, comparing to the same place near central button. Is it just due to another background texture?

[a1exsh]

@a1exsh Also, it feels like shadow to the left contrasts more with the background, comparing to the same place near central button. Is it just due to another background texture?

@Branikolog that is because the OKAY button and the shadow beneath it are part of the original resource, but we add shadow manually for our custom button, and it doesn't match 100%. Now that I think about it, in this case both buttons are of the same size, so what we can try is copying the original button together with the shadow instead.

[a1exsh]

Copied the shadow over from the OKAY button of the same dialog, it is now of the same darkness level:

[Branikolog]

@a1exsh
It's perfect!

[a1exsh]

I'm extending the scope to include more buttons where shadows are missing, e.g. the battle summary dialog:

[a1exsh]

Experimental settings dialog:

Game config dialog:

[a1exsh]

@Branikolog I've added missing shadows to all buttons that I have found. Maybe you would like to have a look ;)

[a1exsh]

Ah, there's also the battle-only dialog. Hide shadow from smaller the original EXIT button, center our button on screen, add shadow:

[a1exsh]

Hi, @a1exsh ! Found an issue with "Town Portal" buttons being pressed.

If the issue you're referring to is with button corners — it is not new and I'm waiting for this PR to be merged before addressing these. I think it was discussed already in the comments here.

[Branikolog]

Sorry, @a1exsh. I have too much tasks now. :) Missed this moment.
I was just confused by the fact, the latest commit version doesn't have these corners.

[a1exsh]

@Branikolog

I was just confused by the fact, the latest commit version doesn't have these corners.

It may be, but the issue is definitely present in the latest release and it's not directly related to the shadows, so let's address it separately.

[a1exsh]

@ihhub please find the time to have a look — I've extracted the relevant code to new subclasses of the base button class.

[ihhub]

Hi @a1exsh , I left few more comments in this pull request. Could you please take a look at them and check if they make sense, especially from architectural point of view?

[ihhub]

Hi @a1exsh , I put few more comments here. I feel we're almost there. Can you please check the comments?

[a1exsh]

@ihhub it complies and works, but I will not pretend I understood this std::move() magic 😅

[ihhub]

Hi @a1exsh , I left just few more optimization related comments. Could you please check them too?

[ihhub]

@ihhub it complies and works, but I will not pretend I understood this std::move() magic 😅

Move semantics are used to avoid memory copy and only for objects which are going to be destroyed just after the call. This is useful when you can just swap values instead of copy them. In case of images it's essential to avoid extensive memory manipulations.

[a1exsh]

Move semantics are used to avoid memory copy and only for objects which are going to be destroyed just after the call.

Well, that is clear. What I don't get is why in some places it's OK to just call new ButtonSprite( ... ) and in other places you have to specify std::move() explicitly...

[ihhub]

Well, that is clear. What I don't get is why in some places it's OK to just call new ButtonSprite( ... ) and in other places you have to specify std::move() explicitly...

std::move enforces in some sense to end object's lifetime within your code block. If you don't do this then you're passing by normal reference rather than by r-value.

[ihhub]

@Branikolog please retest the latest build again as we changed a lot of internal code.

[a1exsh]

appveyor build failed with an internal error, please re-trigger:

MSBUILD : error MSB4017: The build stopped unexpectedly because of an unexpected logger failure.
Microsoft.Build.Exceptions.InternalLoggerException: The build stopped unexpectedly because of an unexpected logger failure. ---> Microsoft.Build.Exceptions.InternalLoggerException: The build stopped unexpectedly because of an unexpected logger failure. ---> System.Net.WebException: The remote server returned an error: (500) Internal Server Error.

[ihhub]

@a1exsh , a huge thanks for these changes!