News:

Want to praise Simutrans?
Your feedback is important for us ;D.

[Project] GUI Theme

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

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

TurfIt

Quote from: prissi on August 07, 2013, 11:07:15 PM
I did a lot of work on scr_coord class to remove the tags and make a readable class. Why has this been reverted?

Too many cooks... I'll return to my hole now.

Max-Max

I haven't reveretd anything (what I'm aware of). I can't remember I have seen it as a class. scr_coord.h was missing from the trunk the first time because you had check in the wrong folder, remember? Maybe you checked in the wrong version the second time?

If it was reverted, it should be in the trunk some where. Maybe you can pull it out and check it in again?
- My code doesn't have bugs. It develops random features...

TurfIt

#247
The file moving patch had significant changes to scr_cooord.h - from 55 to 478 lines. I assumed that was intentional as part of the patch. What should this file look like? Also, now I wonder if there's any other stowaway reversion in there... I'd already found the r6616 reversion...

Max-Max

QuotePlease comment what these ALIGN_INTERIOR_... are doing.
The whole alignment is described in the draft I published. I have uploaded rev 47 of the draft (work in progress)
I added a code example with each alignment operation.

QuoteI am not convinced to make gui_frame a part of gui_component. Those are very different, since frame have help files, owner and so on.
If you compare a window with a component they have a lot in common. Both have a size, a position, can own other components, respond on events, create events, be themed to mention some. Several of these are now duplicated code.

some components are unique while some are an aggregation of several components. A window is nothing more than an aggregation of a container, gadget buttons and a label. Then you can add unique features, like a help file.

Most of the gui components do not use inheritance or aggregation and are derived directly from gui_komponente_t instead of nearest existing gui control. Or simply drawn directly instead of being a component.

QuoteAlso using containers for the sole purpose of grouping labels is very hackish.
The purpose of containers are just to group other controls, there is nothing "hackish" about it. What would you use a container for other wise, if not containing other controls?

See the containers as objects, controls inside the container doesn't need to know anything of the outside, nor does the outside need to know anything about the controls inside the container. The minimap dialogue use containers to group the three information sections. Instead of updating each control individually, the whole container can be shown or hidden. One only need to know the containers size, not every control inside it, to recalculate the position of the map area.

So far all controls, regardless of grouping or not, they need to be aware of how their parent control is organised to add margins that lies outside their interest. A client rect solves this! A control only needs to relate to the client rect, passed down by its parent.

A good example is a scrollbox. If the scrollbars are visible the  scrollbox knows this, because it is a part of the scrollbox control. When updating the child controls these has already been subtracted from the client rect that is passed down and the child controls doesn't need to know if they are visible or not. In fact they don't even need to know that they exist at all.

If the scrollbox don't need to show them (scrollbars), the client rect is adjusted before it is passed down. Any child control, pending on the client's area size will automatically claim that area, again without the knowledge of its parent.

By setting the clip rectangle to the client rect, all child controls will automatically be clipped and not overflow the client area.

The client rect also serves as a margin for all children and doesn't need to be the same all the way around. With the current system ALL margins are affected, but sometimes you want a smaller or larger margin, or a margin not affected by the "global" margin.

Since the scr_rect has members we can quickly calculate needed space for a set of controls, expand/contract borders or just move around a group of controls at once.

Remember that the scr_rect only defines a rectangular area, there is nothing magic about it, it doesn't do anything as a component does, it only defines a boundary that can be manipulated.

After digging in the gui for quite a while now, I realise that I need to restructure this first. There are some new controls needed and the last days has been focusing on fixing what is already there. I need a bunch of new components to transform the dialogues in the way I intended where themes are a central part. Because a theme not only redefines images and colours, it also redefines gui element size. We will have a GUI that isn't fixed any more, we can't rely on number of lines, buttons in a row etc. We need the GUI to reorganize itself.

What we do is that we design the GUI in such way that we tell it HOW it can reorganize. Containers, alignment and resizing is an essential key here, because we define the relations, not the absolute positions. From the container we will derive layout controls that takes care of organizing the child controls.

