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

fix(youtube/custom-branding): use highest resolution adaptive icons #1576

Merged
merged 3 commits into from
Jan 31, 2023
Merged

fix(youtube/custom-branding): use highest resolution adaptive icons #1576

merged 3 commits into from
Jan 31, 2023

Conversation

LisoUseInAIKyrios
Copy link

I fixed the tiny app icon, and also increased the size of the adaptive images to the full resolution (108px, 162px, 216px, 324px, 432px)

I did this using the original high resolution image here on GitHub. By coincidence, it was the ideal size.

I also updated the old legacy ic_launcher.png and ic_launcher_round.png, although I don't think those are ever used because Android 8 uses the adaptive icons (and YouTube requires Android 8 or higher).

Before:
before

After:
after (whose the tiny one now?)

Visual size matches ReVanced Manager.
Adaptive images are Created from ReVanced png logo on github
@oSumAtrIX
Copy link
Member

It seems like the size is a bit off, here are the resources for the ReVanced logo with source and SVG files as reference: revanced-brand-identity.zip.

Copy link
Author

@LisoUseInAIKyrios LisoUseInAIKyrios left a comment

Choose a reason for hiding this comment

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

It seems like the size is a bit off, here are the resources for the ReVanced logo with source and SVG files as reference: revanced-brand-identity.zip.

This first iteration I made the icon the same size as ReVanced Manager:
after

I could revert the second change, and use this first change only.

Of note, ReVanced Manager also is using the wrong icon sizes (Manager is using 192px for xxxhdpi, when it should be 432px).

@oSumAtrIX
Copy link
Member

oSumAtrIX commented Jan 31, 2023

While it might look personally better, we should remain consistent with branding. There will soon be a logo poll that will allow for minor adjustments like your personal opinion before finalization, but in favour of brand consistency, it is currently required for the logo to match the exact size of the original logo.

@LisoUseInAIKyrios
Copy link
Author

Here it is constructed from revanced-logo-no-background.svg
latest

@oSumAtrIX
Copy link
Member

I believe the right one is the patched YouTube app. If so, this PR is ready for merge

@LisoUseInAIKyrios
Copy link
Author

LisoUseInAIKyrios commented Jan 31, 2023

I believe the right one is the patched YouTube app. If so, this PR is ready for merge

Yes that is correct. Merge away.

Should I do another PR to update the adaptive icons for the manager? It's has the same low resolution adaptive icons, which is why it looks a little blurry in these screenshots.

@oSumAtrIX
Copy link
Member

That sounds reasonable to me. Currently there is also the repo for the compose ReVanced Manager app, which might be relevant as well.

@oSumAtrIX oSumAtrIX merged commit c3ae750 into ReVanced:dev Jan 31, 2023
@oSumAtrIX
Copy link
Member

Thanks!

@LisoUseInAIKyrios LisoUseInAIKyrios deleted the launcher_icon_size_fix branch January 31, 2023 23:54
revanced-bot pushed a commit that referenced this pull request Jan 31, 2023
# [2.159.0-dev.1](ReVanced/revanced-patches@v2.158.0...v2.159.0-dev.1) (2023-01-31)

### Bug Fixes

