News:

The Forum Rules and Guidelines
Our forum has Rules and Guidelines. Please, be kind and read them ;).

[Project] GUI Theme

Started by Max-Max, May 31, 2013, 11:12:48 PM

Previous topic - Next topic

0 Members and 2 Guests are viewing this topic.

Markohs

#490
My suggestion is:

- As I allways stated, big patches are preferred imho, it gives you the chance to code freely, each patch should be of significance, and be:

* Significant: Should implement important new aspects of the game, and have a reflection in the user, our players have to be able to see "something new". If it's not visible to the user, and it's just re-design, it has to be of relevance, not minor tweaks.
* Complete: It's suposed to leave the game in a consistent state, no parts of the game should be incomplete or buggy, no excuses here. If a change has to be done in this patch, it has to be completed fully, not leave parts for future patches. If this forces you to write code just for this patch that will disappear later it doesn't matter, you have to do it anyway.
* Consolidated: As it's incorporated to the game trunk, it will end being in the nightlies. From that precise moment, bug reports need to be fixed, asap, in live code. Not fixed in the patch, even if that part of the code has been rewritten again in the patch, or the bug makes no sense in the future patch, the bug has to be solved in live code.

- To avoid a posted patch to be "rejected" or in a "to be rewritten" state, like prissi is doing with your patch, I suggest you posting *frequent updates* of your patch for review. prissi and the rest of coders will *suggest*, or *strongly suggest* you to change aspects of the patch. The discussion has to be constructive *by both parts*, but *at the end, it's YOUR responsability* to update the patch, in your computer. Once the patch is completed, it will be sent to trunk, *as you posted*, as you and the rest of developers agreed it to be. Minor modifications can be at place, ofc, but have to be of very low relevance, not significant changes.
- The responsability to keep your patch up-to-date with the trunk is yours too, you need to do it. You are ofc allowed to ask to pospone a change in trunk, it it's going to cause huge troubles to you, but with a valid argument.

Well, what do you all think? I tried to express some procedure that protects Max-Max and future patchers enough, while allowing the "senior" programmers enough control of the process. But ofc, we have to mind many things already said. All conversations need to stay contructive, people have to mind no member of the community wants to be rude with anybody, and we all respect each other. We also have to mind everybody has somewhat different coding styles, we all should be in the middle, and flexible, but writing quality code. This is not personal, we are just open source programmers, collaborating. :)

We should, as open source programmers, be positive about changes, and see them as an opportunity. But we are not low quality programmers, and we are supposed to identify when a change is needed and wanted, because it gives to us something worth it, because all additions brings us new bugs, and we'll have to keep maintenance in that new code, forever. That new code has to be worth it, and we need to realize when it's time to throw something to the trash, and start again.

Max-Max

Hmm, It all sounds great but every one must sign up to this workflow and as you say respect that we have some different ways of solving problems.

Prissi's addition of one skin_besch_t* skinverwaltung_t::xxxxx for each GUI control completely interferes with my development road. It took me quite some time to restore my code after this addition and now Prissi continues to develop down this path, ignoring that I have a complete different solution in the pipe that already is half implemented and the base for coming theme operations.

In this latest "reworked" patch from Prissi he has also dissected my theme classes further and moved around more code. With these new changes and the one already in trunk makes it very hard for me to keep my code in synch.

I which we could roll back any theme related code to the state it was before Prissi added the theme selector dialogue into the trunk. As it is now, my code is dissected so much that It will take me quite some time to restore it again and my guess is that Prissi will continue his path meanwhile...  :-[

I don't know how these large project are managed, but I take it as the person starting the project is the "project manager" and if people wants to help, at least synch with the project manager, or ask what they can do to help instead of just rewriting code and running their own race in parallel.

How do we proceed from here? I can try to restore my code once again with the current trunk, but it isn't any point if Prissi is going to develop the same things in parallel with me...
- My code doesn't have bugs. It develops random features...

Markohs

I guess we are at a stalled point there, I can't do more on this subject.

That's something you and prissi need to solve. :)

But to reach an agreement, allways both parts need to yield something. Both. :)

kierongreen

My honest advice Max Max is to find another area of simutrans to work on. Whatever your vision was of themes is unlikely to be what finally gets implemented. Keeping on developing to reach your original objectives will only be frustrating.

My advice to people submitting patches is similar to Markohs - by all means have other people look at the code and make comments but don't try submitting to trunk until it's finished. At that point accept there may be some changes made (but hopefully not if there's been good dialogue between patcher and dev team).

Hopefully we can all learn from this and avoid something like this happening again. Only rewrite a patch once author is wanting to submit it to trunk (or has abandoned it of course).

prissi

Actually, I did not touch much of your code when submitting the themeselector changes and the other changes to dialogues. I mostly changed the header, and that is it. The olnly other thing is I did not commit the merged alignment parameters, since this obscures too much. I did not touch and of theme files, so there should no changes needed.

My advice is to focus on one thing at a time. I strongly disagree with Markohs, most large patches never make it. But then that is his style. For me, large patches are take it or leave it. So if I or somebody else is not happy it will not be committed until you change what you may not like, and you feel there is lot of work wasted (as there is lot of work wasted with patch review). This does not work out.

If you want to continue on GUI improvement, here my suggestion for smaller patches:

Make a patch for makeobj to cut stuff into imagelists. This can easily survive whatever you do later on the simutrans side.
Make a patch each dialogue at a time.
Make a patch to use the current skinverwaltung_t with you themes (because cutting images in simutrans will not be the way.)
Make a button patch.
Make a patch in simwin.cc to have gui_frame_t the object handled by simwin and not the win struct, i.e. add magics, gadget etc. into gui_frame_t and let it also draw its titleback itselve. This will obviously also move some event haqndling to gui_frame_t, which is probably a good think, since it shringkes simwin and increase the still small gui_frame_t.

All those would be fairly independent from each other.

Dwachs

As bottom line, an updated version of your patch would be much appreciated.

I agree with prissi that the code to cut images along lines with a particular color is interesting in its own right and should be implemented as part of makeobj (image_writer and friends) to be available for all the graphic authors.

I agree with Max-Max that the images and so on should be handled by a separate theme class. (independent of or superseding skinverwaltung).

I strongly disagree with Kieron on just giving up: this would be a great waste of time and existing work.
Parsley, sage, rosemary, and maggikraut.

Max-Max

Okay, my suggestion is that we halt all the patches for now, I need to clean up this somehow.

I have a questions although:

Why do we need skin_besch_t* skinverwaltung_t::xxxxx other than at loading?
What I have seen so far all display_img_xxxx routines are using the image ID to draw an image. All skin_besch_t* skinverwaltung_t::xxxxx is used for, after loading, is to retrieve this number.
The theme manager groups and holds these image ID's for each theme element and the use of skin_besch_t* skinverwaltung_t::xxxxx is obsolete after loading.
To split skin_besch_t* skinverwaltung_t::xxxxx into one list for each Theme element doesn't really do any difference other than we need to add a new skin_besch_t* skinverwaltung_t::xxxxx every time we add a new theme element. What I can see from this is that all it does is to create more work when we continue to add more theme elements.

What I can see here is that skin_besch_t* skinverwaltung_t::xxxxx can be removed for theme elements and have the theme-pak reader directly in the theme manager, to directly register the images and group the image IDs.
If it is preferable to have the reader in a separate class, it can interact directly with the theme manager instead of using the skin_besch_t* skinverwaltung_t::xxxxx.

Another alternative would be to make the theme related skin_besch_t* skinverwaltung_t::xxxxx dynamic so it can be deallocated after PAK load.
- My code doesn't have bugs. It develops random features...

Dwachs

I my point of view, the purpose of skinverwaltung is exactly what you described: have a class that holds all the pointers to different skin-besch things, which are essentially a list of images.

Deallocating these kind of things is another payload of work: all the besch-classes lack proper destructors.

Adding an new skin_besch_t*  member to skinverwaltung seems to be still less work than to implement a complete pair of reader / writer for theme-paks.
Parsley, sage, rosemary, and maggikraut.

Max-Max

Quote from: Dwachs on October 23, 2013, 05:25:08 PM
Adding an new skin_besch_t*  member to skinverwaltung seems to be still less work than to implement a complete pair of reader / writer for theme-paks.
This is what I have now. At first Prissi added one new skin_besch_t* member called theme, this is the one I'm using now. However, later Prissi added a new skin_besch_t* member for each GUI theme element. Imho it creates more work when we add more GUI theme elements. The skin indexes becomes local for each GUI theme element, where an index number means different things depending on theme element. When referring to a theme element, it isn't enough to use one index, but also the right skin_besh_t* member.

I would like to continue to use one skin_besch_t* member for all theme elements because we can treat all gui theme elements as one, not have to know what theme element we are dealing with.
- My code doesn't have bugs. It develops random features...

Dwachs

Quote from: Max-Max on October 23, 2013, 06:07:38 PM
I would like to continue to use one skin_besch_t* member for all theme elements because we can treat all gui theme elements as one, not have to know what theme element we are dealing with.
You have the choice: either have a lot of named skin_besch_t* members or an array of skin_besch_t* with named indices. Seems like the last method is more comfortable: one pak file and one besch to rule them all.
Parsley, sage, rosemary, and maggikraut.

prissi

#500
I think a list with all stuff in the same object is a monster. With the proposed 9x9 buttons a normal button has 27 (on off disabled) and a color button 9x4=36 images. Add gadgets and arrow buttons, posbuttons, checkboxes, and scrollbars and you have a obj=menu with 93 predefined images. How big a list do you want?

Or you can have one
obj=skin
name=posbutton
with three images ...

Apart from the cleaner approach to find a check box image under skinverwaltung_t::check_button and not under skinverwaltung_t::theme number 54 to 57. If we get rid of the old system that did exactly this, I want to improve the format.

The implementation now has all different objects in one pak, since makeobj can merge paks together (and can also unmerge them easily). That way even a user can exchange certain images, which is also not possible with a monolytic pak.

Furthermore, apart from the ground tile code simutrans internally did not used the bild_t members. Those were just added quite late to move the blending of images into the climate ground code. All other code is geared for the image numbers.

For this reason I am not fond of a single skin, and much less of a new mechanism for skins. Keep it simple!

As it goes, either me or Max-Max will waste work, as my way of implemetation is different from his. I.e. I did reuse the skinverwaltung_t, and just added three functions to display_* to display stretched image and ellipse text (the prior was also invented of by Max-Max).

Anyway, updated patch is attatched, as well as a standardpak (EDIT: now with the paks) THis shoudl take care of the nine image buttons and should be fully scaleable. Just add gui_button_height=20 will give you larger buttons.)

Max-Max