The collapse level is another thing to control how components will be resized. It is basically needed for portable devices or very small screens where we can reduce unimportant gui features in favour for interact able gui controls that needs to be unaffected in size as long as possible (it is explained in the document).

I hope this sheds some lights over what alignment, containers and client rectangles are used for ;)
- My code doesn't have bugs. It develops random features...

Max-Max

I get a compiler error in your trunk because you have renamed the call: client_bound.make_union() to client_bound.merge() without changing the name in scr_rect.

I also noticed that you renamed .calc_client() to .get_min_boundaries(). A boundary is the most outer limit of a control, nothing is drawn outside this boundary. A theme border (it might be a window, a button or other control with a frame) is drawn from the boundary and inwards, so the boundary remains the control's outer limit. The client rect begins next to the themed border, inside the boundry rectangle (the themed border is drawn between the boundry and the client rect). Hence, the child controls are all inside the client rect and the proper function name would be something with client because it referees to the containers client area and returns a rect that is intended for a client adjustment.

I also noticed that you had changed the display dialogue so it now overflows (see attached pictures).
You also removed the .set_children_width(); so all the labels in the container isn't of the same size as the container width (now of some arbitrary size).

If my debug code had been left in there, it had been easy to enable it and spot this.
But I understand your good intention...

I feel more and more the urge of starting the gui restructure (I need the layout controls) and don't want to put too much effort on the dialogues now because the layout controls will handle the most of the child control resize.
- My code doesn't have bugs. It develops random features...

Ters

Quote from: Max-Max on August 08, 2013, 01:19:22 AM
The purpose of containers are just to group other controls, there is nothing "hackish" about it.
I agree.

Quote from: Max-Max on August 08, 2013, 01:19:22 AM
See the containers as objects, controls inside the container doesn't need to know anything of the outside, nor does the outside need to know anything about the controls inside the container. The minimap dialogue use containers to group the three information sections. Instead of updating each control individually, the whole container can be shown or hidden. One only need to know the containers size, not every control inside it, to recalculate the position of the map area.
I wish it was that easy, but I have often found myself in the situation where control inside the container need to know about the outside, or even a control inside another sibling container. This typically happens when you have one big panel made up of smaller sections, where these smaller sections are either reusable, or perhaps made as a container to allows them to be individually hidden. Yet all fundamental controls in the panel should align to the same columns, and the columns' widths is autocalculated from preferred width of each control in every container (which again is based on the length of the text the control needs to show).

prissi

Perhaps I committed from the wrong folder again, since I compiled and test before committing. A mismatched function call would have not been compiled then. I will check at the evening. The SVN seems down at the moment, so I am not sure what is in the repo right now.

The function of the different alignment parameters (if not trivial) should be commented at the definition in the program. And external file is nice, but it bears an even higher risk of being out of date when the code is changed.

Max-Max

QuoteYet all fundamental controls in the panel should align to the same columns, and the columns' widths is autocalculated from preferred width of each control in every container (which again is based on the length of the text the control needs to show).
You forget that the container itself can have logic. I will derive layout objects from the container with the logic to rearrange child objects. A controls size can be set either by client area and have the bounding area to adjust outwards, or set the bounding box and have the client area to adjust inwards. The collapse system can ask a control what size it needs and then make a decision about the layout depending on the needed space.

I know at least one scenario referring to your mentioned problem. If you have a column of buttons it would be nice the have a weighted centre alignment of the button's text, depending on the button with the widest text. Because the container can collect the information from each button it can then make a decision and notify the children about any layout adjustments.
- My code doesn't have bugs. It develops random features...

Max-Max

Quote from: prissi on August 08, 2013, 12:22:43 PM
Perhaps I committed from the wrong folder again, since I compiled and test before committing. A mismatched function call would have not been compiled then. I will check at the evening.
I always count on that every one is doing their best and we all do honest mistakes sometimes... No big deal :)

Quote from: prissi on August 08, 2013, 12:22:43 PM
The function of the different alignment parameters (if not trivial) should be commented at the definition in the program. And external file is nice, but it bears an even higher risk of being out of date when the code is changed.
I will see what I can do :)
- My code doesn't have bugs. It develops random features...

Ters

Quote from: Max-Max on August 08, 2013, 12:42:33 PM
You forget that the container itself can have logic. I will derive layout objects from the container with the logic to rearrange child objects. A controls size can be set either by client area and have the bounding area to adjust outwards, or set the bounding box and have the client area to adjust inwards. The collapse system can ask a control what size it needs and then make a decision about the layout depending on the needed space.

I know at least one scenario referring to your mentioned problem. If you have a column of buttons it would be nice the have a weighted centre alignment of the button's text, depending on the button with the widest text. Because the container can collect the information from each button it can then make a decision and notify the children about any layout adjustments.

As I understand this, then you are no longer using containers to wrap a group of controls that should be hidden and displayed together, which was your argument for having containers. There is just one container level with everything, that uses another mechanism for grouping controls. I did not forget that containers have logic, typically a layout manager, but this manager needs to know internal state of other containers (or perhaps rather their layout manager) to work sometimes. Unfortunately, I have never seen any that are able to do this.

The only solution I've heard (for Java), is to not make nested containers, but builders of sorts. You only make one window/dialog/panel, set up a global layout, and then recursively call the builders, which are passed offsets so that they put their controls (and sub-builders) at the correct place in the layout. However, although the nested builder is ignorant about the layout it is put into, parent builders must know how the builders they nest arrange their controls.

prissi

Since the SVN is still down, here the missing changes.

Max-Max

I will wait for the SVN server to get back up. I don't want to risk messing up things when applying a patch... Does anyone know why it's down?

I'm on a 3 day music festival again (back in Sweden) :)
- My code doesn't have bugs. It develops random features...

Leartin

I'm sorry to intrude into a thread I don't understand at all, but I realized something in the nightly which probably has to do with the GUI overhaul - the active toolbar now has an orange border. With the new toolbar design we made for pak192.comic recently, that looks really ugly, as you can see on the attached picture. I'd like to ask if this border is something that can be removed by pakset settings later (or already), or is here to stay, as in the latter case we'd redo the design to be a rectangle again. Thank you in advance :)

Max-Max

It's configurable under "settings" -> "Window frame active"
or in the simuconf.tab file -> "window_frame_active", set it to 0.
- My code doesn't have bugs. It develops random features...

Max-Max

I just want to give you guys a head up on what's going on.

I haven't been so productive the last weeks due to the unresponsive SVN server and then I got sick, just bad timing. Anyway I doing some "thinking" around the theme manager and experimenting a bit.

I have not digged beyond the call to display_image() and have no clue on how each image is stored internally or how the colour mapping is done.

When we switch theme I need to unload all theme images and load the new theme images. Can some one please explain in the big context of how this works?
- My code doesn't have bugs. It develops random features...

kierongreen

Look at simgraph16.cc. I would suggest changing register_image(struct bild_t* bild) and adding new functions replace_image(unsigned bild, struct bild_t* new_bild) and register_image(struct imd* image). You'll also need to load the new images from pak files or whatever so that there's a valid struct bild_t* new_bild.

Code not tested:

void register_image(struct bild_t* bild)
{
   struct imd* image;

   /* valid image? */
   if(  bild->len == 0  ||  bild->h == 0  ) {
      fprintf(stderr, "Warning: ignoring image %d because of missing data\n", anz_images);
      bild->bild_nr = IMG_LEER;
      return;
   }

   if(  anz_images == alloc_images  ) {
      if(  images==NULL  ) {
         alloc_images = 510;
      }
      else {
         alloc_images += 512;
      }
      if(  anz_images > alloc_images  ) {
         // overflow
         dbg->fatal( "register_image", "*** Out of images (more than 65534!) ***" );
      }
      images = REALLOC(images, imd, alloc_images);
   }

   bild->bild_nr = anz_images;
   image = &images[anz_images];
   anz_images++;

   register_image(bild, image);
}

