The International Simutrans Forum

 

Author Topic: [Project] GUI Theme  (Read 129598 times)

0 Members and 1 Guest are viewing this topic.

Offline Sarlock

  • Devotee
  • *
  • Posts: 1340
  • Languages: EN
Re: [Project] GUI Theme
« Reply #455 on: October 18, 2013, 06:49:31 AM »
I'm going to chime in here as a non-coder and state a few observations.

First, I don't think anyone is intending to discredit your hard work, Max.  You've done some fabulous work here and I think everyone appreciates the effort that you have done and what you have accomplished.

With an open source project that is run by volunteers, we're at the mercy of everyone's ability to donate their time to the game.  We should be very appreciative of everyone's time with this and every other patch: the time that you have put in to coding it and the time that Prissi and others have put in to reviewing the code, making suggestions and modifications and attempting to incorporate it in to the main program.  We also have jobs and families to take care of.  Everyone is doing the best they can.

I am going to note something very important: Prissi and others have been around Simutrans A LONG TIME (I've been here a year now and I'm still a neophyte).  They know this code better than anyone else and they have had a lot of experience with things that have caused a lot of trouble and headaches in the future when the original authors have long gone and left the project.  I would defer to them and trust that they are making the right decision for the project.  You may not 100% agree with them, and you probably never will because everyone has their own opinion, but they have done a very wonderful job up to this point so obviously they must know a thing or two about what they're doing.  I'm sure right now their #1 concern (and mine, too) is that you're going to burn out and disappear and they will be left having to maintain your code for many years in to the future.  I think your priority should be to reassure them that that isn't the case.  You certainly can't blame them for wanting to carefully review the code and restructure it in a way that is familiar to them and in a way that will affect other areas of the code as little as possible.  Trust their experience in this area.

What finally reaches Simutrans may not be exactly what you wrote or even 50% but the hard work that you've done is certainly appreciated and will make the game a better place.  That's the nature of this kind of project: we throw things at the wall and see what sticks and what falls off.  If some or all of it falls off, we just shrug our shoulders and move on to the next job.  I can certainly get your defensiveness, but you come across as combative and it's very unlikely that this will benefit the game or community much.

As a side note, long posts where you reply to multiple quotes from other people is the #1 way to start flame wars.  And we don't want that here... let's be progressive and cooperative.  It's far more beneficial and much less stress for everyone involved.

-You wrote a great addition to the game
-Prissi and others have devoted considerable time to reviewing and modifying it when they would most certainly prefer to work on their own projects
-Whatever makes it in to the code will be a benefit to everyone: don't worry about how it is implemented or to what degree or what gets changed - trust that Prissi and the others have the best interests of Simutrans at heart (because they do, the many years have proven this)
-Smile, this is supposed to be fun.  This isn't that important.  Keep coding.  Work positively with any and all changes.

Offline kierongreen

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 2256
Re: [Project] GUI Theme
« Reply #456 on: October 18, 2013, 11:47:36 AM »
When working on several unrelated things, use different working directories to keep changes for each separate. An alternate solution for when one needs to do a small change, like a bug fix, and only a working directory full of changes is available, is to shelve all changes (in a patch or whatever fancy functionality may be available for this), revert, do the fix, commit and then unselve again.
Just a note when I'm working on larger patches, and fix an unrelated bug report I try to remember to only commit the files associated with the bug report itself, eg:
Code: [Select]
svn commit -m "FIX: bug" simworld.cc

-You wrote a great addition to the game
-Prissi and others have devoted considerable time to reviewing and modifying it when they would most certainly prefer to work on their own projects
-Whatever makes it in to the code will be a benefit to everyone: don't worry about how it is implemented or to what degree or what gets changed - trust that Prissi and the others have the best interests of Simutrans at heart (because they do, the many years have proven this)
-Smile, this is supposed to be fun.  This isn't that important.  Keep coding.  Work positively with any and all changes.
I second this (and the rest of what you wrote Sarlock) :)

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 9353
  • Languages: De,EN,JP
Re: [Project] GUI Theme
« Reply #457 on: October 18, 2013, 07:41:10 PM »
-Smile, this is supposed to be fun.  This isn't that important.  Keep coding.  Work positively with any and all changes.
Yes, this is the core why we all work on it.

I tried to keep as much from Max-Max as I could. I did not completely review my patch then, so spaces and some header changes are unneccessary. I freely admid that.

Max-max patch did not compile on unix (and most likely also not on Mac OS) and would also not work (do to using tolower on paths). Instead chdir() and MAX_PATH will work in simutrans. (Actually chdir() is Posix, so it will work on any Posix system.)

We made Simutrans quite portable over more than seven different architectures (DOS, Windows, Unix, Mac OS (Power PC and Intel), Beos/Zeta, (PowerPC)Amiga OS) So the infrastructure is there, MAX_PATH is available. (But with the prevailance of Windows this is quite understandable.)

I certainly will explain again, why I do not like to introduce a new way of image handling instead reusing the skin_besch_t. The main reason is consistency. You have only one kind of system of how images are handling. This is easy to document, to follow, to maintain, and consistent to use (even though it might not the simplest of all solutions). For the same reason the gui_theme_t works like the existing env_t. If you understood the one in Simutrans you can guess the working of the other. Same reasons, less different systems, less to remember. The latter is of outmost importance which such a large project.

About an OOP approach: Fine, but why those _tag stuff? If you want a structure with functions, then make it a class (because it is an object). You are using also an vector with function objects to draw stuff using predefined index. I can do this is C too, so it is also not very OOP, at least in my eyes. Assuming that OOP does not mean very non-linear code.

