search

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
853 stars 343 forks source link

Refactoring theme-element handling #132

Open zigurana opened 7 years ago

zigurana commented 7 years ago

FOR THE CURRENT STATUS OF THE PROPOSAL, SEE THIS DOCUMENT (For now, this is only a discussion on future work on GameList Views) Currently, the creation of theme-elements (game-lists, images, metadata-labels, text, video's) is hard-coded per view. That means that the three views (Basic, Detailed, Video) expect to find a fixed list of theme-elements in the theme, and these are instantiated, and updated at fixed locations.

This makes the theme-element- handling very brittle, and inflexible to use. A current workaround is for themers to include even unwanted elements in the theme, and simply shift them off-screen. The generation of new view types is also very inflexible, and require a lot of work.

In my perfect world, a proper theming system will allow themers to create new views by adding a new view node to their xml files, and adding the elements they would like to see there.

To make this possible I am preparing a significant refactoring / redesign of the current theme handling. I am considering implementing an abstract factory pattern to create this new framework. This will allow the theme to place 'orders' for theme elements, which will result in the GUIComponents to be displayed on the screen.

This will be a learning experience for me as well, and I would like to use this space as a discussion on the pro's and cons of different programming strategies. At this point I am not much interested in discussing new features, the first step is to get feature parity (i.e. being able to parse existing themes and output them correctly).

I am new to this level of redesigns, and this project will likely take a while, but I am confident we can make this work!

pjft commented 7 years ago

No, wait - we definitely need to add...

I joke. :)

This sounds like a mammoth endeavor, and certainly not one I'd have had significant formal experience in. I fully support starting with feature parity, and I'm happy to chime in on anything I can help with or contribute to, though I've usually deliberately stayed away of the theming aspects of ES as that's something that you and @jdrassa have pretty well covered so far!

If you feel it'd help, would you want to start a Google Document for the technical aspects of it, I'm more than happy to contribute to it (mostly in a reviewer capacity for the most part, as well as keeping us grounded to the best of my ability).

EDIT: Obviously, when you decide to get this started, no pressure!

Or if you have a different or better way to go about it, let me know.

Thanks for getting this started!

sokie commented 7 years ago

From a programming's point of view it should be easy to parse the xml tree and initialise dynamically the elements inside there, a problem would be attaching data to them. I'll explain myself:
Right now we use hardcoded values to define what metadata the element points to, like "md_lbl_publisher" ( hardcoded inside code) and "md_publisher" ( metadata value ). In my opinion labels should be created freely and as many as they like ( and they should include the text too, without hardcoding the text inside code ), and then elements could have an attribute that can point to a metadata field ( like metadata.publisher ).
If we put a first limit where elements can point ONLY to metadata values, it should be easy, but if themers want to access other values like system ( system.name ), core, ecc then it might get more hard.
Also another hard part is that i think it will be hard to implement this without breaking current themes. But again, i think this should be a breaking change for better flexibilty

zigurana commented 7 years ago

Hi @sokie , thanks for your insights, and thinking with me here.

