OpenTTD

Tasklist

FS#54 - smallmap zoom-in

Attached to Project: OpenTTD
Opened by thomasdev (thomasdev) - Monday, 13 February 2006, 11:46 GMT
Last edited by fonsinchen (fonsinchen) - Saturday, 01 March 2014, 14:13 GMT
Type Work in progress
Category Interface
Status Closed
Assigned To Celestar (Celestar)
Operating System All
Severity Medium
Priority Low
Reported Version trunk
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 4
Private No

Details

changes:

window.h:
*added structure for scroll-event (line 137)
*corrected bug in line 386
*added WE_SCROLL AND WE_MOUSEWHEEL events (line 507)

window.c:
*rewritten the function HandleViewPortScroll
==>now creates an event WE_SCROLL that is send to the client, instead of scrolling self ==>client is responsible to handle the scroll

smallmap_gui.c
*included debug.h
*based on lucapillars patch, rearranged the widgets
*included zoom support, etc etc
*scrolling now works correctly when zoomed in

you can contact me under this mail: ottd _at_ thomasfischer.biz
This task depends upon

Closed by  fonsinchen (fonsinchen)
Saturday, 01 March 2014, 14:13 GMT
Reason for closing:  Out of date
Additional comments about closing:  Those patches definitely won't apply anymore today. If we want smallmap-zoom-in we have to start from scratch
Comment by thomasdev (thomasdev) - Monday, 13 February 2006, 12:53 GMT
fixed all spaces and tabs and reduced the patch-size
hope this is better :-D
Comment by thomasdev (thomasdev) - Monday, 13 February 2006, 16:59 GMT
+added mousewheel-zooming
+added vehilce draw if close enough
Comment by thomasdev (thomasdev) - Monday, 13 February 2006, 20:52 GMT
+added restricted vehicle draw if zomed out
+added zoom indicator on the lower left
+added industry strings on the map if zoomed in
+added town population if near enough
Comment by Patric Stout (TrueBrain) - Monday, 13 February 2006, 23:37 GMT
I gave it a quick look (sorry, again quick). Some minor things I noticed:

There is a back lack of spaces. Don't do: if (a==b), but if (a == b). See Wiki for Coding-Style.
There is a very inconsitent usage of comment-style. We mostly use /* */.. you mix it :)
You still have adds of empty lines in your patch ;) Check your own patch :)

But something a bit less minor:

We try to keep the amount of doubles as few as possible. Can't you avoid it? And no, don't come with a float :p Can you do it with an integer? First of all, it is much faster, second, it is much faster ;) And mostly it is easy possible to do so.

I will apply it this week or something to my trunk, see what it does :) From the code, it looks very promissing! Keep up the good work!
Comment by thomasdev (thomasdev) - Tuesday, 14 February 2006, 09:15 GMT
thx for commenting :)
i will fix that code-style soon and noticed the empty lines after adding my comment...
i mix the code-style to fit in the code-style for the other comments in that file-section
new patch is following soon (after my math exam) :-D
just used doubles, no floats, the only (float) was to print a value as i do not know which letter is for doubles...
Comment by thomasdev (thomasdev) - Wednesday, 15 February 2006, 11:01 GMT
+added smooth smallmap zooming
+added patch-settings in gui configfile and code
+added dualscreen resoultion for my testings
+added colored industry-production-signs
-bug: with repositioning of the zoomed area when window is not at default size ?!
Comment by Peter Nelson (peter1138) - Wednesday, 15 February 2006, 11:47 GMT
Thomas, as you left IRC so soon, here are my comments:

1) You're still using doubles. Using ints is preferable.
2) Still the spacing on some lines is inconsistent, e.g. "double diff_abs=(diff<0)?diff*-1:diff;" should be "double diff_abs = (diff < 0) ? -diff : diff;" (ignoring the double / int issue)
3) There are at least three separate patches here, a) zooming b) industry information c) vehicle display. Making them discrete incremental patches would be advantageous.
4) Stick to English for variable names (maybe I'm biased here :))
5) (my >> 1) << 4 == my << 3 (though that's not yours)
6) Don't put irrelevant changes in (screen resolution)
Comment by thomasdev (thomasdev) - Wednesday, 15 February 2006, 11:59 GMT
ok, will fix soon
Comment by thomasdev (thomasdev) - Thursday, 16 February 2006, 09:05 GMT
+patches seperated, only zoom-patch provided
i tried to convert the zoom-logic from doubles to int, but all my tries failed. hopefully someone can show me how it could be don
Comment by Luca Spiller (lucaspiller) - Monday, 27 February 2006, 19:49 GMT
Nice job reintroducing this patch, hopefully you will have more luck than myself at getting it into the tree.

