Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd #6887: Option to show zone inside local authority boundary of towns #7025
Conversation
This comment has been minimized.
This comment has been minimized.
I need some feedback:
I saw the JGR Patch Pack/Zone patch, but the small selection sprites seemed too new to add (not legacy TTD look). |
This comment has been minimized.
This comment has been minimized.
Milek7
commented
Jan 6, 2019
I don't think that |
This comment has been minimized.
This comment has been minimized.
How does the query tile window check? It always shows Local Authority. |
This comment has been minimized.
This comment has been minimized.
Yes, I used the method from land info. |
This comment has been minimized.
This comment has been minimized.
Milek7
commented
Jan 6, 2019
Ok, nevermind. I had problems with this method 2 years ago but can't quite remember what was wrong. |
This comment has been minimized.
This comment has been minimized.
I've just realised that this version requires a lot of computation, so I have to do something about it. |
This comment has been minimized.
This comment has been minimized.
Milek7
commented
Jan 6, 2019
Causes of my confusion about tile authorities 2 years ago were:
So using ClosestTownFromTile with dist_local_authority is correct for determining local authority jurisdiction for destruction/tree planting ratings, but invalid for purposes of determining stations influence on local authority ratings. |
This comment has been minimized.
This comment has been minimized.
Note about the methods to decide what tiles gets the zone indicating tile selection sprite:
|
I like it! I'm open to any further performance improvements, but as it stands currently this is a nice feature that I've seen requested many times and is very simple to implement |
@@ -45,6 +45,7 @@ static const NWidgetPart _nested_town_authority_widgets[] = { | |||
NWidget(NWID_HORIZONTAL), | |||
NWidget(WWT_CLOSEBOX, COLOUR_BROWN), | |||
NWidget(WWT_CAPTION, COLOUR_BROWN, WID_TA_CAPTION), SetDataTip(STR_LOCAL_AUTHORITY_CAPTION, STR_TOOLTIP_WINDOW_TITLE_DRAG_THIS), | |||
NWidget(WWT_TEXTBTN, COLOUR_BROWN, WID_TA_ZONE_BUTTON), SetMinimalSize(50, 0), SetMinimalTextLines(1, WD_FRAMERECT_TOP + WD_FRAMERECT_BOTTOM + 2), SetDataTip(STR_LOCAL_AUTHORITY_ZONE, STR_LOCAL_AUTHORITY_ZONE_TOOLTIP), |
This comment has been minimized.
This comment has been minimized.
LordAro
Jan 10, 2019
Member
I wonder whether this is a bit far out of the way in the Authority window. I suspect it's not going to be noticed here...
This comment has been minimized.
This comment has been minimized.
andythenorth
Jan 10, 2019
Contributor
Try it as a toggle on either the global 'map' menu or 'town' menu? Dunno if it's better.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
GabdaZM
Jan 12, 2019
Author
Contributor
I would like to keep the possibility to toggle the zone display for the towns individually. This way it is not a problem when two towns are really close or even grown into one. I cannot do that in the global map. Placing a toggle button before each town name in the town list would be possible, but finding a town you already see on the viewport can be harder than the current method.
The parent town menu is really close to the original TTD look, and I don't have the heart to modify it.
I agree that it is hard to find as it is, but I think the function belongs to that windows logically. If the same function will be added to the stations to show coverage, maybe more people will look for it and search the town related windows.
This comment has been minimized.
This comment has been minimized.
The main performance problem is that is calculates the closes town for every tile even when none of the town zones indications are on (so I plan to add a counter, and calculate when it is not 0), and when on max zoom out, scrolling refresh all tiles, and all selections, and getting the closest tile for each tile on the screen, especially is there are a lot of town, is too much. I plan to disable this function on max zoom out. |
This comment has been minimized.
This comment has been minimized.
My other approach for this issue: Add #6887: Highlight tiles within local authority of towns #7047 |
This comment has been minimized.
This comment has been minimized.
I'm very tempted to say that this solution is the better one, just because it's so much simpler and doesn't touch the map array. I've not noticed any performance impact myself either... do you have any measurements for each solution? |
This comment has been minimized.
This comment has been minimized.
This solution can have heavy impact, that is why I disabled the function in out zoom 16 and 32. The necessary calculations are proportional with the number of displayed tiles, and also proportional with the number of towns. In a 32x out zoom, the closest town check is done 250.000-300.000 times per second. When I tried it in a 4096x4096 town (it is only interesting because of the number of towns) the graphics needed ~10 ms when none of zones were turned on, and ~300 ms when I turned on one zone. |
This comment has been minimized.
This comment has been minimized.
Have we come to the conclusion that this solution definitely isn't workable? If so, I'd like to close it and get it out of the way :) |
This comment has been minimized.
This comment has been minimized.
It is workable, but calculation heavy, and I like the other solution more. |
This comment has been minimized.
This comment has been minimized.
I used zoning patch for many years and even programmed some new zones for it (including town ones). Yet, I'm not a big fan of zones and I'm looking for a better ways to do it. First of all there are just too many different zones. Even if we only consider towns there are two authority zones, three advertisement zones and five house zones plus growth highlight and they all have their use. Second, they are indeed computational-heavy and there doesn't seem to be an easy way of making it faster. Third, sometimes you need two-three zones at once. E.g., I always keep growth highlight in citybuilder and switch between other zones with hotkeys. What I'm thinking of lately is to somehow switch from having to toggle zones on/off to automatically show them when they're needed. E.g. show authorities when placing stations. Industry forbidden areas when funding industries. Advertisement zones when hovering advertisement options (and building stations I guess) and so on. But I couldn't think of any good way to implement it with current UI. I'd really like to see some new elements like opaque overlays for zones, rich building tooltips for info, something along those lines. |
This comment has been minimized.
This comment has been minimized.
+1 I tried the zones patch included in JGR PP, and I found it quite confusing in use. I wonder if it's simply too much information, or if it's simply a visual problem with the overlay? It might be interesting to toggle ground tile sprites between 'terrain' and 'metadata'? Then tile colours could be used, for e.g. town boundaries. Might not work, dunno :D . Be easy to mockup a fake test with objects grf though. |
This comment has been minimized.
This comment has been minimized.
JGR PP zoning patch uses vibrant colours and smaller tile selection highlights. While it is informative, I don't know how it matches the TTD visual style, and which one is more supported:
|
This comment has been minimized.
This comment has been minimized.
I like the idea, I did a demo on the industry forbidden areas part in an other PR. |
This comment has been minimized.
This comment has been minimized.
stale
bot
commented
Mar 5, 2019
This pull request has been automatically marked as stale because it has not had any activity in the last month. |
6d12072
to
4298701
* Highlights tiles insede local authority of selected towns. | ||
* @param *ti TileInfo Tile that is being drawn | ||
*/ | ||
static void HighlightTownLocalAuthorityTiles(const TileInfo *ti) |
This comment has been minimized.
This comment has been minimized.
GabdaZM
Apr 6, 2019
Author
Contributor
Should it return with a bool, and make the drawing at DrawTileSelection
? (Of course with a renamed function.)
This comment has been minimized.
This comment has been minimized.
Now using a k-d tree to store the towns that have the highlight turned on. This way, when searching for closest town for a tile, most of the times it is enough to search in this small k-d tree. |
This comment has been minimized.
This comment has been minimized.
stale
bot
commented
May 6, 2019
This pull request has been automatically marked as stale because it has not had any activity in the last month. |
This comment has been minimized.
This comment has been minimized.
Can I ask someone to have a look at this PR? |
Couple of small things, but I'm otherwise happy with this |
Can be found at town information > local authority window Layout for button is same as Graph Keys Turn on/off for every town individually
This comment has been minimized.
This comment has been minimized.
This visualization does not collide with the coverage display. |
This comment has been minimized.
This comment has been minimized.
How can I indicate that the requested changes were made? |
This comment has been minimized.
This comment has been minimized.
By telling us :) Why GH doesn't allow the PR author to dismiss reviews, I don't know... |
GabdaZM commentedJan 6, 2019
Can be found at town information > local authority window
Layout for button is same as Graph Keys
Turn on/off for every town individually