* **youtube/custom-branding:** use high resolution adaptive icons ([#1576](ReVanced/revanced-patches#1576)) ([c3ae750](ReVanced/revanced-patches@c3ae750))

### Features

* **music:** bump patches compatibility to v5.41.50 ([#1551](ReVanced/revanced-patches#1551)) ([c456e45](ReVanced/revanced-patches@c456e45))
* **spotify-lite:** enable on-demand patch ([9f0de4f](ReVanced/revanced-patches@9f0de4f))
@kazimmt
Copy link

kazimmt commented Feb 1, 2023

@oSumAtrIX the edge of the changed logo is not correct :)
adaptiveproduct_youtube_foreground_color_108

@oSumAtrIX
Copy link
Member

Yep, I noticed already, will have to be fixed before merge to main

@LisoUseInAIKyrios
Copy link
Author

The image is stacked together with the background PNG, so it should never be seen individually. I will recreate using a variable alpha layer.

Is there a reason for both a foreground and background image? Why not use just 1 image?

@LisoUseInAIKyrios LisoUseInAIKyrios restored the launcher_icon_size_fix branch February 1, 2023 09:06
@oSumAtrIX
Copy link
Member

Iirc thats an Android specification.

@kazimmt
Copy link

kazimmt commented Feb 1, 2023

The image is stacked together with the background PNG, so it should never be seen individually. I will recreate using a variable alpha layer.

Is there a reason for both a foreground and background image? Why not use just 1 image?

I think everything was fine on last version
Except adaptiveproduct_youtube_foreground_color_108 in mipmap-xxxhdpi folder

Mistakenly it was a little small compare to others

@LisoUseInAIKyrios
Copy link
Author

LisoUseInAIKyrios commented Feb 1, 2023

I recreated them all using revanced-logo-shape.svg

edit: new PR: ReVanced/revanced-patches#1580

@kazimmt
Copy link

kazimmt commented Feb 1, 2023

I recreated them all using revanced-logo-shape.svg

new PR: #1576

Ok, whatever
It needs fix again
I'll wait till logo poll

@LisoUseInAIKyrios
Copy link
Author

I think everything was fine on last version Except adaptiveproduct_youtube_foreground_color_108 in mipmap-xxxhdpi folder

Mistakenly it was a little small compare to others

The screenshot I posted is my device that uses xxxhdpi. Maybe the GitHub visual diff makes them look a different ratio to each other, but that's the right size to match the Manager.

@oSumAtrIX
Copy link
Member

The ReVanced Manager app is also using wrong icon sizes, you should not match against it

@kazimmt
Copy link

kazimmt commented Feb 1, 2023

The ReVanced Manager app is also using wrong icon sizes, you should not match against it

The main icon (from branding zip) is not correctly aligned tho. (my 600% zoom -_- )

@LisoUseInAIKyrios
Copy link
Author

How much larger?

Here is:

368px (5% larger)385px (10% larger)

First is 5% larger. Second is 10% larger.

@oSumAtrIX
Copy link
Member

@kazimmt it is aligned, you can check the source file or inspect the svg path to confirm.

@oSumAtrIX
Copy link
Member

@LisoUseInAIKyrios Why is a scaling required at all? Can't the svg directly be used as the logo?

@kazimmt
Copy link

kazimmt commented Feb 1, 2023

@kazimmt it is aligned, you can check the source file or inspect the svg path to confirm.

Check Discord. I've already post the ss about alignment error.

@LisoUseInAIKyrios
Copy link
Author

So Android uses the center 2/3rd of the foreground image (72px / 108px)

the SVG uses 1/10% padding. It needs to be 1/3rd.

Creating the foreground SVG using (2/3)/(1728/1920) * 432 = 320px
then expanding the canvas of the 320px foreground image to 432px and using this as the foreground icon, Android shows this as:
(2-3)-(1728-1920) ratio of SVG

Original SVG output without the blank padding:
unpadded SVG output

Android shows the logo using 43.83% of the circle vertical space.
The original SVG shows the logo using 43.75% of the circle vertical space.

So now it's a match.

@LisoUseInAIKyrios
Copy link
Author

I changed the adaptive foreground to match the SVG and it's checked in on ReVanced/revanced-patches#1580

Ideally, any future ReVanced SVG logo changes would use the same padding ratios (1/6th padding on each side)

@oSumAtrIX
Copy link
Member

oSumAtrIX commented Feb 1, 2023

@LisoUseInAIKyrios I see the motivation behind using an 1/6th padding ratio, though if we would use that, the full logo would look off. The original padding of the shape in the logo project file was used in the shape svg file in order to correctly match in size with the full logo svg. Does that sound reasonable?

@LisoUseInAIKyrios
Copy link
Author

LisoUseInAIKyrios commented Feb 1, 2023

Then there's no strong reason to change the padding of the original SVG's. When exporting the icons Its easy to adjust the padding before exporting the SVG to PNG's.

@kazimmt
Copy link

kazimmt commented Feb 1, 2023

@LisoUseInAIKyrios why not we include the gradient background also?
It's part of logo background. Right?

@LisoUseInAIKyrios
Copy link
Author

@LisoUseInAIKyrios why not we include the gradient background also? It's part of logo background. Right?

You mean like this?

squaresquirclerounded squarecylinder

Is this the correct behavior? It's forcing the circle, even though the device is cutting it into a different shape.

@oSumAtrIX
Copy link
Member

I believe what he means is something like on the README file:

Logo

@kazimmt
Copy link

kazimmt commented Feb 1, 2023

Maybe padding a little more
So the black circle is not in the edge

Btw it's my personal preference
Idk what osuMatrix thought

@kazimmt
Copy link

kazimmt commented Feb 1, 2023

I believe what he means is something like on the README file:

Logo

Exactly

@kazimmt
Copy link

kazimmt commented Feb 1, 2023

I'm worried about this shape
216123197-c8d03e32-19c2-40cf-b14e-b68303b88986
Who use this 😅

@kazimmt
Copy link

kazimmt commented Feb 1, 2023

I believe what he means is something like on the README file:

Logo

Btw README.md logo also needs fix (as per branding guide)

@LisoUseInAIKyrios
Copy link
Author

LisoUseInAIKyrios commented Feb 1, 2023

I'm worried about this shape

It's the 'cylinder' shape from stock Android 12 launcher. Yeah it's an oddball.

@oSumAtrIX
Copy link
Member

Please PR or open an issue regarding this. @kazimmt

@oSumAtrIX
Copy link
Member

oSumAtrIX commented Feb 1, 2023

t'll look almost the same

I think whats necessary is a certain padding:

It should be noted that the correct padding would have to be added for Android in order to allow for the gradient border to look like this:

Logo

Otherwise it would look like this:

image

@kazimmt
Copy link

kazimmt commented Feb 1, 2023

@oSumAtrIX i think icon pack also matters
I test on miui with different icon pack
& here is the result

Screenshot_2023-02-02-00-33-20-619_com miui packageinstaller-edit
Screenshot_2023-02-02-00-34-01-760_com miui home-edit
Screenshot_2023-02-02-00-37-37-712_com miui home-edit
Screenshot_2023-02-02-00-38-37-773_com miui home-edit
Screenshot_2023-02-02-00-39-46-035_com miui home-edit

@oSumAtrIX
Copy link
Member

In that case, the gradient should not be added.

@kazimmt
Copy link

kazimmt commented Feb 1, 2023

In that case, the gradient should not be added.

Maybe there is another way
Let me try it

@LisoUseInAIKyrios
Copy link
Author

And the results:

squaresquirclecircleteardrop

@LisoUseInAIKyrios
Copy link
Author

In that case, the gradient should not be added.

Maybe there is another way Let me try it

What exactly was in that icon test pack?

@kazimmt
Copy link

kazimmt commented Feb 1, 2023

And the results:

squaresquirclecircleteardrop

Looks Good

@kazimmt
Copy link

kazimmt commented Feb 1, 2023

Maybe there is another way Let me try it

What exactly was in that icon test pack?

It's ReVanced Manager --
I just changed the name & package for testing -
-

@LisoUseInAIKyrios
Copy link
Author

Looks Good

I agree

@kazimmt
Copy link

kazimmt commented Feb 1, 2023

Needs test on Memeui (MIUI) & Other OS too

@oSumAtrIX
Copy link
Member

oSumAtrIX commented Feb 1, 2023

Time to move to ReVanced/revanced-manager#675 and ReVanced/revanced-patches#1580. Thanks!

revanced-bot pushed a commit that referenced this pull request Feb 1, 2023
# [2.159.0-dev.1](ReVanced/revanced-patches@v2.158.0...v2.159.0-dev.1) (2023-02-01)

### Bug Fixes

* **youtube/custom-branding:** correct margins and background image ([#1580](ReVanced/revanced-patches#1580)) ([8db2405](ReVanced/revanced-patches@8db2405))
* **youtube/custom-branding:** use high resolution adaptive icons ([#1576](ReVanced/revanced-patches#1576)) ([c3ae750](ReVanced/revanced-patches@c3ae750))

### Features

* **music:** bump patches compatibility to v5.41.50 ([#1551](ReVanced/revanced-patches#1551)) ([c456e45](ReVanced/revanced-patches@c456e45))
* **spotify-lite:** enable on-demand patch ([9f0de4f](ReVanced/revanced-patches@9f0de4f))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants