Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding Vertical Gradient for fonts #8262

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Conversation

MaMadDl
Copy link
Contributor

@MaMadDl MaMadDl commented Jan 12, 2024

based on Discussion #8078
Adding colors bounds for all the colors in pal.h
Adding 3 point Linear vertical gradient creator
Adding new font color for NORMAL and LARGE fonts

Sample Output of Fonts:
Large Font:
Screenshot 2024-04-14 214121
Screenshot 2024-04-14 221955

Normal Font:
Screenshot 2024-04-14 214214
Screenshot 2024-04-14 222113

Adding GOLDEN_GRADIENT ICN for Normal And Large Fonts
…or LARGE and NORMAL font type

- Adding ICN generation for both font types
- style Fix
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.h Outdated Show resolved Hide resolved
@zenseii zenseii added improvement New feature, request or improvement ui UI/GUI related stuff labels Jan 12, 2024
@zenseii zenseii added this to the 1.1.0 milestone Jan 12, 2024
Copy link
Collaborator

@zenseii zenseii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @MaMadDl, I left some suggestions. Please have a look at them should you have the time.

src/engine/pal.cpp Outdated Show resolved Hide resolved
src/fheroes2/agg/agg_image.cpp Outdated Show resolved Hide resolved
src/fheroes2/agg/agg_image.cpp Outdated Show resolved Hide resolved
src/fheroes2/gui/ui_text.h Outdated Show resolved Hide resolved
src/fheroes2/agg/agg_image.cpp Outdated Show resolved Hide resolved
@ihhub
Copy link
Owner

ihhub commented Jan 13, 2024

Hi @MaMadDl , before proceeding with this pull request review please update the original description to include all fonts that you are adding here so every reviewer will have visual interpretation of them.

@ihhub ihhub modified the milestones: 1.1.0, 1.0.12 Jan 13, 2024
Copy link
Contributor Author

@MaMadDl MaMadDl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @zenseii
Requested Reviews have been changed
also for Large Font I've changed the outline pixel width to 2

Please Take a look

@Mr-Bajs
Copy link
Contributor

Mr-Bajs commented Jan 13, 2024

Hello @zenseii Requested Reviews have been changed also for Large Font I've changed the outline pixel width to 2

Please Take a look

Most letters does have some flair to them like the original, but the S is a little bit off? The original S have extra curves at the ends, but this is straight at the ends, somewhat as an inverted Z.

@MaMadDl
Copy link
Contributor Author

MaMadDl commented Jan 13, 2024

@Mr-Bajs you're right.
I'm overwriting the letter color by checking for value in its transform layer.
where at both ends of S it starts get thinner, I suspect I'm overwriting that the transparent part.

@zenseii what do you think

Edit:
The Flair part is not transparent but it's black. so transform layer doing as intended.
Sure

I might have to check for black pixel value as well as transform layer.
Works fine for small s

Fixed
New Fonts looking more detailed
LargeFont
Normal Font

also if possible please check with other Languages. I have access only to English

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
@zenseii
Copy link
Collaborator

zenseii commented Jan 14, 2024

@MaMadDl in general yours looks very good so good work on that! I've looked closer on your generated fonts and have some comments about how we can improve them further:
Here is a comparison image with the original game's font in the middle:
image

Please also have a look at the clang-tidy comments above that @ihhub pointed out that were valid.

Also as a tip in case you don't know, you can use fheroes2::Save() function from image_tool.h to save images like pngs or bmps like fheroes2::Save( imageObject, "imageName.png", 96 ). In for loops you can use std::to_string with the iterator in the filename if you want to quickly save all images. This is easier than having to start the game and create a window, plus it will show you better where the image boundaries and transparent pixels are.

@zenseii
Copy link
Collaborator

zenseii commented Jan 14, 2024

I forgot to add the comments on how to improve it... What I suggest is to instead of a black contour where you have it now, it should first be a darker brown contour, and then outside of that a black contour. Also it seems that in your algorithm some colors which are converted to dark brown in the original effect are instead converted to the yellow hue in yours. Maybe this could be tweaked?

I'm talking about areas like these:
image

Also I suggest ignoring orange-y pixels like these because they are likely just artifacts and would probably look fine if they adhered to the overall effect:
image

EDIT:
The end result I'm hoping we can accomplish would be something like this for the R:
image

It will be 1 pixel taller because the original one is apparently cut in the middle but this is fine. Comparing your golden gradient effect to the original shows that yours is better already.

@MaMadDl
Copy link
Contributor Author

MaMadDl commented Jan 15, 2024

Hello @zenseii
Thanks for the tip on saving image

here's modified version of Fonts
Normal
NormalR
Large
LargeR

I have changed the function to take in a Color as well
and wrap all of them with one black contour
also for the expanded width i noticed there's stretch method. can't we do something with that ?

fixed Clang-Tidy issues mentioned by @ihhub

Let me know what you think

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

src/engine/image.cpp Outdated Show resolved Hide resolved
@zenseii
Copy link
Collaborator

zenseii commented Jan 15, 2024

Hi, @MaMadDl.

NormalR

This is looking very good now! I've looked closer at the original font and noticed that there is a slightly grayer dark contour outside of the black one. I've tried to highlight it here outside of the red line in the image below. Note that it is not the shadow because it goes all the way around, and this case specifically of the font doesn't have a shadow. In places where it does have shadow that gray contour is still there.

dimmer dark contour

In addition I think we could try to see if we can manage to cut one line off from the letters by doing two Copy() calls: First we copy the upper half, and then the lower half offset by 1 line of pixels down.
Something like

