FS#54 — smallmap-zoom
Attached to Project— OpenTTD
Opened by Thomas (thomasdev) - Monday, 13 February 2006, 12:46PM
Last edited by Remko Bijker (Rubidium) - Saturday, 01 December 2007, 10:29PM
Opened by Thomas (thomasdev) - Monday, 13 February 2006, 12:46PM
Last edited by Remko Bijker (Rubidium) - Saturday, 01 December 2007, 10:29PM
| Work in progress | |
| User Interface | |
| Waiting for user | |
| Victor Fischer | |
| All |
| Medium | |
| Normal | |
| Devel (specify rev) | |
| Undecided | |
| Undecided | |
 |
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
smallmap_patch.diff
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
smallmap_patch.diff
This task depends upon
This task blocks these from closing
fixed all spaces and tabs and reduced the patch-size
hope this is better :-D
+added mousewheel-zooming
+added vehilce draw if close enough
+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
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!
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...
+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 ?!
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)
ok, will fix soon
+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
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
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
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 :-)
This version of the patch replaces the floating point operations with integers and removes the patch options
patch ported to the latest svn, zooming & focus does still not work properly, hope luca will fix this
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.
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
fixed a bug that caused a segfault, when smallmap is not opened
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.
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.
TrueLight, I hope you didn't forget this patch :(.
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.
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
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
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.
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.
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.
forgot to add attachment.
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!!
I have already had some tries, but the coordinate system in the smallmap still eludes me a bit.
i will just have a look at Luca's old code, maybe i will find something.
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
The smooth zoom messed up the zoom percentage... so i ripped it some revs ago. Also some comments came about making it an option.
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. :-\
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"
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);
+ }
Here is my version against 10665.
Forgot it again :S
bugfixed smooth zoom + scroll
* fixes the smooth zooming: no more unround values, and no more infinite loops
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.
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.
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.
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.
@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.
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.
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.
What about instead of adding a patch option, we add an extra button to the mini-map to switch smooth zooming?
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.
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.
adapted to recent trunk (buttons were moved)
It should be noted somewhere that this does NOT work in paused mode
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!)
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