In the end we are discussing how to draw buttons and scrollbars only(!) while most of the real work is left untouched.

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 5417
  • Languages: EN, NO
Re: [Project] GUI Theme
« Reply #458 on: October 18, 2013, 09:31:25 PM »
I finally got time to look at the technical stuff.

-- bild_besch.h why add all the (void) parameter lists
A common misconception for C programmers, is to assume that a function prototyped as follows takes no arguments:
int foo();
In fact, this function is deemed to take an unknown number of arguments. Using the keyword void within the brackets is the correct way to tell the compiler that the function takes NO arguments.

Google it or check a few links.
Is there a difference between foo(void) and foo() in C++ or C
int foo (int argc, …) vs int foo() vs int foo(void) in C

I learned to always use foo(void) over foo(), unless you really need to have different implementations of foo with different arguments. foo(void) makes it absolutely clear, 100% that there will not be any parameters what so ever. So I have the habit of filling in the void when I see an foo(). I didn't know this would be a problem worth mentioning to refuse a patch...

I'm a bit surprised that after much complaining that Simutrans isn't C++ enough, that someone actually changes function signatures from C++ style to C style. But habits die hard. People still use #include <stdlib.h> and NULL in C++ even though #include <cstdlib> and 0 is the "correct".

-- scr_coord.h: what is purpose of the typedef tag_size {} size; construct ?
Have you tried to declare a struct referencing itself? The use of tag_ concept is a well known solution to this very problem.