For backwards-compatibility, or rather, to have any sensible info displayed in the themes at all, we will need some kind of lookup scheme that links the name of a theme element (i.e. md_lbl_publisher) to its value (which is currently hard-coded to be "publisher", its the theme element called md_publisher that is populated with the value from the game's metadata).

This would need to happen for only a finite number of names: those currently present, and the ones we can come up with later. Did you know we even currently miss a 'tag' for the selected-game-name? Its ridiculous. Anyways, what I envision:

<text name="md_genre">
    ...
    <set color position etc>
    ...
</text>

Would just populate with the value found in the metadata, which is the current behaviour. While this 

<text name="WhateverNameIChoose">
    <value>"Any text I like"</value>
</text>

Would also be shown, without the extra's stuff being there. Which we also should no longer need, once the Z-index changes are accepted.

Later (much later), we could step it up and implement something akin to a simple string parsing scheme, that allows the user to define text contents by setting the value like "This game is called %name%, and is made by %publisher%". But that is a pipe-dream, even for me. The same thing holds for adding info that is currently not really accessible for ES, such as the emulator core being used, someday maybe.

zigurana commented 7 years ago

@sokie You say:

from a programming point of view it should be easy to parse the xml tree and initialize dynamically the elements inside there.

Well, that is the part I am trying to get right here. Too many quick and dirty changes have made ES the state it is in today, and while it's not the worst I've seen, its also not the prettiest.

Now, I am not a C++ coder by training, but I am forced to do it professionally now anyways, and I'd like to implement this refactoring / redesign as a way to improve my skills in this domain (and also to improve ES theming, of course!). Looking at several sources, an abstract factory pattern seems to be well suited for this job. If you have any good strategies on how to implement an abstract factory pattern, please let me know!

jrassa commented 7 years ago

@zigurana The abstract factory pattern may be a little bit of overkill for what we need. I would hesitate to introduce all of the interfaces that would come with the abstract factory pattern. However, I think a factory class for creating the various GuiComponents is a good idea. In fact a rudimentary one already exists for the "extras" within ThemeData.

We probably would also need a way to indicate how particular elements should be treated during transitions. For example, in the GamelistViews, when scrolling, metadata elements fade in and out, but other elements such as the background, logo, gamelist and extras remain as is.

Maybe a good baby step towards implementing this would be to add some additional media metadata (i.e. fanart, wheel art, boxart ...) and work on just making their inclusion in the views dynamic.

pjft commented 7 years ago

+1 on this. It sounds like a manageable approach to start including such principles in the themes.

That being said, I believe a spec should be useful, even if for the transitions as @jdrassa mentions. Unless we plan on either specifying the transitions in the elements, or just get all of them to have the same transition, and change the rendering pipeline as well. On Thu, 18 May 2017 at 23:33, John Rassa notifications@github.com wrote:

@zigurana https://github.com/zigurana The abstract factory pattern may be a little bit of overkill for what we need. I would hesitate to introduce all of the interfaces that would come with the abstract factory pattern. However, I think a factory class for creating the various GuiComponents is a good idea. In fact a rudimentary one already exists for the "extras" within ThemeData.

We probably would also need a way to indicate how particular elements should be treated during transitions. For example, in the GamelistViews, when scrolling, metadata elements fade in and out, but other elements such as the background, logo, gamelist and extras remain as is.

Maybe a good baby step towards implementing this would be to add some additional media metadata (i.e. fanart, wheel art, boxart ...) and work on just making their inclusion in the views dynamic.

— You are receiving this because you commented.

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

zigurana commented 7 years ago

@jrassa I am not interested in adding new functionality at this point, I just want to get rid of the hardcoded mess that constitutes our current GameListViews (Basic, Detailed, Video). Maybe the abstract factory pattern is not neede, I will have a look at how the extra's are currently handled, and see if that needs to be extended. Re: stransitions etc, I think there is a lot of business logic captured in the current implementation, and this could/should be implemented as default behaviour in the future. @pjft: I had started with an UML-esque diagram for the classes, showing the current implementation, and what I wanted to go towards, would that be helpful in lieu of a proper spec? It's not finished yet, but I can have another go at it, if it helps the discussion here.

sokie commented 7 years ago

@zigurana @jrassa I think we already have parts of an abstract factory pattern ( views having a common abstract interface -> gui components having a common abstract interface ). So the only left step is implementing a factory. I took a look at how extras are handled and i agree it's a good starting point, if we move that out in a factory and make it generic it could work. Other than the fading as @jrassa mentioned I see a few other problems that need taking care of:

zigurana commented 7 years ago

@sokie Yeah, I was hoping that we had many of the necessary elements already in place. I have no clue yet what those two lines are meant to accomplish, but will have a look later.

This means that all views CAN have a video element, but this will make it easier in my opinion ( this means that themes will have ultimate power on what is shown ). Opinions?

I think this is exactly the way it should be. It should be up to the theme to define what elements should be included in a view, not the other way around. In the end, I'd like to see people being able to define arbitrary views (maybe also set a default filter value) and have a specific view for games of a specific publisher, or in a specific genre.

zigurana commented 7 years ago

And I must say, I really appreciate the amount of brainwork you are putting into this discussion! Thank you guys! 👍

pjft commented 7 years ago

I think this is the way to go, agreed.

In terms of specs, I'm not as much a stickler for formality, but given that the 3 of you clearly have strong views over this realm, and have different and complementary perspectives at the moment, it was just to have something that would make sure that you always have a common ground and a shared understanding of the approach we're trying to follow. That was what I was going for.

One thing to bear in mind - or one thing I'd like us to consider - is retro-compatibility. I know it's easy to dismiss it, but I need to bring it up. There's a plethora of users and theme out there.

It's not realistic to expect all theme creators to upgrade their themes. Also, as users install the new version, if it's not retro-compatible, it will just be broken immediately after installation, and they then need to reinstall things. Expecting users and theme makers to adopt a new theme format and structure "just because we have implemented" is also a challenging proposition.

From what I'm reading, it may (?) be tricky to ensure retro-compatibility at a parser/class level (I assume?), so I'd like to hear your thoughts as to:

I have a few in mind - anything from building into the parser a "translation" layer that would upgrade older themes as it reads them, in runtime, to a separate script to upgrade and store themes in the new version. Or even joining the two - in ES, asking the user if we want to save the theme in the new format (backing up the old one, just in case we mess something up).

My 2 cents.

If older themes will work, though, then don't mind this comment at all.

pjft commented 7 years ago

Oh, and also, in terms of specs, the goal is also for us to be able to understand the clear boundaries we are establishing, and what are we effectively choosing to generalize and how.

For instance: we will allow (apparently) a view with several videos, for instance. That's... great, I suppose. Will we abstract the views as well - meaning that the carousel will in the end be just a set of views "horizontally" linked (think an array of views), and the gamelist view is just another view, "vertically" linked to the master carousel view? Would we, conceptually, allow for multiple carousel levels, should the theme maker want to?

Would the following example of a theme structure be valid? Would we want it to be valid?

{
  { arcade, <--- leads to sub carousel
      { 
        { capcom, <--- leads to sub carousel
           { cps1games, cps2games, cps3games }.
        data east games <---- flat, lists games
      }
   },
   snes <--- flat gamelist, at the same level as the arcade entry
}

I can see advantages and disadvantages in all, so bringing this up just so we know where we're drawing the line and why. I'm all up for generalization, but only up until where it is effectively practical and meaningful, otherwise we'll end up with just generic classes all over that nobody can easily use :)

sokie commented 7 years ago

I would like input from others as well, but for me:

zigurana commented 7 years ago

@pjft I believe that backwards compatibility for older themes should definitely be possible, and should be a firm requirement. I personally do not believe in the use for backwards compatibility for 'new themes on older versions of ES', as I think users should just upgrade their ES.

zigurana commented 7 years ago

Will we abstract the other views as well

As you probably noticed by now, I like to sketch out 5 yr horizons a lot, so what I'd like to see at some point is a combined system/gamelist view. Maybe a vertical carousel with a grid of games next to it?

Anyways, let us limit the discussion for now to separated system and gamelists only. I do think that the system-list should use the same factory approach to populate the view with GuiComponents.

pjft commented 7 years ago

@zigurana @sokie Great. Fully agreed.

It's just so these are well-defined from the get go, otherwise - you know, scope creep :)

And yes, I was referring to "new-ES-still-loading-older-themes", definitely not "older-ES-loading-new-themes".

Thank you.

zigurana commented 7 years ago

I've created this document, please have a look and add anything you find missing.

jrassa commented 7 years ago

@zigurana I couldn't edit the document so I am adding some thoughts here for now.

When I mentioned that I thought the Abstract Factory Pattern was overkill, my concern was that we would be introducing interfaces for each GuiComponent type (i.e. ITextComponent, IImageComponent, ...). Since we are unlikely to have multiple implementations, there is nothing to abstract.

With regard to the text labels, I think it is important that the text content still be defined somewhere within the code vs only in the theme. That will allow for the potential of supporting multiple languages down the road. However, theme makers should have the option to override the text if they wish.

Another approach to consider to maintain backwards compatibility, at least during development, would be to implement this as an additional view that can be explicitly enabled, while leaving the existing views as is. Then once we determine that it is functionally complete, the ViewController can be changed to use the new view vs. the old views.

zigurana commented 7 years ago

@jrassa, @pjft, @sokie I thought it would be nice to report back with some of the things Ive been doing on this topic. It would be nice to get some feedback on my approach and general strategy taken. I've pushed the changes to my repo here. Please have a look if you can spare the time.

And oh yeah, I'm stuck :-(

In short the changes I've made so far:

The issue

is with the game- description text component, called 'md_description' in the theme and the ScrollableContainerComponent that, contains it, and governs the scrolling animations.

In DetailedGameListView these two GUI components are created (hardcoded), but in the future situation we would like to create them 'on demand', so once the makeThemeComponent() function encounters a text-element called "md_description", it should create two GuiComponents, and make the one a child of the other. Implementing this is where things get ugly.

The output type for makeThemeComponent() is a mapping of strings (the keys) with GuiComponents. This collection is then later iterated through when applying themes, and adding as children for rendering and such. As a result, I can add only one GuiComponent per theme element (because the keys need to be unique). But this description box needs two! So a workaround is to create a new fake key, but that hampers the applyTheme() functionality, because that needs to get a proper name to be able to find the right info.

I've tried some things to smudge this over, but this generally feels like to wrong way to go, any suggestions?

Another way to go could be to extend the TextComponent to scroll, or to create a new hybrid type that does both?

Other Open Items

Please see the many "TODO" comments still in the code for other jobs still on my list. For instance, I am now making GuiComponents, because it seemed the logical thing to do, but how to handle HelpStyles and sound effects? Those things are not a GuiComponent, should they be?

Ok, I've had enough for now, my eyes hurt, and my brain is mushy. Good night!

pjft commented 7 years ago

Oh wow. Great to see this moving forward! :)

I'll reply further in the morning on my laptop and after reading the code, but at first glance my thought on the problem was that yes, creating a "hybrid component" as you call it could work. I know in some languages we can have multiple inheritance, I don't recall much about c++ in that regard. Alternatively you could create a new class for Animated GUI Component, that would inherit from GUI Component but handle animations. I don't know enough about that code, but in principle it could be an option.

As for help and sound effects, I wouldn't be shocked if they were an Audio Component and a Help Component (does it already exist?) but semantically they feel different to one another. While the help elements would easily be GUI Components, the audio is probably more of an attribute of... Something. Scrolling audio would be an attribute/property of the list component, and probably the launch audio would be a property of the system itself.

I'm not sure if this is sound advice as I don't know enough of the theme structure and architecture, but at first brush these would be my thoughts.

I'll look into it tomorrow!

Thanks and well done :)

On Thu, 15 Jun 2017 at 21:53, D. Polders notifications@github.com wrote:

@jrassa https://github.com/jrassa, @pjft https://github.com/pjft, @sokie https://github.com/sokie I thought it would be nice to report back with some of the things Ive been doing on this topic. It would be nice to get some feedback on my approach and general strategy taken. I've pushed the changes to my repo here https://github.com/zigurana/EmulationStation/tree/ThemingRefactoring. Please have a look if you can spare the time.

And oh yeah, I'm stuck :-( In short the changes I've made so far:

  • Introduced a new file GameListView.cpp which (hopefully, in time) will replace BasicGameListView.cpp and DetailedGameListView.cpp, and all the others .
  • Extended ThemeData.cpp to parse a theme, and create GuiComponents, one for each team element found (see the function makeThemeComponent() ).
  • Cleanup and minor changes in IGameListView, Basic, Detailed, Video, and Grid GLVs.
  • Started with proper CarouselComponents, but ignore those for now.

The issue

is with the game- description text component, called 'md_description' in the theme and the ScrollableContainerComponent that, contains it, and governs the scrolling animations.

In DetailedGameListView these two GUI components are created (hardcoded), but in the future situation we would like to create them 'on demand', so once the makeThemeComponent() function encounters a text-element called "md_description", it should create two GuiComponents, and make the one a child of the other. Implementing this is where things get ugly.

