RetroPie / EmulationStation

A Fork of Emulation Station for RetroPie. Emulation Station is a flexible emulator front-end supporting keyboardless navigation and custom system themes.
Other
858 stars 344 forks source link

Discussion on grid view theming syntax #206

Closed benjdero closed 5 years ago

benjdero commented 7 years ago

EDIT : lastest theming syntax proposition can be found on the wiki : https://github.com/RetroPie/EmulationStation/wiki/Gridview-Roadmap-and-FAQ#theming-syntax

pjft commented 7 years ago

@zigurana summoned as well. :)

Thanks for creating this.

As I mentioned, a bit short on time these days but will try to read this through this week.

Thank you.

zigurana commented 7 years ago

Present! I'll have a look at the document this week.

Thank for your work!

Op zo 13 aug. 2017 18:41 schreef pjft notifications@github.com:

@zigurana https://github.com/zigurana summoned as well. :)

Thanks for creating this.

As I mentioned, a bit short on time these days but will try to read this through this week.

Thank you.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/RetroPie/EmulationStation/issues/206#issuecomment-322052562, or mute the thread https://github.com/notifications/unsubscribe-auth/AF0i2LM52pBaRXvmTIpWR_XvoIZ7UR_Mks5sXydRgaJpZM4O1uzo .

lilbud commented 7 years ago

I'm here as well, and I'll read the document and suggest changes.

Ah shit, I just realized that I was the only person to implement gridview into a theme.

jrassa commented 7 years ago

@Koerty I will read it over and try to post some feedback tonight. You should post this to the forums as well. You are more likely to get input from the theme makers there.

benjdero commented 7 years ago

@jrassa I wanted to post it on the forum at first but I coudln't tag you because I don't know your nickname, plus at this time the google doc was just a bunch of copy pasted xml code. I still need to make it less like basic notes and more like a real specification document before I post it on the forum but my broken english doesn't help :)

@zigurana Thanks. Maybe you can take some inspiration from it or give suggestion based on your work on a more modular game veiw.

jrassa commented 7 years ago

@Koerty I plan to look over this more will provide a more thorough suggestion, but for now I wanted to say that I would favor sticking to a syntax similar to the current gamelist. The reason being that any of the other suggestions you had would require a lot of work rewriting the theme XML parsing code for what I would argue is little added benefit.

One additional question, If a theme can specify the size of the grid and the size of the grid tiles, does it make sense to be able to specify number of rows and columns? I would think that the number of rows and columns would be calculated based on the values above. It would also make the grid view more flexible for different screen sizes.

jrassa commented 7 years ago

Also github issues does have syntax highlighting. The way you do it is add the language after the ```

Default:

<text name="example">
    <property>value</property>
</text>

With syntax highlighting (```xml)

<text name="example">
    <property>value</property>
</text>
zigurana commented 7 years ago

Hi! I had a look at your document, and while I am in no way an XML expert, I still have an opinion :-p. From your variations, my preference would be to use something as close as possible to what is already being used, so basing the new grid on the current syntax for textList. This has the advantage of allowing themers to run with it directly, and not breaking much prior art (sorry @lilbud).

I do not really see the added benefit of going to any of the other approaches (XAML or the nested approach). What problem are we solving with those strategies? Are there constructions that are not possible using the current way we define the XML? If anything, the end-result would need to be internally consistent, so at least similar for all theming elements. Therefore, introducing a new (even slightly better) way of defining the theme would either break that consistency, or require many themes to be updated. Both are dealbreakers, at least for me.

It's maybe not perfect, but those are the constraints we need to engineer within.

So my preference would be:

<imagegrid name="gamegrid">
    // Grid settings
    <size>
    <pos>
    <margin>
    <rowsAndColumns> (set how many rows and columns a grid has)
    <rowsAndColumnsSmall>
    <rowsAndColumnsBig>
    <pathMissingBoxArt> (Replace the missing boxart texture)
    // Gridtile's settings
    <tileSize> (sets size of gridtile and overrides default size based on boxart)
    <tileColor> (not finished, but can be used) (can set Alpha)
    <tileBackgroundPath> (changes gridtile's background)
    <tileBackgroundColor> (RGBA)
    <tileBackgroundPadding>
    <tileTextSize> (only height can be modified)
    <tileTextColor> (only height can be modified)
    [...] (all text properties can be applied)
    // Selected gridtile's settings
    <selectedTileSize> (will add to default size if gridtile size is left unasigned)
    <selectedTileColor> (will set Alpha)
    <selectedTileBackgroundColor> (RGBA)
    <selectedTileTextSize> (only height can be modified)
    <selectedTileTextColor>
    [...] (all text properties can be applied)
</imagegrid>

But I assume you find this too much of an unstructured long list?

benjdero commented 7 years ago

@zigurana Thanks for your feedback (and also for your reviews on my PR).

The thinkings behind this is the following :

So, since we are already going to break backward compatibility and rewrite part of the xml parsing anyway, why not put a little much more efforts into this to create a sexier syntax (by sexier I mean easier to read/write/understand and more modular) ?

pjft commented 7 years ago

I'm only following this via email, and not doing a great job at that.

Still I suppose that, unless these syntax changes affect only the gridView, all other themes would need to be updated for them to work in ES, so the less effort/more familiar it can be for current theme makers, the better. Even if it's just for the new view, keeping it close to what the theme community has been used to, unless something is clearly impossible to pull off, would likely be preferable.

On Thu, Aug 17, 2017 at 12:33 PM, Koerty notifications@github.com wrote:

@zigurana https://github.com/zigurana Thanks for your feedback (and also for your reviews on my PR).

The thinkings behind this is the following :

  • If we change the grid syntax, we already break backward compatibility with themes using the current grid syntax.
  • The syntax for textlist look ok, not good, just ok, due to some "weirds" part (for example fontPath and fontColor are just basic types, respectivly path and color, which we redefine here just to know on what element we need to apply them). Now if I use the same syntax for the gamegrid, it look really ugly to me.
  • The last point is you are already going to rewrite some parts of the xml parsing in your quest of a more modular game view.

So, since we are already going to break backward compatibility and rewrite part of the xml parsing anyway, why not put a little much more efforts into this to create a sexier syntax (by sexier I mean easier to read/write/understand and more modular) ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/RetroPie/EmulationStation/issues/206#issuecomment-323044937, or mute the thread https://github.com/notifications/unsubscribe-auth/AVAV7dlNiC2rBedzraKlRngUPf7_CMasks5sZCUXgaJpZM4O1uzo .

lilbud commented 7 years ago

I'm all for easier gridview syntax, as long as it's easy to use and understand. Breaking older gridview themes wouldn't be that big of a problem...as long as it is easy to update existing themes (All 2 of them) to work with the new syntax and also make it easy to add grid support to new/already released themes without grid.

pjft commented 7 years ago

I just read everything and the document, and it seems this is mostly for GridView syntax, so disregard the majority of my previous comment.

I added some comments in the doc, but the summary is:

On Thu, Aug 17, 2017 at 4:54 PM, lilbud notifications@github.com wrote:

I'm all for easier gridview syntax, as long as it's easy to use and understand. Breaking older gridview themes wouldn't be that big of a problem...as long as it is easy to update existing themes (All 2 of them) to work with the new syntax and also make it easy to add grid support to new/already released themes without grid.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/RetroPie/EmulationStation/issues/206#issuecomment-323115428, or mute the thread https://github.com/notifications/unsubscribe-auth/AVAV7YCO9EoS_j9I1VrKdYZeO1n6pxIJks5sZGIigaJpZM4O1uzo .

jrassa commented 7 years ago

If we change the grid syntax, we already break backward compatibility with themes using the current grid syntax.

This feature was never officially supported so we can't really break backward compatibility.

The last point is you are already going to rewrite some parts of the xml parsing in your quest of a more modular game view.

The refactoring work that @zigurana is doing will not effect the xml parsing. He is refactoring how the gamelists are created so that the elements are dynamically created.

My biggest issue with the current syntax is the misuse of container elements. I am ok with the way it is using multiple image elements for the different images. The carousel does this a bit for the logos. I'd rather take that approach, then have to change the xml parsing logic.

benjdero commented 7 years ago

Current grid theming syntax

<view name="grid">
    <text name="md_title">
        <color>
        <backgroundColor>
        <alignement>
        <text> <!-- Ok there is no point in changing the text manually but you can still do it -->
        <forceUppercase>
        <lineSpacing>
    </text>

    <container name="md_grid_margin">
        <size>
    </container>

    <container name="gridRowsAndColumns">
        <size>
    </container>

    <image name="missing_boxart">
        <path>
    </image>

    <text name="gridtile_text">
        <color>
        <backgroundColor>
        <alignement>
        <text> <!-- Ok there is no point in changing the text manually but you can still do it -->
        <forceUppercase>
        <lineSpacing>
    </text>

    <image name="gridtile_selected">
        <color>
        <size>
    </image>

    <image name="gridtile">
        <color>
        <size>
    </image>

    <container name="gridtile_textRow_selected">
        <size>
    </container>

    <container name="gridtile_textRow">
        <size>
    </container>

    <image name="gridtile_background_selected">
        <color>
        <size>
    </image>

    <image name="gridtile_background">
        <path>
        <color>
        <color-center>
        <color-edge>
        <padding>
    </image>

    <text name="gridtile_text_selected">
        <color>
    </text>

    <text name="gridtile_text">
        <color>
    </text>
</view>
benjdero commented 7 years ago

So I guess you wanna go with this one ? Which I find monstrously ugly and unclear, but this is just my opinion.

<view name="grid">
    <!-- I removed the 'text' field since there is no point in changing the text manually (it is erased as soon as a game is selected) -->
    <text name="md_title">
        <color>
        <backgroundColor>
        <alignement>
        <forceUppercase>
        <lineSpacing>
    </text>

    <imagegrid name="gamegrid">
        <margin>
        <rowsAndColumns>
        <pathMissingBoxArt>
        <gridTileTextColor>
        <gridTileTextBackgroundColor>
        <gridTileTextAlignement>
        <!-- Also removed text field of gridtile -->
        <gridTileTextForceUppercase>
        <gridTileTextLineSpacing> <!-- not sure if we also need to remove this one -->
        <gridTileColor>
        <gridTileSize>
        <gridTileColorSelected>
        <gridTileSizeSelected>
        <gridTileTextColor>
        <gridTileTextColorSelected>
        <gridTileTextRow>
        <gridTileTextRowSelected>
        <gridTileBackgroundPath>
        <gridTileBackgroundColor>
        <gridTileBackgroundColorCenter>
        <gridTileBackgroundColorEdge>
        <gridTileBackgroundPadding>
        <gridTileBackgroundColorSelected>
        <gridTileBackgroundSizeSelected>
    </imagegrid>
</view>
jrassa commented 7 years ago

@Koerty While I would agree that it is a bit ugly, I don't really think it is anymore unclear then the nested xml syntax that you proposed and I believe that it is clearer then the current syntax with multiple disparate elements.

However, I would be ok with a middle ground approach between the 2 syntaxes. My biggest concern with the current syntax is all the various elements that only hold a single value, especially the container elements.

At a minimum I think margin and rowsAndColumns should be removed. These values should be calculated based on the size of the imagegrid and and the size of the grid tile.

As well, there seem to be some odd inconsistencies between what can be set on gridtile_background and gridtile_background_selected.

benjdero commented 7 years ago

@jrassa My main concern is not that it's ugly, it's that it is a bit unclear what you set and what impact it will have on the grid. Their is so much customizations options for the grid. I need to work on make it more clear and easy to understand.

About margin, rows and columns, I had something like that in my mind but not the same approach. I think removing margin and rowsAndColumns is a bad idea for 2 reasons:

This would be such a pain to manage for theme makers.

For me the only parameter we should remove is the number of rows, a theme could already have a "good looking gridview" (according to the theme maker personal preferences) just by defining the grid size, margins and the number of columns.

The grid would then calculate the tile width and height accordingly, and the number of rows which can fit. The bottom row would be cropped until the user scroll down to that row.

gridview1

When the user scroll down the grid and reach the 3rd row on this drawing, the tiles are pushed up so the currently selected tile is not cropped.

gridview2

Similar behaviour will also happen when going up.

This is something I'm thinking about since I first started working on the grid view, but this would ask so much modifications on the formulas that calculate how the tiles are displayed that I need more time to do it.

Again, sorry for my garbage english, I hope you can still understand what I mean :)