I just tried. It works fine without typedef. Having to typedef structs, and naming the struct itself (the tag_ part) for self-referencing structs, is a C thing. In C++ class and struct is exactly the same thing, except that the default access specifier is private and public, respectively. (The GNU standard C++ library actually uses struct many places where I found have found class more natural. I seem to remember they almostly used struct earlier, but it's mostly class now.)

Offline Markohs

  • DevTeam, Coder/patcher
  • Devotees (Inactive)
  • *
  • Posts: 1559
  • Languages: EN,ES,CAT
Re: [Project] GUI Theme
« Reply #459 on: October 18, 2013, 09:39:18 PM »
I'd forget and consider against simutrans coding standards function(void), seems unnatural to me, and we use just function() in the whole code, makes no sense adding yet another way to do it.

Offline kierongreen

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 2256
Re: [Project] GUI Theme
« Reply #460 on: October 18, 2013, 10:54:34 PM »
I'd forget and consider against simutrans coding standards function(void), seems unnatural to me, and we use just function() in the whole code, makes no sense adding yet another way to do it.
I'd actually never noticed simutrans missing void from function declarations before. I also notice the simutrans coding style document doesn't make any mention about this - however as you point out function() is used mostly. While like Max-Max I always thought that it was preferable to write function(void) as Simutrans uses function() in preference I don't see changing this as being particularly useful.

Offline Markohs

  • DevTeam, Coder/patcher
  • Devotees (Inactive)
  • *
  • Posts: 1559
  • Languages: EN,ES,CAT
Re: [Project] GUI Theme
« Reply #461 on: October 18, 2013, 11:43:04 PM »
I'd prefer just using the non-void version and replace all the void occurrences by empty parameter list. Why having two variants in code when we can have just one, and as Max-Max pointed out, C++ suggest us to use a empty parameter list, why going against this recommendation.

So, if noone has anything against this, I'll just apply https://dl.dropboxusercontent.com/u/30024783/void.patch tomorrow and we finish that particular issue forever.
« Last Edit: October 18, 2013, 11:54:29 PM by Markohs »

Offline Max-Max

  • Coder/patcher
  • *
  • Posts: 670
    • MK Development
  • Languages: SV,EN
Re: [Project] GUI Theme
« Reply #462 on: October 19, 2013, 05:10:08 AM »
Max-max patch did not compile on unix (and most likely also not on Mac OS) and would also not work (do to using tolower on paths). Instead chdir() and MAX_PATH will work in simutrans. (Actually chdir() is Posix, so it will work on any Posix system.)

We made Simutrans quite portable over more than seven different architectures (DOS, Windows, Unix, Mac OS (Power PC and Intel), Beos/Zeta, (PowerPC)Amiga OS) So the infrastructure is there, MAX_PATH is available. (But with the prevailance of Windows this is quite understandable.)
Yes, I don't have the possibility to test this on any other system than Windows, so I really appreciate the help here to check this on other platforms.

I certainly will explain again, why I do not like to introduce a new way of image handling instead reusing the skin_besch_t. The main reason is consistency. You have only one kind of system of how images are handling. This is easy to document, to follow, to maintain, and consistent to use (even though it might not the simplest of all solutions). For the same reason the gui_theme_t works like the existing env_t. If you understood the one in Simutrans you can guess the working of the other. Same reasons, less different systems, less to remember. The latter is of outmost importance which such a large project.
The information in skin_besch_t is redundant and only used to pick the skin image number. The theme manager creates an internal structure with additional information for quick access, by using the already implemented bild_t and the global images[] list. In this way the standard display_img_XXX routines can be used to draw them. As I have said all the time skinverwaltung_t::theme is only used to read the PAK file and register the theme elements. This could be done directly by the theme manager or skinverwaltung_t::theme can be destroyed after registration. Nothing else is using skinverwaltung_t::theme because all the images as been registered and recorded by the theme manager.

About an OOP approach: Fine, but why those _tag stuff? If you want a structure with functions, then make it a class (because it is an object).
I have explained several times why I made those as structs and why the tag_ is used. I also have said, fine, use a class but make it all public because it is meant to be a light weight type with some helper functions. There are also pros and cons to use a class or a struct for this. But sure, class is fine by me if it helps to understand the code better...

You are using also an vector with function objects to draw stuff using predefined index. I can do this is C too, so it is also not very OOP, at least in my eyes. Assuming that OOP does not mean very non-linear code.
I think you have missed out the most important design part here. theme_element_t is an ADT, it's a base class with a pure virtual interface and can't be instanced by itself. The real implementation is in frame_element_t, horizontal_element_t, vertical_element_t and symbol_element_t. These derived classes are coded to follow different rules regarding how a theme element behave to size changes. For the moment each GUI element has this class hard coded, but in the end the artist can chose what rule to apply to each GUI theme element.
For example: The round button.
Now the round button is hard coded to be the frame type, extending the edges in both horizontal and vertical directions.

register_frame ( THEME_ID_BUTTON_UP, SKIN_BUTTON_SIDE_LEFT, SKIN_BUTTON_BODY, SKIN_BUTTON_SIDE_RIGHT );

But what if the button isn't suitable for vertical size changes? Here the artist will be able to say that the button is of horizontal type, only allowing it to extend horizontally.

register_horizontal ( THEME_ID_BUTTON_UP, SKIN_BUTTON_SIDE_LEFT, SKIN_BUTTON_BODY, SKIN_BUTTON_SIDE_RIGHT );

In the registration process a class factory pattern creates the correct derivation of theme_element_t (frame, hoeizontal, vertical etc...) and puts it in a vector list holding pointers of the base class type. In this way all the different types of derivatives can be held in the same list.

Code: [Select]
bool theme_manager_t::register_frame( theme_element_id theme_id, image_id left_id, image_id middle_id, image_id right_id )
{
theme_frame_t *element = new theme_frame_t; // <---------------- Create correct class for a frame behaviour
bool ret;

// delete any previous definition
if (elements[theme_id] != NULL) {
delete elements[theme_id];
}

remove_offset(left_id);
remove_offset(middle_id);
remove_offset(right_id);
if ( false  ==  (ret = element->init(left_id, middle_id, right_id)) ) { // <------ Create images for a frame behaviour (3x3)
delete element;
element = NULL;
}

elements[theme_id] = element; //  <---------------- elements is a theme_element_t vector list, the base class of theme_frame_t.

return ret;
}
The GUI element (such as a button or scrollbar) doesn't need to know how this is drawn or what rule is applied for a specific theme. The theme manager keep a vector list of the base class type theme_element_t, and calls the virtual function draw that is defined in the derived class, different depending on the type.

If this had been a button with the horizontal behaviour...

Code: [Select]
bool theme_manager_t::register_horizontal( theme_element_id theme_id, image_id left_id, image_id middle_id, image_id right_id )
{
theme_horizontal_t *element = new theme_horizontal_t; // <---------------- Create a different class for a horizontal behaviour
bool ret;

// delete any previous definition
if (elements[theme_id] != NULL) {
delete elements[theme_id];
}

remove_offset(left_id);
remove_offset(middle_id);
remove_offset(right_id);
if ( false  ==  (ret = element->init(left_id, middle_id, right_id)) ) { // <------ Create images for a horizontal behaviour (1x3)
delete element;
element = NULL;
}

elements[theme_id] = element; //  <---------------- elements is a theme_element_t vector list, the base class of theme_horizontal_t.

return ret;
}

If we look at these two functions, they are identical except for the created theme class. I have not yet done any optimisation! Because the init() function is a pure virtual function, this can be significantly optimised, but I will do this a little bit further down the road when we have a "real" class factory, controlled by the artist choice of applying theme rule.

As you can see, all these theme classes are ending up in the same vector list elements[] where the index is a specific type of GUI theme element, such as BUTTON UP, BUTTON DOWN, GROUP BOX, FRAME OUTLINE etc... But because it is a base class pointer the implementation of the virtual function Display() are different, as described above.

So the GUI button really doesn't need to know what rule the artist as applied to it, it only tells the theme manager to draw a theme ID, such as BUTTON UP. How this is done, is of no interest for the button.

theme_manager.display( (b_enabled) ? ((pressed) ? THEME_ID_BUTTON_DOWN : THEME_ID_BUTTON_UP) : THEME_ID_BUTTON_DISABLED, frame);
// continue button drawing, such as text...
[/tt]

The theme manager picks up the base pointer from the list and calls the virtual function draw().

Quote
bool theme_manager_t::display ( theme_element_id theme_id, scr_rect frame, control_alignment_t align)
{
   theme_element_t *element = elements[theme_id];

   // Element defined?
   if(element == NULL) {  <-------------- We can implement an emergency-last-resort-fall-back here
      char Buff[5];
      itoa(theme_id,Buff,10);
      DBG_DEBUG(LOCATION "Unknown theme id ", Buff );
      return false;
   }

   // Don't draw themes outside current clip rectangle.
   scr_rect current_clip = display_get_clip_wh();
   if ( RECT_RELATION_OUTSIDE  ==  current_clip.relation(frame) ) {
      return false;
   }

   // Draw theme
   element->display( frame, align ); // <----------------  Call the implementation of the pure virtual function Display()

   return true;
}

As you can see, the GUI button or the theme manager don't need to know if this is a frame, horizontal, vertical, or symbol implementation. The correct routine is called through its ADT interface (the base class).
If we want to add more behaviour rules, we only need to add a new derived class from theme_element_t.

The registration process isn't so forgiving at this moment but after the registration, there is a validation phase.

Code: [Select]
bool theme_manager_t::validate( void )
{
if(elements[THEME_ID_BUTTON_DISABLED] == NULL) {
elements[THEME_ID_BUTTON_DISABLED] = elements[THEME_ID_BUTTON_UP];
}
return true;
}

This is only an example of how this will work. If an element is missing (NULL) it can reuse another elements class instance as fall back. In this case, if there is no definition for a disabled button, the button up behaviour and images will be reused.
If you apply the full patch and put a break point for various GUI controls (using the theme manager) you will see that even if they all use the same vector list, different code is called for the Display() function.

Quote
In the end we are discussing how to draw buttons and scrollbars only(!) while most of the real work is left untouched.
As you can see, my design is very OOP, using polymorphism to allowing us (artists and users) assign any of the available behaviours to any GUI element.

Please ask me if there is something here that is unclear, or feel free to fill in to explain the OOP aspect in this design.
« Last Edit: October 19, 2013, 08:50:14 AM by Max-Max »

Offline Max-Max

  • Coder/patcher
  • *
  • Posts: 670
    • MK Development
  • Languages: SV,EN
Re: [Project] GUI Theme
« Reply #463 on: October 19, 2013, 09:08:42 AM »
Regarding foo(void) and foo() it seems like you all misunderstood me.

I'm, and the recommendation is, in favour for foo(void) because foo() can take anything as a parameter, this is the old C style and C++ allows it for backward compatibility.
foo(void) makes it clear that it can't be anything else than a void. foo() is not the same as foo(void). To ensure cross compiler compatibilities, foo(void) is treated the same across all compilers and enforces type checking. foo() turns of type checking for the parameters.

Offline Markohs

  • DevTeam, Coder/patcher
  • Devotees (Inactive)
  • *
  • Posts: 1559
  • Languages: EN,ES,CAT
Re: [Project] GUI Theme
« Reply #464 on: October 19, 2013, 12:07:33 PM »
Okay then, another issue we can't reach consensus then. This is not the way to go, imho. Let's have two way of doing yet the same thing in our code.

And this was a small issue to agree on. This makes no sense.

Using (void) on the declaration you avoid being called with parameters, that well, will avoid... Nothing, since they will be just ignored anyway. You are asking for understanding but can't yet agree in such small and irrelevant issues.
« Last Edit: October 19, 2013, 12:14:50 PM by Markohs »

Offline Dwachs

  • DevTeam, Coder/patcher
  • Administrator
  • *
  • Posts: 4530
  • Languages: EN, DE, AT
Re: [Project] GUI Theme
« Reply #465 on: October 19, 2013, 12:30:12 PM »
To spice up the discussion. Here is a small test file in c++:
Code: [Select]
void foo();

struct bla {

        bla();
        bla(int);
        ~bla();
};

int main()
{
        bla a(1);
        foo();
        foo(1);
        foo(1,1);
}
It does all the bad things: functions without arguments, not void put there. Structs with self-references.

Here is what g++ thinks about this:
Code: [Select]
~/ccc> g++ testcpp.cc
testcpp.cc: In function 'int main()':
testcpp.cc:14:7: error: too many arguments to function 'void foo()'
testcpp.cc:2:6: note: declared here
testcpp.cc:15:9: error: too many arguments to function 'void foo()'
testcpp.cc:2:6: note: declared here
It seems that Max-Max and Gnu C++ compiler have different understanding of the language: the struct compiles fine, can reference itself, the function DOES NOT accept more than zero arguments although the declaration is not written as 'void foo(void)'.

@Markohs, please commit your patch.

Offline Markohs

  • DevTeam, Coder/patcher
  • Devotees (Inactive)
  • *
  • Posts: 1559
  • Languages: EN,ES,CAT
Re: [Project] GUI Theme
« Reply #466 on: October 19, 2013, 01:05:00 PM »
Done @ 6827

Just another tought, I don't like many of the current coding styles of simutrans, Max-Max, I'd use singletons, namespaces and I hate not being able to:

if(cond) do_it();

or

function a(){
}

 Or many other things, but I accept it, it's the way we do it. After all, they are just subtle details that don't forbid you to do what you want to do, that's coding.

 We should restrict our discussions to important things, let's waste time there, and not on this minor details, it's better to all.

 About the other discussions:

 1) I consider completely acceptable using structs with creator functions, in fact there are a lot in our code, it's indeed the way to go imho.
 2) Reading your ADT description, it makes complete sense to me. And given it's a "registration" process, that won't be done *each frame* if I understood correctly, but just on frame creation, I think it's a good idea, and the way to go. If virtual calls are used during frame display as you seem to show on the display function, should be *fast*, very fast code.
 3) As for the 3x3 issue, and having prissi expressed he liked the idea, I guess it's accepted, not more to say on that, I trust him.

 As a general idea, and not knowing much of the details, all the heavy OOP code, should be in the set-up of the windows, and operations that will be done on window creation, management, event processing... At the end this theme management code, should create "data structures", data strectures that a completely data-driven algorithm, a C-Style function is able to just process *each frame*, and draw the items light-speed. Aliginments, positioning, event management... all that kind of stuff *can* be slow, but drawing , the process of displaying all the windows each frame, *must* be fast.

 Why? Because drawing is done 20 to 30 times *per second*, and the other operations are just done *once*, on frame creation, updating, event receiving... much less often.

I have not yet done any optimisation! Because the init() function is a pure virtual function, this can be significantly optimised, but I will do this a little bit further down the road when we have a "real" class factory, controlled by the artist choice of applying theme rule.

 Do it then, asap. It's the best way you can support your code.
« Last Edit: October 19, 2013, 03:50:45 PM by Markohs »

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 9353
  • Languages: De,EN,JP
Re: [Project] GUI Theme
« Reply #467 on: October 19, 2013, 01:14:30 PM »
... foo() is not the same as foo(void). To ensure cross compiler compatibilities, foo(void) is treated the same across all compilers and enforces type checking. foo() turns of type checking for the parameters.
Sorry, this is wrong. Look at the C++ standard draft. It is nearly 1350 pages, but on almost any of them you find call with empty parameter lists: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/n3242.pdf

In C++ f() is identical to f(void) look in the standard! Hence, calling f(bla,i) for something like f() will result in C++ with an error, since functions have name mangling and f() is internally mangled as (void). It would be different, if the functions is called extern "C" int f(); (which is not allowed according to the standard §7.5.7 page 174, btw.). It is actually the same thing as the _tag stuff (which the standard defines on §8.4.2.5 pages 198 as not needed.) This is something only C needs.

Your style and my differ, but is is not required by the standard.  I am with that on Markohs and rather have his patch applied (and amend the documentation). Most void s in the code are relicts from the C to C++ conversion several files underwent about five years ago. So two votes for removing them.

EDIT: goolgeling the standard and all that, I saw it was already submitted.

In order to get this back on track I will try to submit the undisputed parts. Otherwise everything just gets bitrot.
« Last Edit: October 19, 2013, 02:06:29 PM by prissi »

Offline Max-Max

  • Coder/patcher
  • *
  • Posts: 670
    • MK Development
  • Languages: SV,EN
Re: [Project] GUI Theme
« Reply #468 on: October 19, 2013, 09:27:40 PM »
To spice up the discussion. Here is a small test file in c++...
Okej, this foo( void ) stuff is not really what I wanted to have reviewed, I was only answering why I put in some void. But since you are at it I have to correct an misunderstanding of its use, to not make me look like a complete failure...

You can do a forward declaration in your header file like this: void foo();

Because foo() is not specifying any type, it can be anything in the implementation of the function foo.
For example:

void foo(int bar) {}

There is nothing in the header file indicating that your intention is to actually take an int as parameter.
This behaviour is an old C legacy where you could have one header file specifying foo() and then have different .cpp files (like simgraph16.cc and simgraph0.cc) but with different parameters in the actual void foo() implementation.

So in this case foo() isn't necessarily the same as foo( void ) where you distinct specify that the implementation really don't take any parameter, this is what I was taught... enough about this.

So now when Dwachs and Prissi has spent so much time on this minor insignificant detail, I really would like to hear what you have to say about the design I presented. Have you understood how it works? Any questions?

To follow up on Markohs comments.
Yes, you have got it quite right. The theme manager is preparing all images on theme load. The virtual functions make sure that you can treat all the different types of element rules the same by using the common base interface where the implementation of Display() is doing the drawing according to the set of rules applied for this theme.

In this design there is no need to do any tests to see what rule to apply, just call the virtual function and the rest is done in its final implementation (derived class).

If we didn't use a virtual function and implemented it more like a structural C implementation, we first have to test what rule to apply and then call the right function for it. You achieve the same thing but has to do an extra test first.

When it comes to GUI drawing in general (I'm not counting the karte to GUI drawing), it only needs to be updated when the user interact or a value gets updated. A much faster update approach is to have an off screen buffer of the GUI window (the frame_t). The off screen bitmap is the one updated when the user interacts, then 30 times a second, this off screen buffer is just blitted to the screen memory.

This saves a lot of time because GUI updates are very rare in this context.

One way to even minimize the impact further is to only mark the window as dirty (off screen image needs to be updated), but have a separate thread that updates dirty windows when there is time for it (for this a double buffer might be needed to minimize flicker, maybe not).

I did suggested this approach earlier in the project but got the answer that the GUI drawing stuff takes so little time that it isn't needed. Maybe we should reconsider this solution instead of constrain features in the GUI, if we get to that point.

Offline Markohs

  • DevTeam, Coder/patcher
  • Devotees (Inactive)
  • *
  • Posts: 1559
  • Languages: EN,ES,CAT
Re: [Project] GUI Theme
« Reply #469 on: October 19, 2013, 09:44:35 PM »
If we didn't use a virtual function and implemented it more like a structural C implementation, we first have to test what rule to apply and then call the right function for it. You achieve the same thing but has to do an extra test first.

 Well, virtual functions trigger a vtable method lookup too, so it's comparable, anyway ofc it's better to do it with virtual functions because it's way more understandable, maintainable, blablabla... Better design.

Quote
(I'm not counting the karte to GUI drawing)

 Yes, ignore that, that's gonne be handled separately.

When it comes to GUI drawing in general (I'm not counting the karte to GUI drawing), it only needs to be updated when the user interact or a value gets updated. A much faster update approach is to have an off screen buffer of the GUI window (the frame_t). The off screen bitmap is the one updated when the user interacts, then 30 times a second, this off screen buffer is just blitted to the screen memory.

 I can understand the blitting, and it's a good idea, *but* I'd not implement it that way.

 Why?

 1) Blitting is not so fast, given we don't have libraries that can rely that blitting to hardware, we don't have DirectX anymore.
 2) You can acheive *almost* the same effect having each GUI unit (a frame, but you can sub-divide the problem), generating a set of primitives to get drawn on the framebuffer, so the draw() method just has to transverse that list and execute them in order.

 Why I suggest this? Because in our current implementation, it's *fast enough*, and, in the (desirable) case we get proper 3D support in the future, we can just get those drawing primitives inside a vertex buffer in the VRAM, and just tell the hardware to render it, with no extra CPU cost.

 It also has one extra advantage, on a small sub-window change, we don't have to re-create the full bitmap again, just update the affected part, faster.

Quote
This saves a lot of time because GUI updates are very rare in this context.

Indeed.

Quote
One way to even minimize the impact further is to only mark the window as dirty (off screen image needs to be updated), but have a separate thread that updates dirty windows when there is time for it (for this a double buffer might be needed to minimize flicker, maybe not).
I did suggested this approach earlier in the project but got the answer that the GUI drawing stuff takes so little time that it isn't needed. Maybe we should reconsider this solution instead of constrain features in the GUI, if we get to that point.

 We'll see what gets of this.

Offline kierongreen

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 2256
Re: [Project] GUI Theme
« Reply #470 on: October 19, 2013, 09:56:58 PM »
Quote
Okej, this foo( void ) stuff is not really what I wanted to have reviewed, I was only answering why I put in some void. But since you are at it I have to correct an misunderstanding of its use, to not make me look like a complete failure...
Please don't think that's what people are trying to do. I'm sure people understand where you are coming from - just understand that the existing simutrans code style is to use foo() not foo( void ), and no matter what the technical merits or otherwise that won't be changing.

Offline Max-Max

  • Coder/patcher
  • *
  • Posts: 670
    • MK Development
  • Languages: SV,EN
Re: [Project] GUI Theme
« Reply #471 on: October 19, 2013, 10:02:16 PM »
I can understand the blitting, and it's a good idea, *but* I'd not implement it that way.

 Why?

 1) Blitting is not so fast, given we don't have libraries that can rely that blitting to hardware, we don't have DirectX anymore.
 2) You can acheive *almost* the same effect having each GUI unit (a frame, but you can sub-divide the problem), generating a set of primitives to get drawn on the framebuffer, so the draw() method just has to transverse that list and execute them in order.
