Closed DorpsGek closed 10 years ago
thomasdev wrote:
fixed all spaces and tabs and reduced the patch-size hope this is better :-D
Attachments
thomasdev wrote:
+added mousewheel-zooming +added vehilce draw if close enough
Attachments
thomasdev wrote:
+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
Attachments
TrueBrain wrote:
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!
thomasdev wrote:
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...
thomasdev wrote:
+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 ?!
Attachments
peter1138 wrote:
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)
thomasdev wrote:
ok, will fix soon
thomasdev wrote:
+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
Attachments
lucaspiller wrote:
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
Celestar wrote:
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
thomasdev wrote:
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 :-)
peter1138 wrote:
This version of the patch replaces the floating point operations with integers and removes the patch options
thomasdev wrote:
patch ported to the latest svn, zooming & focus does still not work properly, hope luca will fix this
Attachments
peter1138 wrote:
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.
Attachments
thomasdev wrote:
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
Attachments
thomasdev wrote:
fixed a bug that caused a segfault, when smallmap is not opened
Attachments
TrueBrain wrote:
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.
TrueBrain wrote:
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.
precision wrote:
TrueLight, I hope you didn't forget this patch :(.
Rubidium wrote:
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.
Attachments
lucaspiller wrote:
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
lucaspiller wrote:
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
TrueBrain wrote:
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.
NukeBuster wrote:
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.
Attachments
NukeBuster wrote:
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.
NukeBuster wrote:
forgot to add attachment.
Attachments
thomasdev wrote:
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!!
NukeBuster wrote:
I have already had some tries, but the coordinate system in the smallmap still eludes me a bit.
thomasdev wrote:
i will just have a look at Luca's old code, maybe i will find something.
thomasdev wrote:
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
Attachments
NukeBuster wrote:
The smooth zoom messed up the zoom percentage... so i ripped it some revs ago. Also some comments came about making it an option.
thomasdev wrote:
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. :-
NukeBuster wrote:
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"
NukeBuster wrote:
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); + }
NukeBuster wrote:
Here is my version against 10665.
NukeBuster wrote:
Forgot it again :S
Attachments
thomasdev wrote:
bugfixed smooth zoom + scroll
Attachments
thomasdev wrote:
* fixes the smooth zooming: no more unround values, and no more infinite loops
Attachments
NukeBuster wrote:
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.
Attachments
NukeBuster wrote:
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.
bilbo wrote:
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.
NukeBuster wrote:
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.
NukeBuster wrote:
@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.
NukeBuster wrote:
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.
Attachments
NukeBuster wrote:
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.
NukeBuster wrote:
What about instead of adding a patch option, we add an extra button to the mini-map to switch smooth zooming?
bilbo wrote:
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.
NukeBuster wrote:
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.
Phazorx wrote:
adapted to recent trunk (buttons were moved) It should be noted somewhere that this does NOT work in paused mode
Attachments
thomasdev opened the ticket and wrote:
Attachments
Reported version: trunk Operating system: All
This issue was imported from FlySpray: https://bugs.openttd.org/task/54