The output type for makeThemeComponent() https://github.com/zigurana/EmulationStation/blob/ThemingRefactoring/es-core/src/ThemeData.cpp#L483 is a mapping of strings (the keys) with GuiComponents. This collection is then later iterated through when applying themes, and adding as children for rendering and such. As a result, I can add only one GuiComponent per theme element (because the keys need to be unique). But this description box needs two! So a workaround is to create a new fake key, but that hampers the applyTheme() functionality, because that needs to get a proper name to be able to find the right info.

I've tried some things to smudge this over, but this generally feels like to wrong way to go, any suggestions?

Another way to go could be to extend the TextComponent to scroll, or to create a new hybrid type that does both? Other Open Items

Please see the many "TODO" comments still in the code for other jobs still on my list. For instance, I am now making GuiComponents, because it seemed the logical thing to do, but how to handle HelpStyles and sound effects? Those things are not a GuiComponent, should they be?

Ok, I've had enough for now, my eyes hurt, and my brain is mushy. Good night!

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/RetroPie/EmulationStation/issues/132#issuecomment-308862806, or mute the thread https://github.com/notifications/unsubscribe-auth/AVAV7aDpz2V-54OnkRvnK2_P7-ClmcGBks5sEZnfgaJpZM4Nd3hX .

Darknior commented 7 years ago