Hmm, I was referring to software blitting.  So what I meant was the use of memcpy() to just "blit" the off screen memory to the "screen" memory, same memory all display_XXX functions use as destination.

Why I suggest this? Because in our current implementation, it's *fast enough*, and, in the (desirable) case we get proper 3D support in the future, we can just get those drawing primitives inside a vertex buffer in the VRAM, and just tell the hardware to render it, with no extra CPU cost.
Actually in Windows7 and up, all old Win32 GDI API are redirected to use "DirectX", this means old programs using GDI benefits from GPU hardware.
But you get a good point there, when we move to Direct3D ;)

EDIT***
Kierongreen
That wasn't my intention to brag over something, I just wanted to set the record straight. My personal opinion is to be as clear as possible, to specify void when it indeed is a void. But if no one else wants this, fine, put it in the coding rule document so everyone knows, because now it's a mix (not only my code). :)

Offline kierongreen

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 2256
Re: [Project] GUI Theme
« Reply #472 on: October 19, 2013, 10:11:23 PM »
Latest commit really breaks compiles on linux. Please don't use _MAX_PATH or other Windows API defines. Trying to fix this and other compile errors.

Also overloading of scr_coord and koord breaks. Only way to fix this I can see is changing a whole load of koord to scr_coord in gui headers. I'm not doing this as I assume someone else is better qualified to do this.
« Last Edit: October 19, 2013, 10:17:34 PM by kierongreen »

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 9353
  • Languages: De,EN,JP
Re: [Project] GUI Theme
« Reply #473 on: October 19, 2013, 10:15:51 PM »
Quote
You can do a forward declaration in your header file like this: void foo();
No, you cannot in C++, not for a function taking an int. Final statement, read the standard!

You know, being taught and so is nice. But Dwachs and me did look up the stuff or tried it out. You could have done too, since you boldly stated wrongly that f() and f(void) are different even in C++. It would be more helpful to admid that you learned something new (as I discover every day {and that is why I looked it up in the standard} ).

But let's get back to topic.

Markohs gave already very good arguments why not double buffering dialogues. Let me add: there are many dialogues whose contents often changes every, sometimes frame, convois (the view, costs,statistics ), stations (view, list of goods, departure board, statistics), finances, and so one. One very few dialogues have no changes at all. With all this, double buffering dialogues does not sound like we would gain much for for the trouble of extra code.

We will never move to Direct3D, since it is not portable. Markohs did a OpenGL port, which was working a little. It should be in the branch when you check out. It suspect however a little dust around there.

By the way, I submitted you changes as much as I could without any disputed part. That will make you patches much smaller. So I am to blame for breaking, sorry.

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 5417
  • Languages: EN, NO
Re: [Project] GUI Theme
« Reply #474 on: October 19, 2013, 10:37:07 PM »
My purpose has never been to hand out dunce hats, if that's what anyone thinks. I was sharing my knowledge of how things work, not only for those involved in the thread at the present, but for future readers as well.
We will never move to Direct3D, since it is not portable. Markohs did a OpenGL port, which was working a little. It should be in the branch when you check out. It suspect however a little dust around there.
There are also my two attempts, in which I found that Simutrans is very 3D-unfriendly. Recent changes might clear up the problems in the engine, but the biggest problem is in the way graphics and sub-tile positioning is tied to the 2D rendering.

Offline Max-Max

  • Coder/patcher
  • *
  • Posts: 670
    • MK Development
  • Languages: SV,EN