Copy( in, 0, 0, out, offsetX, offsetY, inWidth, inHeight / 2 );
Copy( in, ( inHeight / 2 ) + 1 , 0, out, offsetX, offsetY + ( inHeight / 2 ), inWidth, ( inHeight / 2 ) - 1 );

It's very possible that this will work better on some letters than others, I'm thinking that the E might have some issues with this, and it might be impossible to do this in a general way, in which case we can easily live without this.

I also see that the amount of shadow for the letters depend on the context they are used in but for now I think having 2 pixels' width of shadow overall is fine.

What do you think?

Large
LargeR

This larger font is looking much better too. For the game we have this larger font that is used:
image

It looks like it's scaled up 2x - 1/7 = 0.8572 or something since the original height is 14 px and the larger one is 26 px tall. I'm not sure how best to replicate that. EDIT: Actually, looking closer I can see that the inner larger letter without the contour is exactly 2x the size of the inner smaller letter, so that should make it equivalent to the larger font that you've made.

I think we could try to see if we can cut off the middle part of the larger one too, like I described for the normal font.

also for the expanded width i noticed there's stretch method. can't we do something with that ?

Do you mean to add another style of font with that? If you have any ideas where we could use that then sure. As for this current PR I think it's best if we focus on implementing the golden gradient font first and then we can open a PR with other fonts after.

Copy link
Collaborator

@zenseii zenseii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @MaMadDl. I left some more comments. Please have a look should you have the time.

src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.h Outdated Show resolved Hide resolved
src/fheroes2/agg/agg_image.cpp Outdated Show resolved Hide resolved
src/fheroes2/agg/agg_image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
@MaMadDl
Copy link
Contributor Author

MaMadDl commented Jan 17, 2024

Hello @zenseii

I have added a version of silver gradient however it's slightly thicker than those in campaign
here's an example for letter H:
Original:
Screenshot 2024-01-16 043858
Recreated:
Screenshot 2024-01-16 044005

there's also a problem when I'm trying to separate original white font shade(pixels that's slightly darker) where we are checking by value (Lines 3133 and 3153 in image.cpp ) and i think it's conflicting with Silver color creation and wasn't able to find good solution for it yet.

let me know what you think

@zenseii
Copy link
Collaborator

zenseii commented Jan 18, 2024

@MaMadDl. It's looking very promising!

Are you saying that because of the problem with distinguishing white color the lower half of the letter doesn't have the gradient effect applied?

I will look at the actual code to get some understanding once I have time.

I don't think our font being thicker will pose a problem so that's fine.

@ihhub
Copy link
Owner

ihhub commented Jan 19, 2024

Hi @MaMadDl , please use Resize() method for resizing things. Stretch() is for different purposes.

@zenseii zenseii self-requested a review January 28, 2024 14:06
@zenseii
Copy link
Collaborator

zenseii commented Feb 5, 2024

@MaMadDl, I've had a look at the current effect generation but since this isn't really my territory I couldn't really find any obvious solution to the issues with the silver gradient effect. I'll have a look again once I have time and perhaps if @Districh-ru has some to spare then any input would be appreciated since he has much more knowledge in this area.

@ihhub ihhub modified the milestones: 1.0.12, 1.1.0 Feb 7, 2024
@Districh-ru Districh-ru self-requested a review February 7, 2024 15:23
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
@MaMadDl
Copy link
Contributor Author

MaMadDl commented Apr 14, 2024

Hi @zenseii,

I've managed to fix the issues and made the method a lot simpler and shorter

the final output are:
Silver Gradient (Normal and Large):
silverNormalH
silverLargeH

Golden Gradient(Normal and Large):
GoldNormalH
GoldLargeH

@zenseii
Copy link
Collaborator

zenseii commented Apr 18, 2024

@MaMadDl. Well done! I've checked the two effects and compared them to the original and the silver one looks perfect now. The golden gradient needs just a little bit adjustment. It is a bit too bright and the yellow color needs to be a bit darker/more towards red/brown:
Original above, generated effect below:
image

As I said the silver gradient effect looks 1:1 in my opinion:
image

@MaMadDl
Copy link
Contributor Author

MaMadDl commented Apr 19, 2024

Hi @zenseii
here is the fixed version
Screenshot 2024-04-19 193631

and their pixel comparison:
Recreated:
Screenshot 2024-04-19 193539
Original:
Screenshot 2024-04-19 193434

Note that center-line in original image is actually White not Yellow which we cannot incorporate into our code so we used light Yellow in its place

Copy link
Collaborator

@Districh-ru Districh-ru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @MaMadDl, thanks for the gradient fonts implementation, I like how it looks.
I left several comments, could you please take a look when you have time.

src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.h Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
Copy link
Contributor Author

@MaMadDl MaMadDl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Districh-ru
I have made the changes requested , would please take a look?
also on one of the changes requested I had some issues which is commented on.

Copy link
Collaborator

@Districh-ru Districh-ru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MaMadDl, I left some more comments. Could you please check them?

src/engine/image.h Outdated Show resolved Hide resolved
src/engine/image.cpp Outdated Show resolved Hide resolved
Copy link
Contributor Author

@MaMadDl MaMadDl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @Districh-ru
I've made the changes requested, could you please check them ?

Copy link
Collaborator

@Districh-ru Districh-ru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done, @MaMadDl !

@ihhub ihhub modified the milestones: 1.1.0, 1.1.1 May 22, 2024
@ihhub ihhub modified the milestones: 1.1.1, 1.1.2 Jul 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement New feature, request or improvement ui UI/GUI related stuff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants