-
Notifications
You must be signed in to change notification settings - Fork 819
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Made military area rendering less prominent. #3057
Conversation
In fact danger area rendering was changed, because it uses the same overlay file as standard military area ( |
Danger area uses
It contains a fine 'X' pattern you can just about see in one of examples. I agree it should be improved, but it wasn't the goal of that fix. As for the pattern for the |
I know, I did the SVG version of this x, but I think this is barely noticable even on your "after" rendering and not as clear symbol, so I'd like to get rid of this, making a difference only in hatching. When the standard area will be lighter, it will be easy to see the strong hatching, which has clear connotation ("don't go there"). |
Hi kocio-pl, anything you want me to do with regard to this pr? I assumed danger_area will be improved later/separately. Or do you want to bundle both changes together? |
If there are no objections, I suggest to make it at once. But if there are any or you think this would be too broad, we can deal with it in another PR. |
Any suggestions for the pattern of danger_area? If it is hard to decide I would recommend moving this discussion to #3045. There is also a possibility of further reducing opacity of the military area or removing hatching from it, which would make the existing danger area more noticeable. |
My idea is simple - just continue to show it like currently (so current PNG file shouldn't be removed), but without x and with the same line offset as new military area. |
Sorry, I don't quite get it. Do you mean the following changes to the danger_area:
If you want to keep old and new png's, I will rename them to military_red_hatch.png and danger_red_hatch.png. Just want to make sure that's indeed what is needed. |
Yes, this is a the shopping list I would propose. 😄 |
Thanks, it looks sane for me - we can apply some more tuning and test rendering more, but I guess this code is mergable in general and fixes most of the problems. I'm not sure if the outline offset is big enough (especially with city walls), but more aggressive filtering of military areas is nice. My rendering tests (see also #3045 (comment) ): Military area (with and without the barrier) compared to commercial, retail, industrial and power areas around: |
That's a big improvement, thanks! |
Name rendering and area rendering need to be synchronized. We can have the area with no name label, but not the other way around. I guess since we don't have use case for military areas on z7, we can start on z8, just like with the other areas, for example nature reserves, which have similar rendering. |
Treating them the same way as military_danger_area labes. Both were already using the same font size, colour etc. Now they also use the same filtering options.
I've adjusted text filtering options for military areas (z8->z9). In fact, I've combined military_area and military_danger_area label rendering code, as they were already using the same rendering settings. (Previously military_area was sharing code with natural_wood, landuse_forest, boundary_national_park, leisure_nature_reserve areas which all start at z8). |
Did you test this change? Looks like something got broken and the names are not shown even when they were visible before: |
Sorry. Working on it. |
Now it works as expected, also in New Mexico. I would however start with z8, like we do with nature reserves - we can set filter to avoid too small areas being shown (for nature reserves we filter out the areas that are too small to show their name). |
But before the last two commits it was starting from z8. Do you mean I should revert them and change the stating zoom level for landuse_military to z8? |
Sorry, I try to follow more issues and I loose the sight of details, so I'm not 100% sure what was the state before last two commits. IIRC you've made small code fix which makes sense for me ("I've combined military_area and military_danger_area label rendering code"), so maybe it's better to not revert it, for the sake of clarity, but instead add zoom-entry-level fix on top of it. |
Before my modifications, handling labels of military areas was combined with other land-use labels as they were all rendered from z8. Military danger areas are rendered from z9, so when you asked me to switch the military area from z8 to z9 it was logical to detach it from the existing code and combine it with the code handling labels for military danger areas. But, if the plan is now to switch back to z8, I'd just revert to the original code, as there was nothing wrong with it. Then, of course, there will be cases of lone labels you've noticed. To fix that we would have to push back rendering the military areas themselves from z9 to z8. So, I'd suggest (in order of my preference) :-)
|
You know my reasons to stay with z8 (analogy with nature reserves and some other landuses), so I would choose 2 (however I don't know why danger area is rendered later). What are your reasons to omit z8 completely? |
I chose z9 specifically to match it to military_danger_area and to suppress visibility of military areas in general (initially they were way overexposed because of limits and the rendering style). Z8 is still fairly low ("a map of a small country/large region"), at this level users are still interested in basics (large cities, topography, large land uses and waterways, main communication routes) rather than features, and military areas are IMHO a feature. For comparison:
The above is my opinion. If you disagree let's just switch back to z8. Both solutions are an improvement over the current rendering. |
OK, so let's get back to z8 - rendering will be weaker, so it will be less visible than currently at least. |
This reverts commit 03d5232.
Done |
Thanks! I plan to check it again soon and this time I will also take a look at military danger areas (I haven't found them yet with my random testing). |
Military danger area on the water example (click to see the full image): |
Land example of military danger area with some plain military areas around: My suggestion: this is an improvement also in danger areas (especially on the water, but lack of a pink background helps to notice the forest area too), so I'd like to have it merged, but I think I would keep the bright red font color for all such areas, because the difference between plain military area is not that visible now. |
Just in case you haven't noticed: in the last example all labels come from regular military areas, so no difference is expected there. Military danger area labels (first example) are bold and not slanted. I did change the text colour to match the new background pattern. And since we reused the military area pattern that also meant using the military area text colour. I think the difference in the text rendering is very noticeable but if you prefer the old colour that's fine too. |
Yes, I have noticed, but haven't investigate it. I prefer the old font color for danger areas, but slanted (as we do with most, if not all the areas without icon). |
Can you share a snipped of code for the design you like? Frankly speaking, this is not a part I care about, so I'm OK with pretty much any sensible design. BTW, I've only changed the colour, the font face, sizes, filtering etc are are all original. |
OK, I made the change and it's available in my branch. Looks like nobody noticed the rule that area labels should be slanted. This color works only without landuse=military: otherwise it's rendered just like any other military area: but that's enough for now - tagging problems might need separate discussion. |
Looks suitable for a danger area. BTW, If we were to change the starting level to z8, we could combine it with other landcover areas. |
I feel that it's not worth trying to put more things into this PR, let's finish it now and we may discuss next things in separate issues/PRs. |
Agreed |
Let me ask you - If I understand the plan, you want to add this patch? I would merge this PR right after that. |
I am OK with merging this PR in its current state. The last suggestion brings very little value IMHO (minor refactoring) so let's not bother with it. |
Thanks for both the code and constructive discussion! I hope some more tuning will follow it. |
Thanks a lot both! |
Nice. Thank you for support and testing. I think rendering is fine now but it may be worth adjusting filtering options someday depending on the prevailing feedback (feature not prominent enough / too prominent). |
BTW - did you understand this hint: #3045 (comment)? I don't, it's too specific for me, but I wonder if we could use it and what would it look like? |
Not sure, I think that could have been about making the whole layer ("landuse_overlay") transparent while keeping objects in it 100% opaque. That way, if there are multiple overlapping military areas they would be rendered like a single area. At the moment, opacity of 2 overlapping military areas is higher than that of a single one. I don't necessarily mind the current implementation, I'd even consider it a feature (you can see it in action in #3045 (comment), second screenshot). But I don't know if I understand this issue correctly of if @gravitystorm meant something else. |
@andrzej-r Your description is exactly what I meant - that way, if there are two (or more) overlapping areas they would be rendered like a single area. |
Re #3045
This version does the following:
Examples (the reddish area in the middle is military area + danger_area, the latter was not modified):