I haven't tried this yet to see what is different, but I have a couple of suggestions that may help along the way:

1) WE DON'T NEED PATCHES FOR EVERY SINGLE LITTLE (lots of cursing) OPTION - as I said I haven't tried the patch to see what smooth zooming is like, but IMO it seems rather pointless to have it as a patch, just leave it on by default (of course there will be about a hundred people now hunting me down as they disagree). There are already far too many patch options to confuse users to hell, we don't need yet another.

2) Spacing still needs a bit of improvement, when defining variables, i.e. "double smallmap_zoom=WP(w, smallmap_d).zoom_ist;" should be "double smallmap_zoom = WP(w, smallmap_d).zoom_ist;". Spacing helps with readability so do it!

3) Before you post any patch, proof-read it by hand to make sure there are no changes where there shouldn't be, i.e. changing a blank line. Even if there is a space where there should just be a blank line do not change it unless it will end up within a section you have edited (if that makes sense) - even then you should be wary of such changes.

I have only had a brief skim through the patch, but using doubles seems a bit pointless to me. Using an int you can get 256 different levels - surely this is enough even for smooth scrolling?

If you need any help understanding how something works, or have any questions give me a shout (thelucster at gmail dot com). I will try and give you a hand, but I haven't done any OpenTTD stuff since before christmas so I may be a bit rusty, and with college on my plate I haven't got much free time.

Keep up the good work!
- Luca
Comment by Celestar (Celestar) - Friday, 31 March 2006, 16:51 GMT
I'm going to check that diff out over the weekend. I think it is worth adding for 0.5.0 because the smallmap appears rather useless on large maps in its current incarnation
Comment by thomasdev (thomasdev) - Monday, 03 April 2006, 01:31 GMT
thx for these greate comments :)
some comments from my side:

1) options-checkbox in patch-settings: this was a test, so i could switch "smooth" zooming on and off at runtime, my thoughts: if smooth will proof better, then take this and remove the option ...
2) spacing + blank lines, i know, working on my style :)

using doubles: i was not able to use int instead of doubles, because i couldn get it working to store the zoom-steps as int and calculating with floating-point numbers, i will try it again soon

@victor: thx for your interest, but i do not believe it merges without work anymore, because i wrote the patch against the nightly of 20th feburary
if anything is unclear just mail me thomas @ dontspam @ thomasfischer.biz

@luca: you did the great work with the base of this patch, i only bugfixed and improved it a little bit :-)
Comment by Peter Nelson (peter1138) - Monday, 10 April 2006, 10:45 GMT
This version of the patch replaces the floating point operations with integers and removes the patch options
Comment by thomasdev (thomasdev) - Tuesday, 11 April 2006, 08:38 GMT
patch ported to the latest svn, zooming & focus does still not work properly, hope luca will fix this
Comment by Peter Nelson (peter1138) - Tuesday, 11 April 2006, 09:32 GMT
This version of the patch has the zoom value the correct way around, i.e. 200 = 200%. Zooming appears to work correctly (although it's possible to stop at a arbitary value (is that good or bad?)) but recentreing on zoom is a little odd.
Comment by thomasdev (thomasdev) - Tuesday, 01 August 2006, 15:02 GMT
updated patch to svn revision 5694 (latest) (based on peter's patch int_zoom2) by the hope, that it will be integrated some day ;-)

added function for ScrollMainWindowTo so send a RECENTER event to the smallmap-window in order to recenter to the current viewport. ==> when you click twice on a newspaper-heading or something like this, the smallmap scrolls also to this position
Comment by thomasdev (thomasdev) - Tuesday, 01 August 2006, 15:22 GMT
fixed a bug that caused a segfault, when smallmap is not opened
Comment by Patric Stout (TrueBrain) - Sunday, 20 August 2006, 11:51 GMT
I applied one small piece of your patch. The patch looks very nice to have, so I promise you I will read through it soon and comment on it.
Comment by Patric Stout (TrueBrain) - Monday, 21 August 2006, 13:33 GMT
Okay, this patch should really be applied ASAP :) It only has some minor problems and defects, and some things that can be done more pretty. Also, this patch in fact are several patches. So I will take this patch and apply it piece by piece over the next few days.
Comment by precision (precision) - Thursday, 07 September 2006, 06:07 GMT
TrueLight, I hope you didn't forget this patch :(.
Comment by Remko Bijker (Rubidium) - Wednesday, 22 November 2006, 11:57 GMT
Updated the patch so it compile with revision 7236. One issue remains and that is the fact that when you're zooming it doesn't stay focussed at the point that was in the center of the map. I've looked at this problem for quite a while and was not able to solve this problem.
Comment by Luca Spiller (lucaspiller) - Wednesday, 22 November 2006, 13:53 GMT
I had problems with the centering when zooming myself, I think I got it to work in one of my patches on SF, so have a looky.

http://sourceforge.net/tracker/?func=detail&aid=1206659&group_id=103924&atid=636367
Comment by Luca Spiller (lucaspiller) - Wednesday, 22 November 2006, 13:53 GMT
I had problems with the centering when zooming myself, I think I got it to work in one of my patches on SF, so have a looky.

http://sourceforge.net/tracker/?func=detail&aid=1206659&group_id=103924&atid=636367

Also did you forget TrueLight? :P
Comment by Patric Stout (TrueBrain) - Saturday, 23 June 2007, 22:35 GMT
Such a nice patch, and totally forgotten... if someone can update it, we can reconsider adding it to trunk, if not, this task will be closed.
Comment by NukeBuster (NukeBuster) - Friday, 06 July 2007, 03:18 GMT
I happened to cross this task and thought it would be nice to bring this patch up to it's current revision (r10450).

Hopefully it's trunk worthy.

Please let me know.
Comment by NukeBuster (NukeBuster) - Monday, 23 July 2007, 19:33 GMT
This is the latest rev patch I have at this moment. The last problem is, when you scroll at a certain zoomlevel, and then zoom in. It doesn't stay focused on the place on the map where you have scrolled to.
Comment by NukeBuster (NukeBuster) - Monday, 23 July 2007, 19:34 GMT
forgot to add attachment.
Comment by thomasdev (thomasdev) - Monday, 23 July 2007, 19:51 GMT
that problem existed also in the base versions i based my patch on :-
i hope someone with the knowledge of the map positioning stuff can fix this, i tried but failed :(
and thanks for updating the patch!!
Comment by NukeBuster (NukeBuster) - Monday, 23 July 2007, 19:57 GMT
I have already had some tries, but the coordinate system in the smallmap still eludes me a bit.
Comment by thomasdev (thomasdev) - Monday, 23 July 2007, 20:19 GMT
i will just have a look at Luca's old code, maybe i will find something.
Comment by thomasdev (thomasdev) - Monday, 23 July 2007, 21:17 GMT
updated to revision 10665
and reintegrated the smooth zoom (looking nice!), seems to got lost during the conversions :-
the only remaining problem is the centering on the mainview rectangle during zoom instead of the smallmap view
Comment by NukeBuster (NukeBuster) - Monday, 23 July 2007, 21:19 GMT
The smooth zoom messed up the zoom percentage... so i ripped it some revs ago. Also some comments came about making it an option.
Comment by thomasdev (thomasdev) - Monday, 23 July 2007, 21:29 GMT
mhm its fine and working now again, could you test? :)
also there was already an option for this, BUT it was suggested not to make everything an option.
(see comment by Luca Spiller (Luca) - Monday, 27 February 2006, 08:49PM)
so it got removed. :-
Comment by NukeBuster (NukeBuster) - Monday, 23 July 2007, 21:33 GMT
Hmm... I've just compiled it, it looks nicer and faster than it did before I removed it. But it still has the problem.

A quote by Bilbo, who has tested this patch a number of times:
* After zooming competely out and back in I was unable to return to 100% zoom (except by closing and reopening the window). Zooming was snapping between 80 and 160 or some other values. When I pressed the zoom in/out button, the zoom goes 100->50>25->12->10. Then zoom in: 10->20->40->80->160 ...


The second quote is why I ripped the smooth zoom.
* The "smooth zoom scrolling" is quite slow. I think it should scroll maybe two or three times as fast. Or maybe it will be good to be able to set up scrolling instantly for those who does not want the "smooth zoom"
Comment by NukeBuster (NukeBuster) - Monday, 23 July 2007, 21:35 GMT
The first error is solved by the new calculation see below. But with smooth zoom, you will not be able to return to 100%.

This part:
+ if (zoom < 100) {
+ WP(w, smallmap_d).zoom_target = min(SMALLMAP_MAX_ZOOM, ((1000 / (100 / WP(w, smallmap_d).zoom)) * 2) / 10);
+ } else {
+ WP(w, smallmap_d).zoom_target = max((int)SMALLMAP_MIN_ZOOM, WP(w, smallmap_d).zoom * 2);
+ }
Comment by NukeBuster (NukeBuster) - Monday, 23 July 2007, 21:58 GMT
Here is my version against 10665.
Comment by NukeBuster (NukeBuster) - Monday, 23 July 2007, 21:58 GMT
Forgot it again :S
Comment by thomasdev (thomasdev) - Monday, 23 July 2007, 22:05 GMT
bugfixed smooth zoom + scroll
Comment by thomasdev (thomasdev) - Monday, 23 July 2007, 22:44 GMT
* fixes the smooth zooming: no more unround values, and no more infinite loops
Comment by NukeBuster (NukeBuster) - Tuesday, 24 July 2007, 14:57 GMT
Thomas,

I changed smooth zoom to have constant steps. As zoom changed every tick. diff / 5 produced a changing result. Meaning more "smooth zoom" steps with a bigger zoom%.
It's now using the zoom_target / 10 to get a constant value. Although 5 steps seems a little low.

There is one error currently unsolved. When on 12% you're unable to zoom in or out using the mouse. When right clicking, it snaps to 6%. So probably something I introduced with the new method.

Changed some of your code to comply with coding style. (Some spaces missing here and there.)
Did some code rearrangements to place WE_TICK and WE_MOUSEWHEEL more logically.
Comment by NukeBuster (NukeBuster) - Tuesday, 24 July 2007, 15:30 GMT
Hmm, another strange thing. It does 5 steps on zooming in. But 10 on zooming out. I'm going to look into that a little more.
Comment by Bilbo (bilbo) - Tuesday, 24 July 2007, 16:07 GMT
I liked the old version without smooth zoom more. Perhaps make the smooth zoom optional? Either create new patch option for it, or there is "Smooth viewport scrolling" already, so the zoom will be smooth when this option is on and will work instantly with the option off.
Comment by NukeBuster (NukeBuster) - Tuesday, 24 July 2007, 16:09 GMT
The following:
SetWindowWidgetDisabledState(w, 13, zoom == SMALLMAP_MAX_ZOOM);
SetWindowWidgetDisabledState(w, 14, zoom == SMALLMAP_MIN_ZOOM);

zoom was not changed in WE_TICK. WP(w, smallmap_d).zoom was used to set new values.

Comment by NukeBuster (NukeBuster) - Tuesday, 24 July 2007, 16:15 GMT
@Bilbo, there is no option for smooth zoom yet. Although I do agree that you have to scroll your mouse button more with smooth zoom.
Comment by NukeBuster (NukeBuster) - Tuesday, 24 July 2007, 16:35 GMT
Update,

Although I'm already further along the enhanced smooth zooming path. I'd still like to upload an update in between. This always has 5 steps in between zooming in or out. And the problem with the 12% zoom is also solved.

The enhanced smooth zooming will probably suit some users more.

Please give me comments on this diff.
Comment by NukeBuster (NukeBuster) - Tuesday, 24 July 2007, 16:55 GMT
I'm now trying to use the WP(w, smallmap_d).zoom_target to calculate the new zoomlevel. So when scrolling_the_mouse more, the zooming in will go faster.

But when zooming out, it will get slower and slower. So it is probably not the best solution.
Comment by NukeBuster (NukeBuster) - Tuesday, 24 July 2007, 17:01 GMT
What about instead of adding a patch option, we add an extra button to the mini-map to switch smooth zooming?
Comment by Bilbo (bilbo) - Wednesday, 25 July 2007, 02:56 GMT
I think options should be with other options to be consistent ... it's not good to have some extra options at many individual widgets ... and it will IMHO add quite unnecessary button to the smallmap gui ... most people will set up once what they want and then touch the setting no more.
Comment by NukeBuster (NukeBuster) - Wednesday, 25 July 2007, 17:16 GMT
I will think about the possibilities. I'm about to go on vacation for a week or 2 so the next update may take a while.
Comment by Phazorx (Phazorx) - Sunday, 28 October 2007, 22:49 GMT
adapted to recent trunk (buttons were moved)
It should be noted somewhere that this does NOT work in paused mode
Comment by Romanski (Romanski) - Wednesday, 31 October 2007, 10:17 GMT
Task 1222 (http://bugs.openttd.org/task/1222) implements zoomable smallmap in a different way:

Better:
+ 4x higher resolution when zoomed out (the task 54 patch uses the original smallmap algorithm, which is restricted to drawing the map out of 4x1 pixel blocks)
+ draw towns and industries as dots when zoomed out, to avoid them disappearing altogether
+ pretty dithering
+ works when paused

Worse:
- no smooth zooming
- can't zoom to arbitrary zoom levels
- can't zoom in more than the original, un-zoomable map.

Please consider both fairly! :) (disclaimer: obviously I'm the author of 1222; I'm trying to be as objective as I can here; if there is an advantage of task 54 patch which I didn't mention it was not on purpose!)
Comment by Romanski (Romanski) - Wednesday, 31 October 2007, 11:10 GMT
Actually I forgot a couple of other advantages of 1222:

+ zoom in/out at cursor, just like the main map
+ scrolling the wheel quickly will zoom several levels in/out quickly
Comment by fonsinchen (fonsinchen) - Wednesday, 10 June 2009, 18:24 GMT
I have adapted the patch to current trunk, rewriting most of it. I think it is a little cleaner like this. The large size of the diff is mostly due to DrawSmallMapStuff being moved into the SmallMapWindow class (where it belongs). The function itself has hardly changed. I didn't try to understand or clarify the arcane magic involved in drawing the smallmap, so the code is still not nice to read, but at least it didn't get worse. I also removed the smooth zooming, the zoom level indicator and the dependence on ticks to zoom. And I'm using the regular ZoomLevel enum, but I had to create a few more zoom levels in order to be able to scroll in. I need the scroll in functionality for cargodist. However, as the rest of the code doesn't play well with that, I didn't change the BEGIN, END and COUNT values of the enum, which is sort of a hack. I'll happily change those things if any specific ideas show up or if anyone can give any relevant details on the previous functioning of the smallmap.
Comment by Zdeněk Sojka (SmatZ) - Wednesday, 17 June 2009, 16:35 GMT
The minimap gets incredibly glitchy (it's "shaking") when you are moving the minimap (right-click in the minimap window).
What's the point of zoom-in in the minimap? I would remove it, really. It's buggy anyway (at least wrt. vehicles) and the changes done in zoom_type.hpp are not nice. I think you would like to remove it when you will be fixing that "shaking" problem. (buggy, hacky and useless anyway)
I would expect the mouse wheel to work for zooming.
Added buttons are not disabled when further zooming isn't possible.

 FS#1222  has better behaviour in this sense. But has different issues, town names are disappearing, and diff is harded to read.
Comment by Romanski (Romanski) - Wednesday, 17 June 2009, 16:41 GMT
I could change the town names behaviour in  FS#1222 . Would you prefer if they disappeared later on or not at all?

Also, someone suggested that if I reformatted the diff manually it would be easier to read - i just relied on svn diff there... I'll give it a go, see if it helps.
Comment by Zdeněk Sojka (SmatZ) - Wednesday, 17 June 2009, 16:47 GMT
I think it's fine the names are getting smaller, but I would prefer not to remove them completely. Also, please stick with official coding style (curly brackets, comments) - http://wiki.openttd.org/Coding_style
Comment by fonsinchen (fonsinchen) - Wednesday, 17 June 2009, 16:51 GMT
The point of zooming in is that I need it for cargodist. The problem there is that you cannot read the link statistics of short links. I can split up the patch to form one version with zooming out and one with additional zooming in. The other issues will be fixed in future versions. My post shows the bare minimum needed to allow zooming. I chose this patch especially because it can zoom in and because it changes much less code than  FS#1222 .
Comment by Zdeněk Sojka (SmatZ) - Wednesday, 17 June 2009, 16:55 GMT
Yes, functionality of  FS#1222  and patch size/code changes from  FS#54  would be the best.
Cargodist isn't in trunk...
Comment by Romanski (Romanski) - Wednesday, 17 June 2009, 17:02 GMT
Heh, it would, wouldn't it :)

The size of the change cannot be reduced because the patch rewrites the core algorithm involved in drawing the minimap. The upside is that it can draw single pixels per tile when zoomed out, whereas #54 draws 4x1 pixel "tiles" even when zoomed out. The downside is the patch size.

Zooming in can be added... and would increase the patch even further :)

Not sure whether it's worth spending the time prettifying the diff - if it's too large to be looked at anyway...
Comment by fonsinchen (fonsinchen) - Wednesday, 17 June 2009, 17:15 GMT
I could revert the changes in zoom_type.hpp if I added a flag in smallmap that denotes if the zoom level is to be interpreted as zooming in or zooming out. Would that be more acceptable than the additional zoom levels? I mean, cargodist is really my only motivation for picking up this patch and zooming in is the only function I need there.
Comment by Bilbo (bilbo) - Wednesday, 17 June 2009, 19:16 GMT
With 2048x2048 map and usual screen sizes, the entire minimap does not fit on screen. Even worse if you have small display (some smallish notebooks/netbooks, etc ...)

Therefore I think the feature is good even without any patches - though for some patches, like cargodest or my large map size patch, having zoomable minimap would help much more.
Comment by fonsinchen (fonsinchen) - Friday, 10 July 2009, 00:14 GMT
This version tries to fix the "shaking" problem. In the lowest zoom level you still can't scroll pixel by pixel, but the situation is much better than in the previous iteration. For my taste it's good like this. Please tell me what you think about it.
Comment by fonsinchen (fonsinchen) - Sunday, 12 July 2009, 17:28 GMT
This patch fixes the remaining issues. Changes:
* disable the "+" and "-" buttons when reaching max/min zoom levels.
* enable mouse wheel zooming
* make zooming a little more smooth
* document some things
* remove useless code
* draw vehicles correctly on zoom in

By now I have understood most of the obscurity in smallmap and either documented or removed it. The only thing left are some magic offsets in DrawSmallmap which I can't make any sense of. I have, however, marked those places accordingly.
Comment by fonsinchen (fonsinchen) - Wednesday, 05 August 2009, 02:50 GMT
new version. changes:
* split up in zoom-in and zoom-out as requested on IRC. zoom-out can be applied to plain trunk. zoom-in is to be applied on top of both zoom-out and the patch from  FS#3094 
* corrected a small glitch in DrawIndustries
Comment by fonsinchen (fonsinchen) - Thursday, 13 August 2009, 12:36 GMT
New version:
* +, - and center buttons are visibly depressed now when clicked on
* some constants enumified and consistently named
Comment by fonsinchen (fonsinchen) - Friday, 14 August 2009, 18:42 GMT
New version:

* fixed centermap button getting pressed on implicit centering
* fixed "+" button being enabled on initialization
* fixed hiding/showing enable/disable industries buttons in OnPaint (it's doing that in constructor and OnClick instead now)
Comment by Alberth (Alberth) - Saturday, 06 February 2010, 13:32 GMT
Zoom-out is implemented in trunk in r19039-r19042 using a somewhat different approach.
Keeping the issue open for the zoom-in part.
Comment by fonsinchen (fonsinchen) - Thursday, 10 June 2010, 21:05 GMT Comment by fonsinchen (fonsinchen) - Thursday, 23 September 2010, 19:53 GMT
I have cleaned up the patch and removed the lowest zoom in level as that was as detailed as the main map. I think it is ready for trunk now.
Comment by fonsinchen (fonsinchen) - Saturday, 01 March 2014, 14:39 GMT
https://github.com/ulfhermann/openttd/commit/ed06fdb31990434a1d160c91c41831de430a039e is pretty broken regarding the positioning of town names and the current viewport indicators. However, it's a fairly clean patch on top of the last supported version (r23343) - just in case anyone wants to pick it up again.

Loading...