Re: [Project] GUI Theme
« Reply #475 on: October 19, 2013, 10:58:55 PM »
No, you cannot in C++, not for a function taking an int. Final statement, read the standard...
My GOD! I think I must be really bad at explaining what I mean, maybe I used the wrong word somewhere or got lost in translation... Just let us drop it, I will not make any more tries to explain what I meant.

I have no problem to admit that I have wrong or if I don't know something. I do ask when I have questions, but it isn't always I get an answer to them.

Markohs gave already very good arguments why not double buffering dialogues...
Well, it was only a suggestion to speed things up a little. To only update a small view is faster than updating a whole window. Maybe I failed to get that through too...

We will never move to Direct3D, since it is not portable. Markohs did a OpenGL port, which was working a little. It should be in the branch when you check out. It suspect however a little dust around there.
Well, I was only making a comment on Markohs "in the (desirable) case we get proper 3D support in the future...", did you notice the smiley I put in there?

By the way, I submitted you changes as much as I could without any disputed part. That will make you patches much smaller. So I am to blame for breaking, sorry.
Well, I can't really update my local trunk until we have agreed on the disputed code. Either way, I will have quite some work to do to get it up in synch. There fore I can't really do anything until then...

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 9353
  • Languages: De,EN,JP
Re: [Project] GUI Theme
« Reply #476 on: October 19, 2013, 11:00:09 PM »
Sorry, it seems I misunderstood. Let's cool this off again.

(Just a short remark on this meanderign thread to 3D: The new "transportGeneral" has somehow transferred an isometric engine sucessfully to 3D. Unfourtunately there is no source, despite strong hitns that GPL'ed code has been involved. But we are getting offtopic.)

Offline Max-Max

  • Coder/patcher
  • *
  • Posts: 670
    • MK Development
  • Languages: SV,EN
Re: [Project] GUI Theme
« Reply #477 on: October 19, 2013, 11:07:20 PM »
Although the new "transportGeneral" has somehow transferred an isometric engine sucessfully to 3D. Unfourtunately there is no source, despite strong hitns that GPL'ed code has been involved. (But we are getting offtopic.)
Actually Torque3D and Torque2D has gone open source. I know that Torque2D has great support for isometric rendering.

Offline Markohs

  • DevTeam, Coder/patcher
  • Devotees (Inactive)
  • *
  • Posts: 1559
  • Languages: EN,ES,CAT
Re: [Project] GUI Theme
« Reply #478 on: October 19, 2013, 11:55:18 PM »
Hmm, I was referring to software blitting.  So what I meant was the use of memcpy() to just "blit" the off screen memory to the "screen" memory, same memory all display_XXX functions use as destination.
Actually in Windows7 and up, all old Win32 GDI API are redirected to use "DirectX", this means old programs using GDI benefits from GPU hardware.
But you get a good point there, when we move to Direct3D ;)

 Even using GDI, I'm afraid it makes no difference. Using memcpy you won't trigger the DirecX hardware blit, you need to do more complex operations, expalined here (I just googled it and didn't read it, just to point it's not so simple as memcopy).

 Using our current primitives, display_img_* display_text_* ... we just rle decode a image into a buffer, that's later copied to another buffer, vía GDI, or SDL, whatever. To make use of hardware we'd need to copy uncompressed RGBA 32-bit buffers, from one buffer to others, using blitting. What we do it's not blitting.

 About the 3D implementation, I was not the only author, Ters did maybe even more than me in that subject, and we faced big problems in doing so, given the nature of this game (don't forget this game ran in MS-DOS) . Most of the changes I've been doing so far are trying to ease this problems, so our next try, is hopefully successful. It's not an abandoned project. But I lacked knowledge of the game, something I'm trying to solve, to see how can we blend this game with the 3D game technology, that differ, and quite a lot.

 In fact, the changes I suggested to you are in line in how a modern game handles its GUI, in OpenGL/Direct3D. :)

Actually Torque3D and Torque2D has gone open source. I know that Torque2D has great support for isometric rendering.

 It's way harder than choosing a framework, this problem lies way deeper in the code.

Offline Max-Max

  • Coder/patcher
  • *
  • Posts: 670
    • MK Development
  • Languages: SV,EN
Re: [Project] GUI Theme
« Reply #479 on: October 20, 2013, 12:12:25 AM »
Even using GDI, I'm afraid it makes no difference. Using memcpy you won't trigger the DirecX hardware blit, you need to do more complex operations, expalined here (I just googled it and didn't read it, just to point it's not so simple as memcopy).
Well, I basically meant that one memcpy() call would probably be faster than have to call all the windows GUI controls. Frame_t on its own is basically doing a full memcopy() already (background) and on top of that each GUI control. However it was only a suggestion if we need to speed things up...

In fact, the changes I suggested to you are in line in how a modern game handles its GUI, in OpenGL/Direct3D. :)
I think we are on the same page here. To test and then call a C like function each frame compared to call a virtual function I think takes the same amount of time. In fact I even think the VTable is faster here because we don't need to do any tests, only fetch the virtual function from the VTable. Most modern compilers of today handles VTables quite good and the overhead that gave virtual functions bad reputations in the beginning is almost gone.

...and if we do need more time, I just made the off screen as a suggestion (and only redraw GUI children that has changed) to save more time. Again this is only a suggestion...

Offline Markohs

  • DevTeam, Coder/patcher
  • Devotees (Inactive)
  • *
  • Posts: 1559
  • Languages: EN,ES,CAT
