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

Few fixes for asset store browser #13008

Merged
merged 1 commit into from
Nov 20, 2017
Merged

Few fixes for asset store browser #13008

merged 1 commit into from
Nov 20, 2017

Conversation

Chaosus
Copy link
Member

@Chaosus Chaosus commented Nov 18, 2017

  1. Set an icon size to constant 80x80, assets will be stretched to this size. I think its better be constant for all assets, cuz different sizes of different assets looks ugly

  2. Added missed icons for rating stars(used python script for generate header)

So the result looks like
image
yes I fill star with gray color, cuz transparent looks very hard for eyes(on dark themes)
and(if stars is active)
image

@leoddd
Copy link
Contributor

leoddd commented Nov 18, 2017

Godot already comes with a star icon, couldn't that be used? The one displayed in the project browser for example.

@Chaosus
Copy link
Member Author

Chaosus commented Nov 18, 2017

Ah.. word "favorite".. but it is in svg format, needs to be converted into png

@Chaosus
Copy link
Member Author

Chaosus commented Nov 18, 2017

image

image
looks good ?

@reduz
Copy link
Member

reduz commented Nov 18, 2017

It definitely looks better, hope @djrm eventually gives it a lift in general :)

@Chaosus
Copy link
Member Author

Chaosus commented Nov 18, 2017

Ok, than I done for myself :) Its not good if in beta people looking on ERROR messages in place of rating stars, even if it is impossible(for now) to rate from asset store

@Chaosus
Copy link
Member Author

Chaosus commented Nov 18, 2017

OOps, found a bug inside 1).. needs to be fixed before merged

@Chaosus
Copy link
Member Author

Chaosus commented Nov 18, 2017

Ok, now its looking correct, I setup thumbnail and screenshots to correct size

image

Ready to merge !

@Chaosus
Copy link
Member Author

Chaosus commented Nov 18, 2017

I also fix invalid screenshot size so

image

@ghost ghost added this to the 3.0 milestone Nov 18, 2017
@vnen
Copy link
Member

vnen commented Nov 18, 2017

Ah.. word "favorite".. but it is in svg format, needs to be converted into png

Not really, Godot has support for SVG and all icons are in this format, no need to convert them.

@Chaosus
Copy link
Member Author

Chaosus commented Nov 18, 2017

Than why is make_icon method existed and called multiple times in default_theme.cpp ? It used to load png icon and I used it...

@Chaosus
Copy link
Member Author

Chaosus commented Nov 18, 2017

I didnt find function for load icon from SVG, can someone help me or its fine to use png ?

@djrm
Copy link
Contributor

djrm commented Nov 18, 2017

Using the header icons is discouraged, and actaually we only do that to keep the old editor theme, the thing you have to do to get icons is simply call
get_icon("Favorites", "EditorIcons");
and
get_icon("NonFavorite", "EditorIcons");

screenshot from 2017-11-18 16-47-57

and you are getting the rendered SVGs, alternatively you can put an optimized version of your SVGs in editor/icons, all of them are placed under a header and then rendered in editor, so if you put an icon there called icon_star.svg you can retrieve it by calling get_icon("Star", "EditorIcons");

The optimization of the svg is quite important since it affects the binary size and the editor boot time.

Copy link
Contributor

@djrm djrm left a comment

Choose a reason for hiding this comment

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

please check my other comment to see how to load svg icons, that would make this PR simpler

// Asset store

theme->set_icon("RatingNoStar", "EditorIcons", make_icon(star_png));
theme->set_icon("RatingStar", "EditorIcons", make_icon(star_active_png));
Copy link
Contributor

Choose a reason for hiding this comment

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

not good, deafult_theme should not be touched except for bug fixes, and this is not one, check my comment on how to load icons.

image->resize(image->get_width() * scale_ratio, image->get_height() * scale_ratio, Image::INTERPOLATE_CUBIC);
case IMAGE_QUEUE_ICON:

image->resize(80, 80, Image::INTERPOLATE_CUBIC);
Copy link
Contributor

Choose a reason for hiding this comment

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

this might need to be multiplied by EDSCALE, so it works in hiDPI

@Chaosus
Copy link
Member Author

Chaosus commented Nov 19, 2017

How its now ? A much cleaner I think... I applied hidpi fix as @djrm suggested

break;
case IMAGE_QUEUE_THUMBNAIL:

image->resize(134 * EDSCALE, 69 * EDSCALE, Image::INTERPOLATE_CUBIC);
Copy link
Contributor

Choose a reason for hiding this comment

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

are you sure the backend always produce thumbnails with this aspect ratio, i dont see why we should asume such thing, as far as i can tell a thumbnail should also preserve its aspect ratio, with the icon it kinda makes sense but with this im not sure.

@Chaosus
Copy link
Member Author

Chaosus commented Nov 19, 2017

And now its ok ?

@@ -297,7 +297,7 @@ EditorAssetLibraryItemDescription::EditorAssetLibraryItemDescription() {

previews_bg = memnew(PanelContainer);
vbox->add_child(previews_bg);
previews_bg->set_custom_minimum_size(Size2(0, 85));
previews_bg->set_custom_minimum_size(Size2(0, 101));
Copy link
Contributor

Choose a reason for hiding this comment

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

every time you use sizes for GUI you have to multiply by EDSCALE, so in this case it will be
previews_bg->set_custom_minimum_size(Size2(0, 101) * EDSCALE);

@djrm
Copy link
Contributor

djrm commented Nov 19, 2017

i think its ok, well there is some code repetition but its not that bad.

@akien-mga
Copy link
Member

Nice work and thanks for your patience :)

@akien-mga akien-mga merged commit 7f52db7 into godotengine:master Nov 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants