Minimap improvements

[Mariculous]

There's a thread in the extended section of the forums but I guess most of these minimap improvements are actually not specific to standard but might also do a good jon in standard.

In the past, Ranran already implemented some improvements to the minimap.
Those changes I remember are:
- Merging the Mail/Passenger layer into to coverage layer. The usual line filters can be used to filter the display by Pax/Mail/Freight including specific freight types.
- The "show contour" checkbox. This will hide height informations on the minimap, which sometimes is quite useful to get a clearer look on the map.
I guess there had been a few more changes, I might have a detailled look later.

If the standard team agrees that those changes might be a good hing in standard, I will backport this from extended.

Further, In preparation to some changes of minimaps "network" overlay, I have reorganised the drawing code of those ordinal-cardinal lines.
It's just a code cleanup in preperation to further work, but I guess it's worth to backport that cleanup.
This can be found in here:
https://github.com/irgend42/simutrans-extended/commit/b77b8022edc09fcd03b3e2a6a84c31dbd8fb240f

Further changes currently WIP:

[prissi]

The contour thing was in standard and I think Ranran took it to experimental.

The changes in the constructor minimap.h in the diff I personally do not like. This is much harder to follow (and also involves more conditionals) than the old code.

The rest of cleanup is not clear to me (apart from deleting the obsolete constructor), because I cannot compile it after patching:

Code Select Expand


Error C2664 'void swap(planquadrat_t &,planquadrat_t &)': cannot convert argument 1 from 'scr_coord' to 'planquadrat_t &' Simutrans GDI C:\msys64\home\Markus\simutrans\trunk\gui\minimap.cc 495

same in line 496.

[Mariculous]

This is an extended commit!
You cannot simply apply the patch to standard.
It's just there to show what has changed in this commit.
To apply these changes to standard I'll first have to backport this properly.

About the constructor, usually initialisation should be prefered over (re-)assignment.
If you don't like the conditions in the initialisation list, then this might be moved to the constructors body, so these can be swapped when the condition is not met.

About the swap: It does not refer to swap(planquadrat_t &,planquadrat_t &), but to void swap( T& a, T& b ) from the C++ standard. https://en.cppreference.com/w/cpp/algorithm/swap
Anyways, It's not explicitly importated but "simply there" due to imports from other files. I'll clean this up.
Which C++ standard does simutrans comply to? swap seems to be a C++ 11 feature.
Anyways, this will not prevent a backport to standard. Manually swapping the values is not a big thing.

[prissi]

The compiler should be clever enough that both give the same code. Maybe the single it is even better one, since there is only on if evaluation instead of four.

I must confess that putting everything into the constructor is harder to follow for me, which is the most important thing on not performance critical code for me ...

Anyway, if this is not a workable patch, I cannot really figure out what you want to achieve from this, so I cannot comment at all.

[Mariculous]

It IS a workable commit in extended.
The question here is: Is standard interessted in a backport of this commit to standard or not?
It seems quite pointless to backport this just to throw the backport away.

[Dwachs]

Seems like an unproblematic change. Why not implement that big constructor in the cc file?
I would like to see this

Code Select Expand


if((bool)start_offset!=(bool)end_offset) {
..
}

changed to

Code Select Expand


if( (start_offset==0) != (end_offset==0)) {
..
}

Also please add space after start of comment //.

[Mariculous]

Seems like an unproblematic change. Why not implement that big constructor in the cc file?

I have no idea.
Sombody moved it to the .h file so I expected there was a reason for this.

That change makes sense, changed in the extended branch.

I'll go on and prepare this for standard now.

What about mentioned change of Passenger/Mail layer into the "stop coverage" layer?

[Mariculous]

https://github.com/aburch/simutrans/compare/master...irgend42:standard-minimap-cleanup.diff
Just the essential changes.

[prissi]

The code is not identical, it is much more than a leanup. Your code generates more wiggles. See the red marks.

[Mariculous]

Indeed I screwed the "reduce overlap" fix up.
Fixed now.

Call it a code reoganisation if you prefer this term.
I actually didn't cherry pick all of the cleanup things as it was rejected in the first place.

[prissi]

Sorry, I did not want to offence you., especially since you mainly work for experimental. Just back then I spent some effort to reduce bends;so the change was obvious.

However, everything looks like before, so in it goes in r 9750, thank you.

[Mariculous]

I am totally fine with this. Sorry, if it didn't seem like that.
Pointing out issues is part of a proper review and the code was actually wrong in the diagonal fixup part.

[ampersand]

so in it goes in r 9750, thank you.

Does it mean it will be introduced to the standard Simutrans?

[Mariculous]

Yes, I think so.

However, so far it's just a small internal reorganisation of how those ordinal-cardinal lines are drawn.
Once implemented in extended, I will list the minimap improvements and backport these if standard considers these worth.

[prissi]

The contour drawing is in as well, since I followed the thread of experimental. (Or rather I did it my way, since at that time it was not finished.)