Quote from: prissi on October 23, 2013, 08:42:45 PM
I think a list with all stuff in the same object is a monster. With the proposed 9x9 buttons a normal button has 27 (on off disabled) and a color button 9x4=36 images. Add gadgets and arrow buttons, posbuttons, checkboxes, and scrollbars and you have a obj=menu with 93 predefined images. How big a list do you want?
I'm more and more convinced to put the cutting in the them manager.

1)
The artist only needs to supply one image per GUI theme and state. This can be one image with guard lines (cutting lines) or just a plain image if the default cutting is enough. The default cutting works fine for irregular shapes as long the irregularities are within the auto cutting.

2)
By letting the theme manager do the cutting, we only need one image per element and state, not 3x3xn states. If the artist doesn't paint in cutting lines, it can still be configured to be cut as a 1x3, 3x1 or 3x3. The advantage of this is that the Artist can for example configure it to only be horizontally resized, but the user can override this and have it both horizontal and vertical (maybe he needs it for touch screen). This also speaks for the theme manager to do the cutting.

Quote from: prissi on October 23, 2013, 08:42:45 PM
Or you can have one
obj=skin
name=posbutton
with three images ...
...or as I explained only one image. But 2 or 3 are also possible, everything for the artist to chose.

Quote from: prissi on October 23, 2013, 08:42:45 PM
Apart from the cleaner approach to find a check box image under skinverwaltung_t::check_button and not under skinverwaltung_t::theme number 54 to 57. If we get rid of the old system that did exactly this, I want to improve the format.
The whole point with the theme manager is that a GUI control should not know what images to use, it tells the theme manager to draw a specific theme. The manager created an internal list, grouping the image ID (not the skinverwaltung_t index, since they are not used at all after PAK load). The theme manager use the image ID from the big images[] list directly in the calls to display_img_xxxx

Quote from: prissi on October 23, 2013, 08:42:45 PM
The implementation now has all different objects in one pak, since makeobj can merge paks together (and can also unmerge them easily). That way even a user can exchange certain images, which is also not possible with a monolytic pak.
It sounds like a good idea, but splitting and Merging PAK, isn't that a typical makeobj functionality that could be developed further to extract and repack images from another PAK. I thinkn this should be an extended developent of the makeobj, not the theme system, so it can be used with any kind of images in a PAK.

Quote from: prissi on October 23, 2013, 08:42:45 PM
Furthermore, apart from the ground tile code simutrans internally did not used the bild_t members. Those were just added quite late to move the blending of images into the climate ground code. All other code is geared for the image numbers.
The theme manager is using the image numbers (image ID) and the already in place display_img_xxxx functions, so yes, the theme manager is working with image numbers already, it's al in my original code.

Quote from: prissi on October 23, 2013, 08:42:45 PM
For this reason I am not fond of a single skin, and much less of a new mechanism for skins. Keep it simple!
If you refer to the old one-image-all-controls, you can't have opened the theme source I posted together with the patch. All controls have their own bitmaps, they don't need to be in one single image, neither did they need to do this in the past either. People for some reason just put all of it into one image. This also makes it possible to share single controls and their graphics. It's all in there...
Your implementation of one skin_besch_t* member for each theme element makes all previous skin-PAK and formats obsolete and needs to be redone. My patch use the current skin PAK format, nothing has changed! Everything is exactly as before, you can even use your old skin files and PAK if you like.

Quote from: prissi on October 23, 2013, 08:42:45 PM
As it goes, either me or Max-Max will waste work, as my way of implemetation is different from his. I.e. I did reuse the skinverwaltung_t, and just added three functions to display_* to display stretched image and ellipse text (the prior was also invented of by Max-Max).
Well, my implementation is object oriented and encapsulate the skin drawing from the GUI control. The GUI control doesn't know what images are used or what rule has been applied by the artist. The theme manager can reroute themes to use the same single set of images, a technicality the GUI control doesn't need to know.
The theme manager also use stack oriented clip rectangles, meaning clipping can be nested over several function calls always clipped to the previous rectangle to prevent children from drawing outside their parent's client area.

I'm now even more convinced that the theme manager should do the cutting, to use one skin_besch_t* member for all theme images (because it is only used at PAK load and never again) and keep track of the image numbers directly for drawing. All this contributes to a simpler way to define the graphic, more freedom to the artist and continue building on the already used system in Simutrans (image ID).
- My code doesn't have bugs. It develops random features...

prissi

You are convinced to cut the images in Simutrans, Me and also Dwachs adviced agaist. Either adhere to this advice, or we can only agree that we disagree.

I cut the images in makeobj, and it took me less than 10 minutes compared to the time it took me to draw them it is nothing. With color lines it would be even easier to cut, but ok, cutting by hand works fine too.

The GUI elements are object oriented, the drawing is not. At a certain points there will be a break, since the OS is not object oriented (apart for Zeta/BeOS). In simutrans all objects themselves call display_xxx functions to do their drawing is a routine called display or zeichnen. Doing different in the GUI from all other objects in the game will add to confusion.

I also fail to see how adding another layer will make things more simple: I compare

display_stretch_image( gui_theme_t::imagename, place )

to

theme_manager->display_element( GUI_MAGIC_NUMBER, place );

One magic parameter to get the image information, and one location. Only that the second code jumps through some hoops to finally call the first function anyway.

And stacking clipping works quite well, otherwise the objects on scrollpanes would not clip. They exactly clip their children. But maybe I misunderstood again.

The only critic on my patch so far from Markohs was using too long constants which I shortened. Well I fixed this. I am happy to here some other critice apart from "Well that is boring like the rest" It is ment to be like the rest.

But this is going in circles now for so long now. It would nice if there is a decision, since otherwise Max-Max and me are not getting happier.

Max-Max

#503
I think we need to break down this in small steps because I'm not sure you have understood the purpose of the theme manager or I'm not understanding what you try to replace it with. I think I have explained this many times before and all I can do is to try one more time.

What is a theme element?
A theme element is is the graphical visualisation of a whole GUI control or a part of it. Several GUI controls can share the same graphical theme elements. One example of this might be all GUI controls with a frame. A button use the frame as a background and lays text on top of it. A group box can use the same frame and put text in the upper corner. The window has a frame defining its borders. An edit control has a frame, listbox, combobox etc... So the frame in this example is an element of a control's graphical visualization. Another theme element can for example be a symbol drawn inside a button. The basic idea is to let all GUI controls have their own theme images, but many times they can share the same images and in this case doesn't need to be specified at all.

The theme element itself can be mad up of 1 to 9 images for frames, or even more if it is a more specialized GUI element. All these images doesn't need to be specified or cut by the artist, nor does the GUI control itself need to know what images to use or how many they are.

Where does the skin enum fit into all this?
The skin enum identifies the individual graphical images in the PAK file. In the end there should be enough to only define one image per control and state, meaning less SKIN_XXXX enums than now. A theme element can be an aggregation of several skin images but identifies an element by one THEME_ID_XXXX enum as a whole, regardless of how many images it was composed of. The internally cut images doesn't need any enum because they are created and managed by the theme manager and doesn't need an enum for each little individual image.

What is an element rule?
A button with a rectangular theme element can be sized both horizontal and vertical to fill the bounding box and this works fine as long the theme element is made for this. But as you said yourself in the artist theme forum, it doesn't work for rounded buttons. To solve the fact that some design can handle both horizontal and vertical and some can't, the artist/user can if he wish, apply a rule that tells how the theme element is drawn. At this moment there are 4 rules available: Horizontal, Vertical, Symbol and Frame (both Horizontal & Vertical).

The Horizontal rule
Even if the GUI control change in both directions (vert and horiz), it will centre the element vertically and only size it horizontally left-right in the control's bounding box.

The Vertical rule
Even if the GUI control change in both directions (vert and horiz), it will centre the element horizontally and size it vertically top-bottom in the control's bounding box.

The Frame rule
When the GUI control change in both directions (vert and horiz), it will size it both horizontally and vertically to the control's bounding box.

The Symbol rule
Even if the GUI control change in both directions (vert and horiz), the element will not change in size at all, instead it is centred both horizontally and vertically in the controls bounding box. This will for example be used in gadgets and toolbars.

Further down the road I have an idea of be able to add different resolutions of the same symbol and the theme manager will pick the one best suitable for the requested bounding box size. Before you star to tell me how ridiculous this is, this is only a brain storm thought to keep high quality on both low and hires screens. I believe you can compare it to mipmaps and windows has been using this technique for decades in their icons. I will also make it clear that this wouldn't be mandatory, it is up to the artist!

So the GUI control is still drawing itself in the ziechnen() function but use the theme manager as a service to draw the elements it is built up by. It still draws the text and other non-theme related details. It can even layer several theme elements. For example a tool button first draws the theme for tool buttons and on top of it the theme symbol. The symbol will automatically fit in the button and even increase resolution if supported and needed. All this without the GUI control's know how.
Because the artist/user can specify one of the above 4 drawing rules for a GUI control, it doesn't know what images to use and how to use them, it always draw itself the same way regardless of this. This is where the theme_element_t base class comes in. This is the base class for all the 4 rules and are created when the theme PAK loads.



This is somewhat how the GUI control and theme manager cooperate:



The actual drawing is done in the derived element class' virtual function display(). By this approach, the GUI control itself can call the same function regardless of what rules has been applied.

I have several times also stated that none of these patches has been the full theme system, it is still in development and more things will come. I did the huge mistake to believe that I could submit work in progress. I know better now and first we need to clean up the mess that this has become to be, before I can carry on and then submit a completed theme patch.


Quote from: prissi on October 24, 2013, 09:42:56 AM
You are convinced to cut the images in Simutrans, Me and also Dwachs adviced agaist. Either adhere to this advice, or we can only agree that we disagree.
I cut the images in makeobj, and it took me less than 10 minutes compared to the time it took me to draw them it is nothing. With color lines it would be even easier to cut, but ok, cutting by hand works fine too.
Cutting all images in makeobj you still end up with a all the magic numbers you thought needed to be specified from the beginning (27,36 and 93) to find these images in the PAK file. By cutting them in the manager you only need one image and no more magic numbers because the manager knows best where they are located. If the User wants to use another rule the manager can cut them different or prepare them in another dynamic way at load. Sure this can be done if they are pre-cut, but first the image needs to be reassembled (well for images with cut lines this is needed anyway)...

Quote from: prissi on October 24, 2013, 09:42:56 AM
The GUI elements are object oriented, the drawing is not. At a certain points there will be a break, since the OS is not object oriented (apart for Zeta/BeOS). In simutrans all objects themselves call display_xxx functions to do their drawing is a routine called display or zeichnen. Doing different in the GUI from all other objects in the game will add to confusion.
I have explained this so many times now; the GUI is still drawn in the zeichnen() routine, exactly how can this be confusing and for whom? Why isn't a call to display_ddd_proportional_clip_cl() or any other function also confusing?

