Skip to main content ↑ ↓
Topic: [patch] sensible limits in the for-loops in simview.cc (Read 11264 times) previous topic - next topic

[patch] sensible limits in the for-loops in simview.cc

The limits in the for loops depend miraculously on unrelated variables: the upper limit of the for-loop over all rows depends on the screen width (and vice-versa). The lower limits of the loops are way too small, which means that the loops make many unnecessary iterations.

This patch corrects this, effectively reducing the steps in the loops by 60-70%

Please test! Maybe the patch does not work with high bridges, mountains, deep holes, underground view etc.

As bonus, the patch allows buildings up to a height of 4 x imagesize (eg 4 x 128 for pak128). That means for instance, that the TV tower in pak96.comic is displayed correctly.

Edit: Reuploaded the patch file, it contained lots of unrelated changes accidentally.
Parsley, sage, rosemary, and maggikraut.

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #1 –
Accidentally (?) this was committed with revision 3561. So lets wait for bug reports 8)
Parsley, sage, rosemary, and maggikraut.

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #2 –
I have tested it slightly and found that when zooming out the main view, left margin is sometimes not updated correctly (drag the map may fix it though). Please see the 1st attachment. It's easier to reproduce with Pak128 but harder with Pak64. Probably something related to tile raster width.

There is an existing problem (which exists before you patch) in the underground mode which you may want to fix as well. Please see the 2nd attachment. Sometimes part of the underground road isn't drawn. But usually only happens with roads built deeply underneath very high mountains.

Good news is, your patch fixes this problem : http://forum.simutrans.com/index.php?topic=4581.0. Excellent ;)

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #3 –
Quote from: Knightly – on
I have tested it slightly and found that when zooming out the main view, left margin is sometimes not updated correctly (drag the map may fix it though). Please see the 1st attachment. It's easier to reproduce with Pak128 but harder with Pak64. Probably something related to tile raster width.
If dragging fixes this, then it sounds like it is another problem. Maybe the dirty-status was not correct after zooming? Do you have a savegame, where zooming reliably yields to this error?

Quote
There is an existing problem (which exists before you patch) in the underground mode which you may want to fix as well. Please see the 2nd attachment. Sometimes part of the underground road isn't drawn. But usually only happens with roads built deeply underneath very high mountains.

Good news is, your patch fixes this problem : http://forum.simutrans.com/index.php?topic=4581.0. Excellent ;)
No, this is not fixed. At least it happens for maps with sea level = -10. :(

I think to fix these issues, one has to keep track of min/max heights along vertical and horizontal lines ( x+y and x-y constant)...
Parsley, sage, rosemary, and maggikraut.

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #4 –
Quote from: Dwachs – on
If dragging fixes this, then it sounds like it is another problem. Maybe the dirty-status was not correct after zooming? Do you have a savegame, where zooming reliably yields to this error?

It's reproducible in any game, and does not depend on particular save games. Please try to create a 512x512 map with Pak128, then try to zoom max in and zoom max out at different spots, and eventually you will encounter the problem. BTW, it's not always in the left margin. I encountered the same problem once in the right margin. And you are right, probably the dirty-status is not set as when I move the cursor to the problem region, the correct ground under the cursor shows up.

Quote from: Dwachs – on
No, this is not fixed. At least it happens for maps with sea level = -10. :(

I think to fix these issues, one has to keep track of min/max heights along vertical and horizontal lines ( x+y and x-y constant)...

In my case it happens when a road at height 4 is built beneath a mountain of height 13. The problem is in karte_ansicht_t::display() :

Quote
   const grund_t *gr = plan->get_kartenboden();
   .
   .
   .
   if(yypos-IMG_SIZE*2<disp_height  &&  yypos+IMG_SIZE>menu_height) {
      plan->display_dinge(xpos, yypos, IMG_SIZE, true, hmin, hmax);
   }

When checking whether the tile is off-screen, the check is based on the natural ground. But a grid square may have grounds at different heights. Thus, instead of checking it here in karte_ansicht_t::display(), we should move the check inside planquadrat_t::display_dinge() and check against each ground.

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #5 –
I think I found the reason for thos graphic errors: the values of karte_t::x_off and y_off are usually between -raster_wdith and +raster_width (and -rw/2 and +rw/2 respectively). When zooming out, these values are not updated, and go way beyond the limits.

You must have fixed a similar problem in your viewport pathc. Did I got it right, that you simply rescaled these offsets (called fine_offset in your patch) ?

Edit: does this patch cure the graphical errors in zoom-out for you?
Parsley, sage, rosemary, and maggikraut.

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #6 –
Quote from: Dwachs – on
I think I found the reason for thos graphic errors: the values of karte_t::x_off and y_off are usually between -raster_wdith and +raster_width (and -rw/2 and +rw/2 respectively). When zooming out, these values are not updated, and go way beyond the limits.

You must have fixed a similar problem in your viewport pathc. Did I got it right, that you simply rescaled these offsets (called fine_offset in your patch) ?

Yes, I have a separate function for this purpose :

Quote
void karte_ansicht_t::recalc_fine_offset(const sint16 old_raster_width)
{
   const sint16 new_raster_width = ( is_rezooming() ? get_tile_raster_width() : get_base_tile_raster_width() );
   // need to rescale the existing offsets
   set_fine_offset( koord( (sint32)get_fine_offset().x * new_raster_width / old_raster_width,
                     (sint32)get_fine_offset().y * new_raster_width / old_raster_width ) );
}

In my viewport patch, there are different offsets so that they can vary separately depending on situation/user actions.

BTW, thanks for committing this patch. When I was working with the viewport patch and got stuck with the "invisible tile" problem (which is fixed by your patch as mentioned above), I wanted to change those loop limits too. But I wasn't able to understand why those limits were like that, and thought I was not smart enough to figure out the logic/rationale of such limits. You have cleared my bewilderment now -- a trillion thanks!! ;)

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #7 –
Just to clarify:

(1) prissi comitted my patch 8)
(2) it does not fix the problem with too high mountains and too deep holes!

And I think one can simplify the logic in simview a lot by pre-computing and storing the loop-limits....
Parsley, sage, rosemary, and maggikraut.

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #8 –
Oh, I thought you committed it (as you said "accidentally"). I don't know which problem is "the problem with too high mountains and too deep holes", but the "invisible tile" problem is caused by high mountains and your patch does fix it.

I have tested your patch and seems that the problem is gone (though not 100% sure). Since those offsets need to be re-scaled upon rezooming, it has to be committed anyway IMHO.

Quote from: Dwachs – on
And I think one can simplify the logic in simview a lot by pre-computing and storing the loop-limits....

Sometimes I want to bring the viewport patch up-to-date with the latest revision. But there are so many changes to the codebase that I need to review everything all over again...

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #9 –
Quote from: Knightly – on
Oh, I thought you committed it (as you said "accidentally"). I don't know which problem is "the problem with too high mountains and too deep holes", but the "invisible tile" problem is caused by high mountains and your patch does fix it.
I created a map with sea level -10 and the invisible tiles problem is still present (as in those thread you linked).
Quote
I have tested your patch and seems that the problem is gone (though not 100% sure). Since those offsets need to be re-scaled upon rezooming, it has to be committed anyway IMHO.
you mean the patch for scaling the offset?
Parsley, sage, rosemary, and maggikraut.

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #10 –
Quote from: Dwachs – on
I created a map with sea level -10 and the invisible tiles problem is still present (as in those thread you linked).

Oh, you are right. I didn't test with such low sea level.

Quote from: Dwachs – on
you mean the patch for scaling the offset?

I mean 3561-correct-offs.diff which you have posted earlier on.

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #11 –
The drawing routines on simutrans will generate glitches with depth below -4 and heights above 14. Make a very rough map and built a town on a tile with height 16. The skyscraper will vanish before even reaching the lower border. And I actually accidently commited this patch, since I was too tired yesterday (and still with 38C fever.)

Maybe after moving the origin the map will redraw a much larger region, then tracks the highest and lowest tiles and afterwards smartly calculate the needed starting row to display everything nicely.

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #12 –
I committed the patch for scaling the offsets.

@prissi: I created such a map (sea level 0), max height of mountains 20, and got such graphical errors for tiles at height >=19. And: get well soon :)