jrassa commented 7 years ago

Second, I think the theme maker should have total control on margins.

For me the only parameter we should remove is the number of rows

The problem I see with this is that it won't work well when the theme is run at different resolutions and aspect ratios. It a theme specifies that it wants five columns and with a set margin, and was design to work with a widescreen aspect ratio, it will look very bad when running 4:3, as all of the tiles will be squished horizontally, or worse some will be rendered offscreen. If the theme just specifies a tile size, then it can dynamically adjust by calculating how many columns can fit the on screen and choosing an appropriate margin so that everything looks centered.

so if I wanted to change the total grid size of my theme, I would have to recalculate the width of my tile by dividing the grid size by the number of columns I want (+ the margins), then recalculate the height of my tiles for every system based on the common cover width/height ratio

So we are essentially dealing with the following equation.

SIZE = COLUMNS * (TILE_SIZE + MARGIN)

Theme makers would still have to calculate the tile width or else they would potentially be rendered too wide and flow offscreen, or too narrow and leave a bunch of unused screen space.

benjdero commented 7 years ago

The problem I see with this is that it won't work well when the theme is run at different resolutions and aspect ratios.

They won't be squashed or go offscreen they will just be smallers. And we will have this problem with all the solutions proposed, we can't render everything properly whatever you use emulationstation on a 1080p 15" monitor or a 1080p 100" tv screen or a 800*600 monitor. That's why I submited the idea to allow users to override this parameter (the number of columns), but you already tackled that idea before.

If the theme just specifies a tile size, then it can dynamically adjust by calculating how many columns can fit the on screen and choosing an appropriate margin so that everything looks centered.

Wait, are you talking about a size in % or in pixels ? If it's in % we will have the exact same problem than before plus it's harder for theme maker to calculate as stated before, if it's in pixels it won't solve the problem properly, a 400px width tile may be ok on a 1080p 15" monitor but too big on a 1080p 100" tv screen.

So we are essentially dealing with the following equation. SIZE = COLUMNS * (TILE_SIZE + MARGIN)

Your equation is wrong, it should be: GRID_SIZE = COLUMNS * TILE_SIZE + (COLUMNS - 1) * MARGIN

As there will always be one less margin than tile.

Also, you will have to calculate this for every system, because if you set a 400px width tile for everywhere, megadrive games cover will have around 600px height, but n64 games cover will only have around 300px height, thus being significantly smallers.

lilbud commented 7 years ago

could 'maxSize' be used to determine the size of the tiles?

benjdero commented 7 years ago

could 'maxSize' be used to determine the size of the tiles?

What do you mean ?

jrassa commented 7 years ago

Wait, are you talking about a size in % or in pixels ? If it's in % we will have the exact same problem than before plus it's harder for theme maker to calculate as stated before, if it's in pixels it won't solve the problem properly, a 400px width tile may be ok on a 1080p 15" monitor but too big on a 1080p 100" tv screen.

All sizes in ES are currently expressed in %. Anything being added, including GridView, should do the same.

Your equation is wrong, it should be: GRID_SIZE = COLUMNS TILE_SIZE + (COLUMNS - 1) MARGIN

I was simplifying things. However, it doesn't change my point. For sake of simplification the following will only deal with the width and will be expressed as % of screen width (0.8 = 80%) Currently GRID_SIZE is hardcoded to .8 and TILE_SIZE, COLUMNS and MARGIN are all currently configurable.

If I were to create a theme with the following values: COLUMNS = 4 TILE_SIZE = 0.3 MARGIN = 0.01

Then: GRID_SIZE = 4 0.3 = 3 0.01 = 1.23

If I were to then set TILE_SIZE = 0.2 Then: GRID_SIZE = 4 0.1 = 3 0.01 = 0.43 0.43 is smaller then the currently hardcode width of 0.8 which would cause excess empty space.

If we go back to the equation: GRID_SIZE = COLUMNS TILE_SIZE + (COLUMNS - 1) MARGIN Currently all of the variables are configurable which can break the equation. At least one value needs to be dynamic and calculated at runtime. It sounds like you intend to make tile size dynamic, but I would think that it would be more important to theme creators to control the size of the grid tile vs. the number of grid tiles. This also would align with how the textlist currently works. Themes can specify the size of the textlist and the font size of the individual text items, and the number of rows/entries is computed dynamically.

Also, you will have to calculate this for every system, because if you set a 400px width tile for everywhere, megadrive games cover will have around 600px height, but n64 games cover will only have around 300px height, thus being significantly smallers.

This will still be an issue with your approach, just inverted to effect width instead of height. If the number of columns is set to 4 everywhere, n64 covers that are wider will be rendered larger then megadrive covers that are narrower.

Also keep in mind that not everyone will even use box covers for the grid tile images. Some may choose to use screenshots or logos. Most themes will likely be designed in a way to be flexible enough to handle whatever image the user has scrapped and won't try to maximize the number of visible tiles.

lilbud commented 7 years ago

maxSize will define the limits that an image can stretch. So if maxSize is set to 'x=400px, y=400px' then the image will stretch until one of those has been met, and scales the other side equally. So for N64 games, the art will stretch to 400px wide and scale the y axis accordingly

benjdero commented 7 years ago

Currently all of the variables are configurable which can break the equation. At least one value needs to be dynamic and calculated at runtime.

Yes and my proposition is to calculate the tile size at runtime, which allow both the theme maker and the user, if you accept my proposition to allow him to change the grid number of columns, to easily control the size of the tiles.

This will still be an issue with your approach, just inverted to effect width instead of height. If the number of columns is set to 4 everywhere, n64 covers that are wider will be rendered larger then megadrive covers that are narrower.

Yes, but it is way easier for theme makers to setup the gridview to have let's say 4 columns globally, 3 columns for the n64, 5 for the megadrive, than search for games cover aspect ratio on google and use equations to calculate the tile width, and repeat all of this steps for all the systems he want to support.

Plus as I said before, even you do all of this, the grid view may render good with your settings on a 1080p 15" laptop screen and completely garbage on a 1080p 100" tv screen.

benjdero commented 7 years ago

Just tested current implementation.

gridtile_selected is the size of the tile when selected, this act like a weird zoom since:

<image name="gridtile_selected">
    <size>0.5 0.5</size>
</image>

will make the selected game take half the screen both in height and width, completely destroying the game cover aspect ratio.

The gridtile size is not taken into account the first time the grid is rendered, the tile will only be set to this size once it got unselected (and also have the same break-aspect-ratio problem than the other one.

A better implementation would be an option called something like selectedTileZoomRatio which take a positive real number between 0 and 2 which define by how much the tile size will be multiplicated when selected (1 being no change in size, something like 1.2 being the default value).

jrassa commented 7 years ago

Yes and my proposition is to calculate the tile size at runtime, which allow both the theme maker and the user, if you accept my proposition to allow him to change the grid number of columns, to easily control the size of the tiles.

Wouldn't it be be even easier to control the size of the tiles by actually controlling the size of the tiles? I understand that this wouldn't work well with giving users the ability to set the number of columns, but I am not sure that we should do that. Some themes will want tight control over the size and number of columns. I could see the nes-mini theme adopting this to create a grid with a single row to better mimic the interface of the actual nes mini.

Also, how will you determine the height of the tile? Calculating it based off of the height of the image is a flawed approach. Do you only look at the first image? What if that particular game doesn't have an image, would the height be based on the dimensions of the default image.

Here are a few samples showing the difference between the 2 approaches.

<!-- 4 columns, no margin -->
<imagegrid>
  <size>0.8 0.8</size> <!-- current hardcoded value, ideally should be configurable in theme -->
  <columns>4</columns>
  <margin>0</margin>
</imagegrid>

<imagegrid>
  <size>0.8 0.8</size> <!-- current hardcoded value, ideally should be configurable in theme -->
  <gridTileSize>0.2 0.2</gridTileSize>
</imagegrid>

<!-- 5 columns, no margin -->
<imagegrid>
  <size>0.8 0.8</size> <!-- current hardcoded value, ideally should be configurable in theme -->
  <columns>5</columns>
  <margin>0</margin>
</imagegrid>

<imagegrid>
  <size>0.8 0.8</size> <!-- current hardcoded value, ideally should be configurable in theme -->
  <gridTileSize>0.16 0.16</gridTileSize>
</imagegrid>

<!-- 4 columns, small margin -->
<imagegrid>
  <size>0.8 0.8</size> <!-- current hardcoded value, ideally should be configurable in theme -->
  <columns>4</columns>
  <margin>0.01</margin>
</imagegrid>

<imagegrid>
  <size>0.8 0.8</size> <!-- current hardcoded value, ideally should be configurable in theme -->
  <gridTileSize>0.1925 0.1925</gridTileSize>
</imagegrid>

Yes, but it is way easier for theme makers to setup the gridview to have let's say 4 columns globally, 3 columns for the n64, 5 for the megadrive, than search for games cover aspect ratio on google and use equations to calculate the tile width, and repeat all of this steps for all the systems he want to support.

As I said before, themes have no guarantee about what images a user has scrapped. Most themes do not make any assumptions about what type of image it is. That is why the maxSize option is typically used for displaying the images now. That will still be the case for grid view and as such most themes will likely take the approach and having a squarish shaped area for the grid tile, with the image displayed inside at the proper aspect ratio.

benjdero commented 7 years ago

I could see the nes-mini theme adopting this to create a grid with a single row to better mimic the interface of the actual nes mini.

This look like a really specific case to handle

Also, how will you determine the height of the tile? Calculating it based off of the height of the image is a flawed approach. Do you only look at the first image? What if that particular game doesn't have an image, would the height be based on the dimensions of the default image.

If I remember right (I can't look at the code right now), the grid analyze all the tiles to be displayed and search for the one with the biggest height or width, and strech all the images to that size. This is because all the images actually have different dimensions, if I take the megadrive example again, some image have a size of 600x700, some 600x703, some 582x700 ... the difference is small so it doesn't look squashed but look definitly better than just leaving it to the original size.

That will still be the case for grid view and as such most themes will likely take the approach and having a squarish shaped area for the grid tile, with the image displayed inside at the proper aspect ratio.

Ok I got the idea, you worked on the custom game collections so you want the gridview to be fully compatible with your work even if it render worse on classic game collections. I can understand that but maybe then we should do something that handle the 2 separate cases (when all the game covers have nearly the same aspect ratio, and when they don't). I have an idea for that but I can't show you right now I will take some time tonight when I will be at home.

pjft commented 7 years ago

Actually, that request does not come from the custom game collections angle, though it still stands. Arcade is an easy system to exemplify that with multiple aspect ratios for the screenshots/artwork - vertical, horizontal, etc.

The artwork will stay centered in that area for the most part. On Mon, 4 Sep 2017 at 08:01 Koerty notifications@github.com wrote:

I could see the nes-mini theme adopting this to create a grid with a single row to better mimic the interface of the actual nes mini.

This look like a really specific case to handle

Also, how will you determine the height of the tile? Calculating it based off of the height of the image is a flawed approach. Do you only look at the first image? What if that particular game doesn't have an image, would the height be based on the dimensions of the default image.

If I remember right (I can't look at the code right now), the grid analyze all the tiles to be displayed and search for the one with the biggest height or width, and strech all the images to that size. This is because all the images actually have different dimensions, if I take the megadrive example again, some image have a size of 600700, some 600703, some 582*700 ... the difference is small so it doesn't look squashed but look definitly better than just leaving it to the original size.

That will still be the case for grid view and as such most themes will likely take the approach and having a squarish shaped area for the grid tile, with the image displayed inside at the proper aspect ratio.

Ok I got the idea, you worked on the custom game collections so you want the gridview to be fully compatible with your work even if it render worse on classic game collections. I can understand that but maybe then we should do something than handle the 2 separate cases (when all the game covers have nearly the same aspect ratio, and when they don't). I have an idea for that but I can't show you right now I will take some time tonight when I will be at home.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/RetroPie/EmulationStation/issues/206#issuecomment-326881543, or mute the thread https://github.com/notifications/unsubscribe-auth/AVAV7YbPaRorQE-mXh3eWabdgi0Ub-8hks5se6A7gaJpZM4O1uzo .

benjdero commented 7 years ago

@pjft I don't know, looking back at all the requests and comments on this issue and the gridview's pull request, I don't think it will ever get merged.

zigurana commented 7 years ago

@Koerty, Hey man, come on now, you are doing a heck of a job here! You've brought the code and this discussion further than anyone before, and you should feel proud, not sad! I hope you realize that we are not trying to put you down, with our endless discussions and review comments, I am just trying to bring a mature implementation into the code base. I have the feeling that we might be working with different expectations on how long these things should take. I for one feel that we have covered a lot of ground in a very short period of time, especially compared to the rate of development not so very long ago (1.5 years ago). Unless large bugs pop up, we are in the final stretch of the game, the finish is in sight, please do not drop out now!

jrassa commented 7 years ago

@Koerty sorry to hear that you feel that way. If there wasn't a desire to see this get done, it would have be rejected. You have taken on a very large task and it requires a lot of scrutiny. My intention has only been to ensure that we get the best implementation possible.

benjdero commented 6 years ago

This is my grid theming syntax propositions. Maybe I missed one or two parameter since there is so many of them, but it doesn't hurt the general idea.

Condensed syntax, similar to the one used by the text list :

<view name="grid">
    <text name="md_title">
        <color>
        <backgroundColor>
        <alignement>
        <forceUppercase>
        <lineSpacing>
    </text>

    <imagegrid name="md_grid">
        <margin>
        <rowsAndColumns>
        <missingBoxArtPath>
        <folderImagePath>
        <gridTileTextColor>
        <gridTileTextBackgroundColor>
        <gridTileTextAlignement>
        <gridTileTextForceUppercase>
        <gridTileTextLineSpacing>
        <gridTileColor>
        <gridTileSize>
        <gridTileColorSelected>
        <gridTileSizeSelected>
        <gridTileTextColor>
        <gridTileTextColorSelected>
        <gridTileTextRow>
        <gridTileTextRowSelected>
        <gridTileBackgroundPath>
        <gridTileBackgroundColor>
        <gridTileBackgroundColorCenter>
        <gridTileBackgroundColorEdge>
        <gridTileBackgroundPadding>
        <gridTileBackgroundColorSelected>
        <gridTileBackgroundSizeSelected>
    </imagegrid>
</view>

Middle ground approach, halfway between current theming syntax and the condensed one from the text list :

<view name="grid">
    <!-- Game title configuration (the one displayed at the top of the screen) -->
    <text name="md_title">
        <color>
        <backgroundColor>
        <alignement>
        <forceUppercase>
        <lineSpacing>
    </text>

    <!-- Grid configuration -->
    <imagegrid name="md_grid">
        <size>
        <margin>
        <rowsAndColumns>
    </imagegrid>

    <!-- Grid unselected tiles configuration -->
    <image name="md_grid_tile">
        <color>
        <size>
        <backgroundPath>
        <backgroundColor>
        <backgroundColorCenter>
        <backgroundColorEdge>
        <backgroundPadding>
        <textColor>
        <textRowSize>
        <textAlignement>
        <textForceUppercase>
        <textLineSpacing>
    </image>

    <!-- Grid selected tile changes configuration
    (override md_grid_tile parameters when selected) -->
    <image name="md_grid_tile_selected">
        <color>
        <size>
        <backgroundColor>
        <backgroundSize>
        <textColor>
        <textRowSize>
    </image>

    <!-- Image used for games wihout cover -->
    <image name="missing_boxart">
        <path>
    </image>

    <!-- Image used for folders -->
    <image name="folder">
        <path>
    </image>
</view>

As soon as we agreed on a theming syntax I will start its implementation and update the documentation. Feel free to give me your opinion or suggestions.

benjdero commented 6 years ago

@jrassa I think we completely missed a point a few comments ago, the grid display games name (the one right under the tiles, not the selected game name displayed on top of the screen). This mean the margin can't be set to zero or a low value, or we should allow the theme to hide games name, which isn't really practical either for ergonomic reasons (in case the user miss some box arts in his game collection for example). We can't either display the game name on top of the tile in case it is missing a box art, since it would render badly when tiles are too smalls or the theme changed the default image for a more "colored" one.

capture d ecran de 2018-01-30 14-38-29

benjdero commented 6 years ago

@jrassa What do you think about my 2 previous comments ?

lilbud commented 6 years ago

I feel like having the ability to just not add certain elements if they are not needed should be used for all of ES. Because if your theme doesn't use something, you gotta hide it off screen.

EctoOne commented 6 years ago

@lilbud I believe that is already implemented. I noticed that when I was working on the Chicuelo theme. That only had elements in the xml that the theme used. When I made the code for the current version, I tried using that method and it worked. But it seems to be not fully supported yet. I got a message that in some case the rating stars popped up. So in the end I added a code block to move all unwanted elements off screen again.

benjdero commented 6 years ago

@lilbud @EctoOne If you talk about showing/hidding metadata (the game name displayed on top of the screen in the grid view being part of metadata) then we already discussed this on the related pull request with @zigurana and chose to use the same behavior as with detailed view : themers have to set metadata's position offscreen to hide them. If we decide to change this behavior for the detailed view and hide them by default, then I will update the grid view accordingly.

I'm not against discussing about this but I think this is a little bit offtopic, what I wanted to talk about is if we should either :

I think this is a different topic because it have very different issues, mostly ergonomic/readability issues. That's why I would like to have @jrassa 's point of view on this and the theming syntax so I can move forward.

EctoOne commented 6 years ago

@Koerty I personally would always say that the theme creator should be able to decide what he/she wants to show. If that is to complex to implement, I would prefer no names in the grid if md_title is available. Then I could at least freely choose the position of the names instead of being restricted to have it at fixed positions.

When it comes to the syntax, I would prefer the condensed code example which resembles the textlist look.

benjdero commented 6 years ago

@EctoOne This isn't complex to implement, but I think hidding games name in the grid is debatable. I don't know if that happen often, but if the user miss some games' box art then it will be a pain for him to navigate through the grid if names are hiddens.

Darknior commented 6 years ago

Yes for my part i will show it too ;)

EctoOne commented 6 years ago

Would it be possible to have the name inside the grid so that if an image exists it would be on top of the text? Kind of like the system carousel works, where the text appears if an logo is missing. Anyway, since grid views main purpose is to show images, I think that the user is also responsible to get/make all of them.

benjdero commented 6 years ago

Technically it shouldn't be hard to do that but this mean big changes to this part of the code. I don't know if this is the right solution either, that's why I would like to have some feedback from retropie's main developers.

EctoOne commented 6 years ago

It was just a suggestion. Another question tho, how does the text interact with the grid spacing? I assume that if we are forced to have text, the spacing between two rows depends on that and therefore they can't be very close. And has the text a fixed length? If yes, how long is it? If its based on the grid, it can be really short if someone wants a dense grid. If it's a fixed size, will the names overlapping each other when the grid is dense. Or does the text length determine the spacing then?

benjdero commented 6 years ago

Actually the text width is the same as the image width, you can see it on the screenshot about 10 comments ago.

benjdero commented 6 years ago

@jrassa What about an algorithm similar to what google image do ?

google_image

Pros :

Cons :

lilbud commented 6 years ago

I agree with you that it feels way too cluttered. Maybe do the grid with a max size option.

Like this: 0.40 0.40. The box art would stretch until either the height or width is 0.4, and then the rest scales proportionally.

benjdero commented 6 years ago

@lilbud That's what I did, as @jrassa suggested.

The problem with this is our screen aspect ratio is either 4:3, 16:9 or 16:10, so our tile will never be a square, but a rectangle with the same aspect ratio.

This lead to huge margins in most case, as you can see in the screenshots on the related pull request : https://github.com/RetroPie/EmulationStation/pull/396

So I'm searching for alternative solutions, since this one don't completely satisfy me.

jrassa commented 6 years ago

@Koerty I would suggest focusing on a standard grid layout for the time being.

Part of the problem with the screenshots you reference is that the images are of mixed sizes. Most are boxart with a squarish aspect ratio, while one is widescreen aspect ratio. The tile size you have set for now appears to be based on the single widescreen image which results in large margins between the mostly squarish boxart images. If the tile size was set to a more square ratio to match the boxart, then only the one widescreen image would have larger visible margin on the top and bottom, while maintaining a consistent slimmer margin on the sides.

One possible solution to mitigate the varying margins would be to support background color/image for the tiles. That way the background fills the whole tile space, maintaining a consistent visual margin, while the image within the tile can be sized appropriately within.

However, I think implementing something like this should be more of a long term goal to look into once the core grid view implementation is complete.

benjdero commented 6 years ago

@jrassa No, the tile size is not based on the single widescreen image, it is based on the screen size, like all the UI elements.

In this example :

That's why the tile size have a bigger width than height, because of my screen aspect ratio.

Using the same max tile size and formulas, on a screen with 4:3 ratio and size 1280x1024px, the tiles will have a size of 243x195px, which is "squarer" than on a 16:10 or 16:9 screen, but that's still not a square.

The only other solution I see, is to take the size of the smallest side of the rectangle and resize the biggest side to this size to make a square. If I take my first example, the tile size will not be 256x146px, but resized to 146x146px.

This give us a better looking grid, where the tile width/height ratio doesn't depend on the screen aspect ratio, but the number of columns will depend on the screen aspect ratio (or the number of rows, if vertical screens is used in some retropie configurations).

benjdero commented 6 years ago

One possible solution to mitigate the varying margins would be to support background color/image for the tiles. That way the background fills the whole tile space, maintaining a consistent visual margin, while the image within the tile can be sized appropriately within.

I did some experiments with that and I think I can affirm a background/selector sized to the image + some padding around it look way better than a background/selector sized to the tile max size.