Quote from: prissi on October 24, 2013, 09:42:56 AM
I also fail to see how adding another layer will make things more simple: I compare

display_stretch_image( gui_theme_t::imagename, place )

to

theme_manager->display_element( GUI_MAGIC_NUMBER, place );

One magic parameter to get the image information, and one location. Only that the second code jumps through some hoops to finally call the first function anyway.
It is about encapsulation, to isolate the theme responsibilities. If you study the code carefully you will see that display() is a pure virtual function and it is the derived implementation that is called. Since the artist/user can define this behaviour for each element, the GUI control doesn't know what Display() function to call. The right implementation of Display() is called through polymorphism. If the requested element is missing the theme manager can substitute it or draw a proxy instead. By putting this logic in the manager, we don't need to do this in each GUI control.

Quote from: prissi on October 24, 2013, 09:42:56 AM
And stacking clipping works quite well, otherwise the objects on scrollpanes would not clip. They exactly clip their children. But maybe I misunderstood again.
The stacked clip routines in the manager is cascading, meaning all pushed clip rectangles are clipped against the previous pushed rectangle. This prohibit a any child clip region to draw outside the parent's clip region.

Quote from: prissi on October 24, 2013, 09:42:56 AM
The only critic on my patch so far from Markohs was using too long constants which I shortened. Well I fixed this. I am happy to here some other critice apart from "Well that is boring like the rest" It is ment to be like the rest.
But this is going in circles now for so long now. It would nice if there is a decision, since otherwise Max-Max and me are not getting happier.
Back in the early days we saved code space by using short names, because we had limited amount of memory and the customer paid per line of code. This also created a lot of cryptic acronyms only known to the initiated.
Today when the memory isn't an issue any more and cut&paste operations goes easy and fast, there is no need to shorten stuff down. Of course some common sense has to be used and using_whole_sentences_as_names should be avoided.
I have used a namespace like nomenclature to organize the skin and theme elements into a logic structure. I'm not saying it is the best but this is my explanation to why it was implemented the way it was.

It seems like I have failed to explain the virtual Display() function and why it is implemented the way it is. MAybe some one else can explain this polymorphic construction better than me.
I only did the mistake to share my thoughts and code to early, when it is still in development and everything doesn't make 100% sense yet, due to unwritten code that will be developed further down the road.
- My code doesn't have bugs. It develops random features...

Markohs

I took the time to read your description fully now and I like the design. Just some questions, and not related to if this is included in simutrans or not, because we already know the status of that.

Why is text handled not as theme_element_t? Can't it be abstracted as a text_element_t?

I share some of prissi's thoughts respecting zeichnen and virtual calls, but not for the same reasons that him, I think. I just think as I told you before the process you describe in zeichnen should be named "generate_primitives", called on window update, that generates a list of the primitives to draw. Anyway, not having that infrastructure in our project, your solution is good enough, and easily refactorable to the one I propose, in the future.

I also understand and share your comments about encapsulation and responsability isolation.

About magic numbers, when I looked at that section of the code, it just understood it's an enumeration of frame classes, I don't really know what's so magical about them. But I might not have understood it fully, I guess. They just look like handlers. :)

Well, about the code, my personal advise is that if you feel that you want to do this, and it ends not being accepted or too modified, you can allways create your own fork of simutrans in github, and implement it. Once you have it finished and working, maybe the community changes his oppinion. I made a fork the other day to learn how git works, and quite easy to work with, you lose nothing trying.

And to prissi, I'd just suggest another way of handling this. Just give green light to Max-Max to implement this on his way, and when it's finished, if you don't like it, you can allways rollback his changes and change his code. Looking how things are, looks like it's the only realistic option we have now. Seems like prissi and Max-Max getting an agreement is almost impossible.

Ters

Quote from: Markohs on October 24, 2013, 07:04:59 PM
And to prissi, I'd just suggest another way of handling this. Just give green light to Max-Max to implement this on his way, and when it's finished, if you don't like it, you can allways rollback his changes and change his code. Looking how things are, looks like it's the only realistic option we have now. Seems like prissi and Max-Max getting an agreement is almost impossible.

Roll back? If it's committed to trunk, I'm pretty sure it won't be rolled back. If it's done, it will likely be done by rolling back all revisions on trunk since the GUI "went in the wrong direction", baby, bathwater and all, and starting over from there. Trying to roll back changes from a multitude of individual commits interspersed with other changes that should not be rolled back is simply too much work.

This is what branches are for, but merging such a branch back into trunk is no different than a big patch. It's a bit easier for others to follow what's going on, though.

Markohs

Ofc it's not so easy as pressing two buttons. But it's not rocket science, it's just code. But well, you are probably right, a branch is better suited for this.

prissi

Yes, I think getting an agreement is impossible.

The sole purpose of gui_frame_t is to assemble gui_elements in their positions. The drawing is entirely up to the elements. So what do I gain by having another layer, which is called to do the drawing?

You say the elements should have rule. Fine, but then these rules (or its positions) need to be changed in gui_frame_t (and within the element) to reflect the actual positions. Or you can click outside a button and still trigger it, which is not what should happen.

So what does the theme manager service provides in the end? Something like a graphic primitive to draw a button with alignment out of nine (or how many) images, and maybe labels. I fail to see the need for another layer.

If you really want a radical new GUI, one should rather go to a portable GUI lib, which would be better integrated into the OS anyway. Maybe even use Java, since with the dominance of Andriods, a Java frontend might became handy anyway.

But as Markohs said, go ahead. I keep the patch in sync, and I may make some other skins for my patch. But those are easily reused for whatever is applied in the end.

I would start to separate the buttons into different classes, but that will give quite lots of conflicts, until this is resolved. SO I may go back on the scrolled lists.

Ters

Quote from: Markohs on October 24, 2013, 07:57:38 PM
But it's not rocket science, it's just code.
I actually wish it was rocket science. That could be fun.
Looks like I should stop holding my breath waiting to see if the new GUI would get rid of the Go to depot button bug.

Max-Max

Quote from: Markohs on October 24, 2013, 07:04:59 PM
I took the time to read your description fully now and I like the design. Just some questions, and not related to if this is included in simutrans or not, because we already know the status of that.

Why is text handled not as theme_element_t? Can't it be abstracted as a text_element_t?
Sure it could be handled just like any other theme element but I have no intentions at this time to do this. But I see your thought here and it is an interesting design proposal to even abstract away the theme from the GUI control further. Maybe it would be more beneficial if we have hardware support and text can be done in the GPU. Let us all put that thought back in our heads to see if it grows...  ;)

Quote from: Markohs on October 24, 2013, 07:04:59 PM
I share some of prissi's thoughts respecting zeichnen and virtual calls, but not for the same reasons that him, I think. I just think as I told you before the process you describe in zeichnen should be named "generate_primitives", called on window update, that generates a list of the primitives to draw. Anyway, not having that infrastructure in our project, your solution is good enough, and easily refactorable to the one I propose, in the future.

I also understand and share your comments about encapsulation and responsability isolation.
Sorry, I forgot to comment you on that one.
Well, as for now the manager is only dealing with images, not primitives like rectangles or circles. I guess the images would be textures in a design using the GPU later on.
But as you pointed out, we don't have such system in place and with this abstraction it wouldn't be too hard to modify the theme manager to work in this way.

Quote from: Markohs on October 24, 2013, 07:04:59 PM
About magic numbers, when I looked at that section of the code, it just understood it's an enumeration of frame classes, I don't really know what's so magical about them. But I might not have understood it fully, I guess. They just look like handlers. :)
The THEME_ID_XXXX is a way for a GUI control to tell the theme manager what theme element to draw. The manager is mapping the requested theme ID (or handler if you prefer this term) to the actual instance of the derived theme_element_t class. Because the manageris doing the mapping, because it knows what is available, several THEME_ID_XXXX may use the very same instance (if they draw the same graphics and rule). The instance may also end up in reusing the same small cut images if they use the same graphics, but different rules.

Quote from: Markohs on October 24, 2013, 07:04:59 PM
Well, about the code, my personal advise is that if you feel that you want to do this, and it ends not being accepted or too modified, you can allways create your own fork of simutrans in github, and implement it. Once you have it finished and working, maybe the community changes his oppinion. I made a fork the other day to learn how git works, and quite easy to work with, you lose nothing trying.
I'm mostly used to use TortoiseSVN and the patch thing is nothing I have used before until now. Even if I know how to update my local tree, I still have some difficulties sometimes to keep in synch. To use Github would probably slow me down even more, if I don't fail completly  :o

Quote from: prissi on October 24, 2013, 08:02:46 PM
You say the elements should have rule. Fine, but then these rules (or its positions) need to be changed in gui_frame_t (and within the element) to reflect the actual positions. Or you can click outside a button and still trigger it, which is not what should happen.
If you look into my original patch you will out that there is a virtual member:

virtual scr_rect  get_client ( void ) = 0;

This returns the element's client rect, adjusted to the element's current rules so hit test can be done properly. So, yes, I have thought about this too. The filter button is using this to position the text, still implemented more like a proof of concept to work with the old skin format.

Quote from: prissi on October 24, 2013, 08:02:46 PM
So what does the theme manager service provides in the end? Something like a graphic primitive to draw a button with alignment out of nine (or how many) images, and maybe labels. I fail to see the need for another layer.
Try to read my previous post again, where I explain what it does. There are in fact several post where I explain this.

Quote from: prissi on October 24, 2013, 08:02:46 PM
...since with the dominance of Andriods, a Java frontend might became handy anyway.
Just a side note, you can develop in C/C++ on Android, you need to use the official NDK.

Quote from: prissi on October 24, 2013, 08:02:46 PM
But as Markohs said, go ahead. I keep the patch in sync, and I may make some other skins for my patch. But those are easily reused for whatever is applied in the end.
Okay so, how do you want me to do this. Develop everything into one big beta patch for testing, or smaller chunks, even if you don't see the full context of it as long it doesn't break anything?

Quote from: prissi on October 24, 2013, 08:02:46 PM
I would start to separate the buttons into different classes, but that will give quite lots of conflicts, until this is resolved. SO I may go back on the scrolled lists.
This is the path that lead me to the theme manager.

I started with the dialogue TITLE_HEIGHT thing and discovered that most dialogues where using hard coded numbers and even GUI functions that should be its own class. I came to the conclusion that I need to restructure the GUI classes to make better use of them.

So I started to look at the GUI, trying to make use of polymorphism and create the GUI controls that weren't GUI objects, but should have been. So In order to do this, since all GUI controls should be themed, I started with the theme manager so I can use it when I restructure the GUI classes.

...and it is here we are now...
- My code doesn't have bugs. It develops random features...

Dwachs

Max-Max, thank you for explaining your idea about the themes. It is a well-thought concept imho.

Let me rephrase it to see whether I understood it right. You are proposing an extra layer of theme-elements that handle the display of gui-components frames. The idea behind this is to have a unified way of displaying those frames. This has potential (like buttons that could handle different font sizes ?). But I share prissi's concern: Where this is useful besides using this for the button classes and window frames? I mean, which of the components in gui/components would benefit from the theme layer?

I agree with you about the need for more gui-components (for instance to take care of texts in a way that switching languages does not make the gui look messy). If this theme stuff is the price to pay to get gui-code refactored, better maintainable, easier exentable (different font size, no absolute pixel coordinates) etc. I am willing to pay it.

The way you described it, this theme thing consists of two fairly independent parts: First, the communication gui component <-> theme classes, and second, the communication theme manager <-> pak files/ besch system. Both are controversial, as the need for the theme manager is discussed as well as the structure of a potential new pak/besch/cutting system.

My proposal would be the following: Could you provide an updated patch(1) that contains the theme-related code, implements the theme for the gui-components, and does not introduce a new pak/cutting system (ie which uses the existing skinverwaltung stuff for compatibility)? That way we would have a smaller patch to discuss. Maybe you posted such a patch earlier - I did not notice as I did not follow this topic from the very beginning.

For development, I suggest to you to use git: mirror the simutrans-project at github: github.com/aburch/simutrans . This tool has a learing curve ofc, but it is so much better in handling branches than svn. I myself started with developing against a checked out svn repository. But then switched to git to develop larger patches (for instance the scripted scenarios).

(1)A patch that contains that contains exactly one feature: the theme manager and its relation to the gui components. No white-space changes in unrelated files, no debug code for makeobj, no change to resource file etc.
Parsley, sage, rosemary, and maggikraut.

Markohs

Quote from: Max-Max on October 24, 2013, 11:35:23 PM
Well, as for now the manager is only dealing with images, not primitives like rectangles or circles. I guess the images would be textures in a design using the GPU later on.
But as you pointed out, we don't have such system in place and with this abstraction it wouldn't be too hard to modify the theme manager to work in this way.

Indeed a image in your current code whould be 2 triangles with a texture applied in a 3D world.


You can generate a hardware vertex buffer when the frame is created, and just keep it updated when changes come to the frame, using a texture with all the theme images on it, a atlas.

Example of an atlas:



If you have the hardware vertex buffer updated, given it's physically stored on the video card memory, with the texture atlas, drawing it costs almost 0 CPU, as it's done entirely by the GPU.

The only problem is keeping that buffer updated, that's a IO-dependant iperation (travels by the PCI bus), and shoudn't be done each frame, just from time to time, when it's really needed.

Rendering text it's done in a similar fashion, you load the fonts in the texture atlas and map the letters to objects.

You are not designing that now, I'm just asking you to have in mind that idea is going there when we can, so try to avoid decisions that go against this idea.

prissi

I think this is rather how display_img must work, since it is low level drawing.

Since we only use handles (image ids) I think the exchange to a different drawing mechanism is the task of simgraph and not some upper GUI layer.

Max-Max

Quote from: Dwachs on October 25, 2013, 08:48:24 AM
Let me rephrase it to see whether I understood it right. You are proposing an extra layer of theme-elements that handle the display of gui-components frames. The idea behind this is to have a unified way of displaying those frames. This has potential (like buttons that could handle different font sizes ?). But I share prissi's concern: Where this is useful besides using this for the button classes and window frames? I mean, which of the components in gui/components would benefit from the theme layer?
Buttons and frames was only examples because they are simple and easy to understand, the system is meant to handle all theme related drawing, as a service to all GUI controls. The purpose is to let the GUI control to focus more on its function and less on theme adaptation, meaning a simpler way of drawing in the GUI control. The theme manager takes care of adjustments against the current selected theme.

Quote from: Dwachs on October 25, 2013, 08:48:24 AM
I agree with you about the need for more gui-components (for instance to take care of texts in a way that switching languages does not make the gui look messy). If this theme stuff is the price to pay to get gui-code refactored, better maintainable, easier exentable (different font size, no absolute pixel coordinates) etc. I am willing to pay it.
With small screens in mind the priority for a dialogue should be to not resize the dialogue to its content. This would have the effect that it might take up the whole screen and extend beyond it. Instead the dialogue content has to adapt to the given space by its parent. For this I proposed the collapse system that will resize children with focus on user interaction. This means that cosmetic GUI elements are likely to collapse first to give space for elements that are user interactions. For a normal Desktop this isn't a problem and collapsing will only happen if the user shrink the dialogues to much. The collapse system is described in this, somewhat, outdated document. This document can also be found in the very first post of this thread, under "project documentation".

Regarding text, it is truncated to its parent's client area, displaying an ellipse sequence to indicate truncated text. This will guarantee that all text stays inside the client and does not spill over. This is already in place for the most GUI controls. Again, this sin't really a problem on a Desktop but still handles the different lengths between languages. If you happen to select a language with generally longer strings, you only need to resize the dialogues until the ellipses disappear.

I have not put so much further thought about text, but it is an interesting idea to adapt the default button size to the most common button text length. The calculation of the adapted text length would be in the translator I guess and the theme manager can use this information to calculate the default button size, because the frame thickness also has to be in the mix.

Quote from: Dwachs on October 25, 2013, 08:48:24 AM
The way you described it, this theme thing consists of two fairly independent parts: First, the communication gui component <-> theme classes, and second, the communication theme manager <-> pak files/ besch system. Both are controversial, as the need for the theme manager is discussed as well as the structure of a potential new pak/besch/cutting system.