To fix this bug, I thought of keeping track of min/max heights along horizontal lines. And after zooming and scrolling the visible area (the bounds for those for-loops) is calculated (and the results are as long as they are valid).
Parsley, sage, rosemary, and maggikraut.

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #13 –
@Dwachs

It seems that the margin problem has not been solved completely. Loading the save game <a href="http://files.[ simutrans [dot] us (site down, do not visit) ]/files/get/ouGOTHolFj/margin-problem.sve">here[/url] should trigger the problem on the upper margin of the main view; if the problem does not show up right away, please click on the monument to open its info dialog, and click on the mini world view to re-center the map, then the problem should appear.

Hope this helps.

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #14 –
yes, there is a related bug report on the German forum. And I found another misbehavior: after maximizing the window, something like 5 or 6 rows from below are not displayed properly (the pointer is there but nothing else).

Edit: at least the problem in your savegame should be fixed in rev 3589, please test.
Parsley, sage, rosemary, and maggikraut.

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #15 –
Thanks for looking into the problem. :) However, with the same save game, there is still a half row at the top margin that is not displayed correctly. Please see the attached image.

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #16 –
cannot reproduce this :(

could you please print the values of these variables:
Code: [Select]
	const sint16 disp_width = display_get_width();
	const sint16 disp_real_height = display_get_height();
	const sint16 IMG_SIZE = get_tile_raster_width();
	const int i_off = welt->get_world_position().x - disp_width/(2*IMG_SIZE) - disp_real_height/IMG_SIZE;
	const int j_off = welt->get_world_position().y + disp_width/(2*IMG_SIZE) - disp_real_height/IMG_SIZE;
	const int const_x_off = welt->get_x_off();
	const int const_y_off = welt->get_y_off();
Parsley, sage, rosemary, and maggikraut.

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #17 –
It's not as easy to reproduce the problem this time, but I have found how to trigger it :

1> Open Simutrans but do not maximize the window
2> Load my save game
3> Click on the tourist attraction to open its info dialog
4> Click on the mini world view to re-center the map
5> Maximize the window
6> Click on the mini world view again
7> Minimize the window
8> Problem appears in the top margin

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #18 –
This happens only on areas where the outside-graphics should be displayed?
Parsley, sage, rosemary, and maggikraut.

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #19 –
Not sure, but can you reproduce the problem with my procedure?

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #20 –
Could reproduce it. Patch attached. The patch first computes worst-case lower bound for y-coordinates, then adapts the lower bound if nothing is displayed. Should fix the problems with very low sea-level (e.g. -8 or something).

Still a very flexible/dynamical approach to adapt the y-bounds in the loops is needed. These bounds only needs to be recomputed after zooming/scrolling etc.
Parsley, sage, rosemary, and maggikraut.

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #21 –
With your patch, the problem no longer appears. Thanks :)

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #22 –
Problem appear in pak128 oepening screen. Just scroll the water, so that a little less of a half tile is seen; all those less than half tiles are not redrawn.

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #23 –
Quote from: prissi – on
Problem appear in pak128 oepening screen. Just scroll the water, so that a little less of a half tile is seen; all those less than half tiles are not redrawn.
Does this happen with or without the patch applied? I cannot reproduce it with the patch.

It is not committed yet.
Parsley, sage, rosemary, and maggikraut.

Re: [patch] sensible limits in the for-loops in simview.cc

Reply #24 –
Ok, I am sorry, then it should go in as it fixes this bug.