void register_image(struct bild_t* bild, struct imd* image)
{
   /* valid image? */
   if(  bild->len == 0  ||  bild->h == 0  ) {
      fprintf(stderr, "Warning: ignoring image %d because of missing data\n", anz_images);
      bild->bild_nr = IMG_LEER;
      return;
   }

   image->x = bild->x;
   image->w = bild->w;
   image->y = bild->y;
   image->h = bild->h;

   image->base_x = bild->x;
   image->base_w = bild->w;
   image->base_y = bild->y;
   image->base_h = bild->h;

   image->len = bild->len;

   // allocate and copy if needed
   image->recode_flags = FLAG_NORMAL_RECODE | FLAG_REZOOM | (bild->zoomable & FLAG_ZOOMABLE);
   image->player_flags = NEED_PLAYER_RECODE;

   image->base_data = NULL;
   image->zoom_data = NULL;
   image->data = NULL;
   image->player_data = NULL;   // chaches data for one AI

   // since we do not recode them, we can work with the original data
   image->base_data = bild->data;

   // does this image have color?
   if(  bild->h > 0  ) {
      int h = bild->h;
      const PIXVAL *src = image->base_data;

      do {
         src++; // offset of first start
         do {
            uint16 runlen;

            for (runlen = *src++; runlen != 0; runlen--) {
               PIXVAL pix = *src++;

               if (pix>=0x8000  &&  pix<=0x800F) {
                  image->recode_flags |= FLAG_PLAYERCOLOR;
                  return;
               }
            }
            // next clear run or zero = end
         } while(  *src++ != 0  );
      } while(  --h != 0  );
   }
}

void replace_image(unsigned bild, struct bild_t* new_bild)
{
   if(images[bild].zoom_data!=NULL) {
      guarded_free( images[bild].zoom_data );
   }
   if(images[bild].player_data!=NULL) {
      guarded_free( images[bild].player_data );
   }
   if(images[bild].data!=NULL) {
      guarded_free( images[bild].data );
   }
   register_image(new_bild, image);
}


If the number of theme images isn't fixed then you would probably have to reload all pak images as well - you'd be as well off restarting simutrans.

Max-Max

So far we have decided that theme related images will be in its own single pak-file, so there will not be any need to reload other pack files when changing the theme.

Why are we "registering" each image? Is it here we unpack them from the PAK file format? (recode?)
Why are the registered images stored with realloc and not in a std::vector or other stdlib container (or is this code from the C version of Simutrans)?
- My code doesn't have bugs. It develops random features...

kierongreen

When you load a pak file you will end up with a number of besch structures. These will contain image data however this then needs to be registered with the drawing routines. Look at the code that runs when simutrans starts up to see how this works.

note that it is REALLOC not realloc. Simutrans doesn't use some std functions because they weren't around when it was first coded, or are still not available on some platforms. This might be one.

I realise it's going to be in one pak file but depending on assumptions regarding images and so on you might have difficulties just reloading one pak.

Ters

Reloading pak files is not something Simutrans supports. I think it would be better to get theming working before starting on implementing reloading pak files while a game is running. There are a couple of potential gotchas.

Max-Max

Okay, I'm well enough to do some coding and I have updated everything and can now have a look at the scr_rect that Prissi rewrote completely. I thought we would discuss things before we change the whole design of some one's else work?

I do remember that you made a comment before regarding left,right,top,bottom instead of x,y,w,h. I agree that x,y,w,h makes more sense for a geometrical figure such as drawing a rectangle. but if scr_rect is used as a padding or margin, x,y,w,h makes no sense at all.

In my original design left,right,top and bottom where all public because updating them doesn't need to recalculate something. This gives fast and easier access to them.

Because height and width was a function call, they where calculated when needed from the left,right,top,bottom variables and always gave a correct result.

There are countless of places where we add get_pos().x and get_groesse().x to get the absolute position (right edge of the rectangle) and working with my original design makes the code less cluttered because we already have that information in the rightmember.

compare the trunk code...
foo.get_bounds().get_pos().x + foo.get_bounds().get_width()

with my original design...
foo.get_bounds().right

Can we describe a rectangle, margin, padding, clip region with only x,y,w,h? Well, we can but it makes absolutely no sense for margins or padding.

Can we describe a rectangle, margin, padding, clip region with only left,right,top,bottom? Well, we can and it does make sense in the cases (you can describe a rectangle with left,right,top and bottom positions).

We need a getter and setter to access the bounding box, client area, margins etc... for a component because the component needs to be aware when these are changed. But scr_rect doesn't need any getter or setter because it doesn't need to be aware if any of the left,right,top,bottom (or x,y,w,h) variables changes.

I included the get/set_width() and get/set_height() to for easy access when needed, as helper functions.

Further, all setters are calling normalize() and that makes sense if you want to draw a rectangle, but not if you actually want a negative offset, such as in a margin or padding. A negative result can also be used to check if something is before or after the scr_rect.

Why do we need to mark a scr_rect as invalid? Just hide the default constructor if you want to force a size on creation. However a margin or padding would make a lot of sense to be initialised to 0, but is still valid.

Your version of expand doesn't work in the same way as my original version. Your version works as my add_offset (which you have removed).

make_union() is renamed to merge() and I did put some thought behind this. Merging two rectangles would result in a non rectangular shape (unless both rectangles are of exact same shape and location). After more thinking it would probably be more suitable with a name such as bound() because it creates the bounding box around two rectangles.


If you had asked me how I intended to use it, started a discussion before you rewrote almost everything I did, we could have saved us both some work.

The scr_rect was planned to be used as bounding box, client area, padding (inside client area) and theme clip frames. We can easily add some display_xxxx functions accepting a scr_rect as a parameter as well.

I can do the job to restore the class to function as I intended (with left,right,top and bottom, or just short l,r,t,b if you think it suits better). I will try to keep as much of your code as possible, but I need to know if you are going to rewrite it again if you don't like it...

But, I do see some good optimisation in some places, good work there :thumbsup:
- My code doesn't have bugs. It develops random features...

prissi

Well, the only reason to use OOP instead of structures is to hide private members. At least this is the proper way. get_rest().right is against the spirit of simutrans (at least as originally concieved) to be able to change internal later.

As such it is also important to initialise rectagles properly and avoid use with a default behavior. A private constructor would require to pass all rectangles as pointers, and forbids also the use of arrays of reactagles (or similar structures).

However, if you want to do further optimisations, you can also add a bottom right to this structure, and return this by get_right() That is the beauty of OOP, isn't it?

About the functions: Aparently I misunderstood the purpose of expand from the name. The merge is properly rather split into merge_outline and merge_overlap.

Rectangles should not be used for margins: By definitions a position will not make sense for margins. As points will be still used for offsets, I see no wil be introducing a new class for margins. Rather more important the margins should be stored in variables containing the word "margin" ...

That my input so far. I will be still away from home until the 5th, so do not expect fast responses until then. Sorry.

Max-Max

#266
The original design was a type with helper functions operating on the type, it doesn't need to be a class more than an int or a double does.

As I said, there is no need for private members because nothing needs to be updated when these variables change. Since there was no need for private members I originally designed it as a struct, but because of the member functions, you wanted it to be a class. I'm fine with that, but just because it is a class, it doesn't need to have private members.

If the class needs to be aware of access to one of its members, getter/setters are used, but if there is no need for this it is perfectly all right to make the member public without getter/setters. Just because it is a class doesn't mean that you have to use getter/setter (if that was the case there makes no sense to be able to declare public variables to begin with).

The use of getter/setter creates a load of unnecessarily function calls because scr_rect doesn't need to be aware of access to the variables. This adds up to unnecessarily CPU time because we do this several times every frame. You might say that it is so little that it doesn't matter, but you could use that time on other things instead, such as improved graphics, animations, more complex AI, path finding etc... All small things adds up in the end...

One of the main functions of a constructor is to initialise the object, I have never, in my whole career, meet someone that thinks this is a bad idea.
If you really don't want to have a default initialisation, I proposed to hide the default constructor so you would be forced to use another public constructor accepting parameters for initialization. On the other hand, if the default is 0,0,0,0 (as it was in my original design) you could use is_empty() to check if it is initialized/invalid.

Your new design initialize x to an integer value to mark an "invalid" rectangle. This doesn't work because some setters use the x value to calculate internal stuff, resulting in really screwed up rectangles. A rectangle with width or height 0 isn't really a rectangle, it's a line or a dot, so is_empty() would work fine to check if it is invalid.

Defining a rectangle by its sides isn't something I have just grasped out of the blue. It is a proven concept used in Win32, OWL ,VCL, FireMonkey, DirectX,  and Android to mention a few. The benefit of using the sides is just that it can be used to anything with 4 sides, such as a rectangle, margin, padding, clipping etc...

Some frameworks also define a position and a size type, both having the same content, but under different names; position(x,y), size(width,height). I did suggested a separate size type before and got the answer that a scr_coord did the same thing and in this case there is no problem to call it x,y instead of width,height.

Since you are not concerned by all the function calls to access the members, one way would be to use my original design (as a class if it feels better, with left, right, top and bottom as public members). Then we can add a get_pos() and set_pos(). Now you can still use foo.get_pos().x and I will get my direct access to left, right, top and bottom.

I have used your class in my theme_manager to see how it is to work with (I thought I would give it a try), but it got complex to do some very simple operations:

void foo (scr_rect frame, bild_t part) {

scr_coord(frame.get_pos().x+frame.get_width()-part.w,frame.get_pos().y+frame.get_height()-part.h);

or
scr_coord(frame.get_bottomright().x-part.w,frame.get_bottomright().y-part.h);

compared to my original design...

scr_coord(frame.right-part.w,frame.bottom-part.h);
}


If we define margins and padding with 4 members, we can't pass these definitions as a single parameter, or use them in operations on rectangles (bounding boxes), especially if you want to access all 4 of them through getter and setters (in the spirit of Simutrans).

A margin declared as:

scr_rect Margin;

can be accessed by:

Margin.left;
Margin.right;
Margin.top;
Margin.bottom;


...and this makes a lot of sense.


What exactly is the spirit of Suimutrans? To not change anything? To code C with a C++ compiler? To always code classes just because it is a C++ compiler? I really don't get the logic sometimes in how you reason regarding the "spirit of Simutrans".

Should we refuse a solution just because it isn't the "spirit of Simutrans"? How can we thinking outside the box if we are restricted to "this is how it always has been done"?
- My code doesn't have bugs. It develops random features...

Ters

Quote from: Max-Max on August 27, 2013, 01:24:01 AM
The use of getter/setter creates a load of unnecessarily function calls because scr_rect doesn't need to be aware of access to the variables. This adds up to unnecessarily CPU time because we do this several times every frame. You might say that it is so little that it doesn't matter, but you could use that time on other things instead, such as improved graphics, animations, more complex AI, path finding etc... All small things adds up in the end...

Although it is more typing, always using methods is the safest bet. There is after all a reason getters and setters are recommended. While it doesn't seem to matter for a plain old simple, stupid rectangle, in the future, it might not be so simple or stupid. Maybe something has proven that it's a good idea to have a rectangle interface, with several implementations. Maybe you want a rectangle that validates whether it's coordinates are within some bounds.

Getters and setters that are defined in the header file will be inlined, ending up with the same machine code as a direct member access (if that's all the setters and getters do).

Quote from: Max-Max on August 27, 2013, 01:24:01 AM
What exactly is the spirit of Suimutrans? To not change anything?

That goes for every system with lots of users and low capacity for maintenance. The current trend in software development methology encourages throwing old code aside, writing and rewriting code, and continously pushing these changes into production, all with impunity. However, all this depend on things like test driven development and CI servers running unit tests on every single commit. Simutrans has nothing of this. There is no safety net. A screw-up can go unnoticed until hundred player have had their save games corrupted.

prissi

#268
Simutrans was concieved as a teaching project for OOP. Furthermore, one of the priorities is stability before speed. Therefore the rule of get/set functions, as Ters described. You could put the same argument onto almost any other private variable, which is not overlaid.

The are several tastes of OOP. For simutrans the style of mostly private members and get/set access was the one where it finally settled. Even more, since a simple access is without an penalty in speed, but allows for very easy debug checks (which helped a lot). And rectangle are very unlikely performance critical compared to the actual drawing.

And believe it or not: I can understand your thinking; I came without any OOP experience to Simutrans (and a lot of my code still tells of this). But slowly I got convinced of some OOP features, like get/set. The same is with an implicit initialisation with invalid state of stuff. Like pointers with zero, arrays without any members and so on. Lazy initialisation is a recipe for errors later on. If you want a rectangle then use it as a rectangle. Using it as an array of four margins is not a rectangle object. OOP (object with methods) is violated. It is very C-ish, but margins are not reactangle, they are four offsets.

And my understanding of a rectangle a geometric object: A rectangle is defined by the length of two sides, which both have to be positive and 90° to each other. Cornerpoints does not define a rectangle without an implicit rotation, and negative areas of rectangles do not make sense. (Just look at wikipedia.)

Max-Max

#269
The OOP purists, usually fresh students applying all the text books and professors without any real world experience propagate for using all the OOP feature just because in theory it looks nice. Under my 20 years experience of real world OOP I discovered that you need to use some common sense and apply OOP where it does something good, not because you can. It is not practice to always use setter/getter, there are two sides of it, the purist that do it of Principe no matter what, and the other side that use it when needed.

I don't see why code would be more buggy without a getter/setter if it doesn't do anything else than just pass a variable.
Something that produces buggy code is to have several getter/setter for the same variable (functions that read/write directly to the private member). If you need to update the behaviour to a set operation, you also need to go through the whole class to make sure no one else is using that variable directly. In these cases there should only be ONE getter/setter that also is used internally. Then you only need to update one pair of functions.

With your arguments, why does scr_coord, KOORD or other classes with public members not have getter/setters? Why do we have structures at all and not classes for everything with private members with getter/setter?

I proposed a size type before and got the answer that we have KOORD (scr_coord), but why are scr_coord used for size? a size is a width and a height, not x,y point (to refer to your own arguments for a rectangle only being a geometric figure and nothing else). So why is it that we in some cases use OOP even if it isn't really needed and in some cases we don't?

Regarding the use of types, you have to think outside the box and not hang up non nomenclature. My original rect type described 4 sides. This can be used for anything with 4 sides, like a margin inside a rectangle. Being of the same type you can easily adjust an rectangle with a margin object. It makes sense and feels natural to be able to subtract a margin from a bounding box.
- My code doesn't have bugs. It develops random features...

Ters

We're not arguing that code is more buggy without setters and getters. At least I am not. However, with direct access, you can't easily add code to be run every time a field is written or read. It also gives you a consistent interface, whether you're asking for left edge, right edge or width, no matter which two of them are actually stored.

As for existing code, some of it was written by people who didn't know OOP yet, as prissi admitted. They were coding C in C++. Knowing how to code Simutrans is difficult, as you have to know which code was written after the developers figured out how to write it.

As for types, a rectangle has four sides, and a margin (for a rectangle) has four sides, but a margin is not a rectangle. Also, a rectangle plus or minus another rectangle is only in special circumstances another rectangle. Simutrans code is hard to read as it is, with its mixture of languages. With scr_coord we tried to make things clearer, but using rectangle for margins feels like two steps back after one step forward. You need to operate differently on a "rectangle" depending on the context.

Max-Max

But size is store in a scr_coord wich is a point, not two sides, so you aren't consistent in your argumentation.

You are using the name of the declared variable, not the type.


I think

scr_rect Margin
Margin.left
Margin.bottom

...is very clear of what is does.

you are not writing scr_rect.left to get left margin ;)
- My code doesn't have bugs. It develops random features...

Ters

Quote from: Max-Max on August 27, 2013, 05:28:34 PM
But size is store in a scr_coord wich is a point, not two sides, so you aren't consistent in your argumentation.

Have I argued for using scr_coord for size? But I must say I'm not 100 % sure what coordinate even means. I've always though of it as having to do with any numeric value along an axis (and whatever the equivalent term is in spherical coordinate systems), but according to Wikipedia, the definition is a bit narrower.

Quote from: Max-Max on August 27, 2013, 05:28:34 PM
You are using the name of the declared variable, not the type.

I think

scr_rect Margin
Margin.left
Margin.bottom

...is very clear of what is does.

you are not writing scr_rect.left to get left margin ;)