My proposal would be the following: Could you provide an updated patch(1) that contains the theme-related code, implements the theme for the gui-components, and does not introduce a new pak/cutting system (ie which uses the existing skinverwaltung stuff for compatibility)? That way we would have a smaller patch to discuss. Maybe you posted such a patch earlier - I did not notice as I did not follow this topic from the very beginning.
Yes, it is about encapsulation and unifying handling. My original patch was still using the current skin-PAK system as a base and added more flexibility. You can still use the "old" skin images. I originally even used the skin object in the .dat file to be 100% compatible. Prissi added the new theme object and removed the old skin object. Since I had plan to do this myself further down the road I kept the theme object, so all you need to do is a small edit in the theme .dat file, change the object from skin to theme. However after that Prissi started to ad a new object for each GUI element, and because the object itself is only used at PAK load it made things a lot more complicated and extra work if new elements where added in the system.

In my original patch I had changed everything back to use only one theme object for all elements. There is no support for cut lines, but the concept has been brought up to spread light on why things are done as they are. the cells are used as vertical cut lines as a way to be compatible with he current skin system with the addition that one, two or three cells can be used, depending on how many cut lines you want to use. the remaining undefined cuts are handled by the theme manager.

Quote from: Dwachs on October 25, 2013, 08:48:24 AM
For development, I suggest to you to use git: mirror the simutrans-project at github: github.com/aburch/simutrans . This tool has a learing curve ofc, but it is so much better in handling branches than svn. I myself started with developing against a checked out svn repository. But then switched to git to develop larger patches (for instance the scripted scenarios).
Well, I did installed and tried to fork the experimental and had somewhat success. I found a TortoiseGIT but the nomeclature is quite different from SVN so I didn't understood really how to do the things I'm used to do in SVN.

Quote from: Dwachs on October 25, 2013, 08:48:24 AM
(1)A patch that contains that contains exactly one feature: the theme manager and its relation to the gui components. No white-space changes in unrelated files, no debug code for makeobj, no change to resource file etc.
Maybe we should try to stabilize the current patch before I star to mess around with GitHUB, I can try to isolate the stuff into smaller and more defined patches or do we want things forked first? How does it work on gitHub, should I create patches or just announce a revision for review?
- My code doesn't have bugs. It develops random features...

Max-Max

Quote from: Markohs on October 25, 2013, 09:45:47 AM
...a atlas.
Yes, I'm familiar with the atlas concept...

Quote from: Markohs on October 25, 2013, 09:45:47 AM
You are not designing that now, I'm just asking you to have in mind that idea is going there when we can, so try to avoid decisions that go against this idea.
Yes, I will agree with Prissi here, this is on a low level and might be better off to implement in the low level layer if display_img_XXXX.
But I will keep this in mind for further design...
- My code doesn't have bugs. It develops random features...

Dwachs

Split the posts about git & friends, please continue the git related discussion here:
http://forum.simutrans.com/index.php?topic=12770
Parsley, sage, rosemary, and maggikraut.

prissi

Ok, the latest version with scaleable buttons is now at github: https://github.com/prissi/simutrans/tree/scaleable-gui It is essentially the one above, only with the images.

Max-Max

Wait, wait, wait.... What's going on her?

Is this Githhub link the one I should use, or should I do one of my own?
I'm in the middle of trying to restore my patch to what it once where, I thought we all agreed on letting me do my design?
- My code doesn't have bugs. It develops random features...

prissi

#518
Now really. This is just the patch I made earlier, on a personal branch in github (as was suggested in the other thread). Nothing changed there, just instead of a patch file a branch which contains also the graphics.

Max-Max

Quote from: prissi on October 26, 2013, 11:12:07 PM
Now really. This is just the patch I made earlier, on a personal branch in github (as was suggested in the other thread). Nothing changed there, just instead of a patch file a branch which contains also the graphics.
Hmm, it sounded worse than I meant  :-[

I was just wondering if that was the GitHub I should use or if I should fork a new one, from the standard?
- My code doesn't have bugs. It develops random features...

Dwachs

Quote from: Max-Max on October 27, 2013, 12:12:10 AM
if I should fork a new one, from the standard?
you should fork a new one, from the aburch/simutrans repository. This is the semi-official git-repository, which is continuously updated against the svn repository. Prissi registered moments ago at github and started his branch very recently ;)
Parsley, sage, rosemary, and maggikraut.

Max-Max

Quote from: Dwachs on October 27, 2013, 08:18:09 AM
Prissi registered moments ago at github and started his branch very recently ;)
So this means we are two developers developing the same thing in parallel, on different implementation paths...  :o
- My code doesn't have bugs. It develops random features...

Ters

Quote from: Max-Max on October 27, 2013, 11:40:27 AM
So this means we are two developers developing the same thing in parallel, on different implementation paths...  :o

It's not the first time that's happened in open source. In fact, Git and Mercurial were both written in parallel to become the replacement VCS for Linux.

But as I read it, it's only the branch that is new. The changes in it is old, and something prissi apparently has shared with us earlier.

kierongreen

I would also interpret this as prissi indicating this is the expected starting point for future patches. i.e. git branch is the planned implementation.

Max-Max

It seems like I have managed to restore my patch, at least it compiles again. I think I have managed to keep all the other updates that was made.

I have to convert the theme colour loading to the new colours system with RGB, then I might be able to isolate the patches a bit.
- My code doesn't have bugs. It develops random features...