So cool to read this topic :) Many good ideas here ... and for the big part i'm agree with @sokie ;) For my part i search a solution to have 3 more media to show in my skin please 3 images for Boxfront Boxback and maybe Boxcase ? https://retropie.org.uk/forum/topic/10654/es-add-some-news-artworks-to-skin-like-in-a-vg-museum/6

zigurana commented 7 years ago

@Darknior , please use the forum or the issue tracker to request new features, this is outside the current discussion.

zigurana commented 7 years ago

So I've tried to implement a new class called ScrollableTextComponent, and learned something along the way about virtual inheritance no less! scrollabletextwip So now the scrollable text box renders correctly (lower right), except that there is some unexpected spillage over the textlist component. Progress at least!

pjft commented 7 years ago

Well done :)

Now, are you sure there's some spillage? What it seems to me is that you're creating a second element at position 0,0 as the text seems to be the full description text, not spillage from the one you have set up down there.

Obviously I'm not seeing it moving, but at first glance I'd check if there aren't two objects - or if, for some reason, you're not assigning it to an unintended element as well.

On Fri, Jun 16, 2017 at 1:18 PM, D. Polders notifications@github.com wrote:

So I've tried to implement a new class called ScrollableTextComponent, and learned something along the way about virtual inheritance no less! [image: scrollabletextwip] https://user-images.githubusercontent.com/6103768/27226250-5929ad8c-529e-11e7-94b4-9f41920b9273.png So now the scrollable text box renders correctly (lower right), except that there is some unexpected spillage over the textlist component. Progress at least!

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

zigurana commented 7 years ago

Yep, it was rendered twice, once from the container, and once from the new hybrid component. Now onwards to sound handling!

Op vr 16 jun. 2017 14:23 schreef pjft notifications@github.com:

Well done :)

Now, are you sure there's some spillage? What it seems to me is that you're creating a second element at position 0,0 as the text seems to be the full description text, not spillage from the one you have set up down there.

Obviously I'm not seeing it moving, but at first glance I'd check if there aren't two objects - or if, for some reason, you're not assigning it to an unintended element as well.

  • Paulo Tavares

On Fri, Jun 16, 2017 at 1:18 PM, D. Polders notifications@github.com wrote:

So I've tried to implement a new class called ScrollableTextComponent, and learned something along the way about virtual inheritance no less! [image: scrollabletextwip] < https://user-images.githubusercontent.com/6103768/27226250-5929ad8c-529e-11e7-94b4-9f41920b9273.png

So now the scrollable text box renders correctly (lower right), except that there is some unexpected spillage over the textlist component. Progress at least!

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

.

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

zigurana commented 7 years ago

Ok, I've updated my dev branch.

By no means done, but it's good enough to give a a first test-drive / look over. Please note that I've currently disabled all of the legacy GLV's if you want to have them back, look at the Viewcontroller.

Known issues:

zigurana commented 7 years ago

https://retropie.org.uk/forum/topic/11122/testers-wanted-gamelistview-theming-refactoring/1