You don't always have the informatively named variable's name at hand. Inside member functions, you only have the meaningless name "this". You also don't get compile errors when passing a margin to something expecting a "rectanglerectangle" and vice-versa, or if you mix up the parameters to a function accepting one of each. I prefer having errors brought to light as soon as possible, not after starting the game and performing lots of interactions to bring me to the part I was coding on.

Furthermore, I could also interpret scr_rect Margin as the rectangle formed by each of the four margins. It certainly doesn't come naturally to me to see the four values (left,right,top,bottom) as insets rather than coordinates (by Wikipedia's definition of it).

The cost of introducing all these types to maintain strict typing is high here and now, while the costs due to confusion caused by types that can be used for anything remotely similar is low. But the latter sums up over a very long time.

It also becomes ambigous what one means when saying/writing rectangle. Does it mean scr_rect or "rectanglerectangle"?

Max-Max

I didn't say that you (Ters) was against it. I meant it was down voted when I brought up the question in this thread.

...and if we are going to look up everything on Wikipedia, nothing will make sense in the code... ;)
- My code doesn't have bugs. It develops random features...

Ters

Quote from: Max-Max on August 27, 2013, 06:16:45 PM
...and if we are going to look up everything on Wikipedia, nothing will make sense in the code... ;)

I'm OK with that, for coordinate at least.

prissi

We should avoid to be stuck in details so much ...

Lets conclude: scr_coord is probably not the best name choice when it comes to extend/sizes. Rect is probably not really margins.

Lets make three types: scr_coord, scr_extend (or named scr_size), and scr_rect, which is made from scr_coord and scr_extend and always have an extend >=0. conversions between scr_coord and scr_size would be automatically.

margins would be derived from extends, and could be positive, negative or whatever obviously.

It would allow for invalid rects, sizes, and coords, as well as keep those types properly for their intended purpose. Declaring something like resize( scr_size new_size ) seems quite straight forward.


prissi

I had some ideas on drag working for rail construction on tablets: i.e. drag updated via click and building via doubleclick as simuconf.tab option. That could make simutrans more playable on tablets.

TurfIt

SDL2 appears to have some sort of support for touch/gesture events. They're disabled in the current testing SDL2 backend, but might be worth investigating if someone wants better tablet support...

Max-Max

Some project update info!

I have created a theme manager that prepares all the GUI images. In this first version it assembles the current skin-PAK images into the 9 needed images (theme parts) for drawing a rectangle. These new images are registered in the system with register_image().

The theme manager also have a member function theme_frame(scr_rect, theme_id) to draw a rectangle with the requested theme component (theme_id).

So far it looks promising, but I have to handle if the theme parts (9 images for a specific theme id) don't have the same size. I will do this by extending them  until they line up with each other.

Of course not all themes are rectangles, such as symbols or icons. These only have one Theme part (or as many they need). Whit this said, not all Theme ID have 9 Theme Parts...

The thought is that the Theme manager also will fill in missing images so we always have images for all theme components. This can be done by a number of fall backs. First we can let several Theme ID share the same set of Theme Parts and as a last resort just create a new very simple image as replacement. But this will come further down the road...

Definitions:

Theme ID
A specific theme component as a button, check box, gadget, label etc...
The Theme ID is a constant (enum) to identify a specific GUI control, such as THEME_ELEMENT_BUTTON_UP, THEME_ELEMENT_BUTTON_DOWN, THEME_ELEMENT_CHECKBOX, THEME_ELEMENT_CHECKBOX_CHECKED etc...

Theme Part
To draw a rectangle such a button, 9 images are needed; TopLeft,Top,TopRight,Left,middle (face), Right,BottomLeft,Bottom,BottomRight. One of these images is a Theme Part.
- My code doesn't have bugs. It develops random features...

Ters

Quote from: Max-Max on August 31, 2013, 05:38:15 PM
So far it looks promising, but I have to handle if the theme parts (9 images for a specific theme id) don't have the same size. I will do this by extending them  until they line up with each other.

Just let it fail fataly for now, with a understandable message. I more desire to see this working in all it's glory, on the assumption that every input is correct. Besides, Simutrans isn't know for correcting the artists' mistakes. Maybe the correct adjustment would have been to crop rather than extend anyway.