Re: [Project] GUI Theme
« Reply #480 on: October 20, 2013, 12:32:08 AM »
 I think the more flexible and viable option, with an acceptable performance in any circumstance is the one we discussed that's:

* Use heavy OOP programming on all the GUI code, window movement, updating, gui elements alignment, event handling and management, but this functions just need to keep up-to-date a list of primitives, not complicated.
* Then, a routine, with little to no OOP just transverses that list and redraws the *whole* gui. This has to be fast, super-fast. It's going to be done ~30 times per second.

 Forget about partial redraws, better to go all-in here, focus on the fastest full redraw. Simutrans and all the games I know of, just redraw the whole UI per-frame, fully.

Offline Dwachs

  • DevTeam, Coder/patcher
  • Administrator
  • *
  • Posts: 4530
  • Languages: EN, DE, AT
Re: [Project] GUI Theme
« Reply #481 on: October 20, 2013, 07:30:03 AM »
Also overloading of scr_coord and koord breaks. Only way to fix this I can see is changing a whole load of koord to scr_coord in gui headers. I'm not doing this as I assume someone else is better qualified to do this.
should compile again.

Offline kierongreen

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 2256
Re: [Project] GUI Theme
« Reply #482 on: October 20, 2013, 09:49:30 AM »
Does indeed compile now many thanks. However there are still a large number of warnings in gui. Will try seeing what is behind these.

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 5417
  • Languages: EN, NO
Re: [Project] GUI Theme
« Reply #483 on: October 20, 2013, 10:47:00 AM »
Here's a patch for the reorder warnings. The other warnings in the GUI are unused parameters and variables, I assume because something isn't finished yet.

Apart from that, Simutrans is surprisingly warning free these days.

Update:
Another patch for a possibly GUI related warning in simstring.cc.
« Last Edit: October 20, 2013, 10:53:35 AM by Ters »

Offline kierongreen

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 2256
Re: [Project] GUI Theme
« Reply #484 on: October 20, 2013, 11:02:46 AM »
Quote
Here's a patch for the reorder warnings. The other warnings in the GUI are unused parameters and variables, I assume because something isn't finished yet.
Thanks :)

Quote
Apart from that, Simutrans is surprisingly warning free these days.
Indeed - want to keep it that way too as it makes debugging much easier!

Offline Dwachs

  • DevTeam, Coder/patcher
  • Administrator
  • *
  • Posts: 4530
  • Languages: EN, DE, AT
Re: [Project] GUI Theme
« Reply #485 on: October 20, 2013, 11:15:18 AM »
Thank you, all of this should be now in r6837.

Offline Markohs

  • DevTeam, Coder/patcher
  • Devotees (Inactive)
  • *
  • Posts: 1559
  • Languages: EN,ES,CAT
Re: [Project] GUI Theme
« Reply #486 on: October 21, 2013, 01:22:14 PM »
updated coding_styles.txt to reflect the (void) issue, if someone wants to make a better description, please do so, my english is not so good. :)

Offline Max-Max

  • Coder/patcher
  • *
  • Posts: 670
    • MK Development
  • Languages: SV,EN
Re: [Project] GUI Theme
« Reply #487 on: October 22, 2013, 09:39:58 PM »
So where are we standing here?
I'm not sure if you'll are waiting for me to submit something or if more of the patch is on its way in?

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 9353
  • Languages: De,EN,JP
Re: [Project] GUI Theme
« Reply #488 on: October 22, 2013, 10:03:59 PM »
I though long about how to proceed: I am now convinced that it could be nice to cut the images semi-automatically, as an alternative. But I think makeobj is the right place for the cutting (as Dwachs suggested too).

I could image a command "cutimage[0]=bla.#xFF0000" as alternative (not complete replacement) to "image[0]=m.0.0 image[1]=m.0.1, ..."  which then takes this image and cuts it along the red lines and adds it to a list (either 2, 3, 2x2, 2x3, 3x3, ... according to the lines). It should work also for other stuff, not only buttons (like buildings etc.)

Second thing is, that 3x3 scalable buttons are nice. I am just playing around with them (I append work in progress a patch with the sources and pak for the only skin that is halfway designed. It is an ugly color choice too!) If you want to move the drawing into gui_theme or another file, well I can live with that too.

Apart from how stuff is actually loaded and where the drawing subroutine is located: Many thing are still not finished, when thinking at the original goal. Like to derive posbuttons form a base button, and same for squarebuttons, checkboxes etc, since some need coordinates, some need text, some need text and colors, other need nothing. Also using freetype for text rendering in larger sizes (and the problems of how to get a nice dialogue coping with different sizes).

Offline Max-Max

  • Coder/patcher
  • *
  • Posts: 670
    • MK Development
  • Languages: SV,EN
Re: [Project] GUI Theme
« Reply #489 on: October 22, 2013, 10:51:56 PM »
I think the major problem her is that this "patch" workflow doesn't work for large projects. If patches are to be small, then the project will be submitted in small pieces, where each part might be a temporary state or still in development.

If patches has to be full implementations and reviewed as a finished piece of code, then they become quite large. Either model is working for only one part of us. It is impossible to do both for a larger project.

You can imagine yourself if you have written half your program only to find that the next day someone has rewritten all your code and implemented a different design. That would severely interfere with your planned work ahead...

I think we need to find another workflow to get this project up to speed. I did the mistake to believe a patch could be a working copy of work in progress, but now I understand that you all expect a patch to be a finished patch for code review.

So how do we proceed so the patches doesn't get to big and my work isn't rewritten before I have the chance to finish it myself?