Modify

Opened 4 years ago

Closed 3 years ago

#12104 closed enhancement (fixed)

mapcss: partial fill

Reported by: Klumbumbus Owned by: team
Priority: normal Milestone: 15.12
Component: Core mappaint Version:
Keywords: fill area multipolygon Cc: bastiK

Description

partial fill of areas similar to id editor.

Attachments (13)

partialfill.png (5.9 KB) - added by Klumbumbus 4 years ago.
linecap.png (10.0 KB) - added by Klumbumbus 4 years ago.
area-unclosed-closed.png (8.8 KB) - added by bastiK 4 years ago.
z.png (6.1 KB) - added by Klumbumbus 4 years ago.
buildings.PNG (151.5 KB) - added by Don-vip 4 years ago.
buildings-filled.png (125.1 KB) - added by bastiK 4 years ago.
margin.gif (27.8 KB) - added by Klumbumbus 3 years ago.
zoom.gif (735.2 KB) - added by Klumbumbus 3 years ago.
artefact1.png (1.4 KB) - added by Klumbumbus 3 years ago.
artefact2.png (2.1 KB) - added by Klumbumbus 3 years ago.
artefact3.png (4.0 KB) - added by Klumbumbus 3 years ago.
partial fill test.osm (5.8 KB) - added by Klumbumbus 3 years ago.
artefact4.png (1.6 KB) - added by Klumbumbus 3 years ago.

Download all attachments as: .zip

Change History (69)

comment:1 Changed 4 years ago by bastiK

In 9005/josm:

see #12104 - mapcss: add option for partial filling of areas (inspired by iD)

comment:2 Changed 4 years ago by bastiK

In 9008/josm:

see #12104 - mapcss: partial filling of areas (fill-extent) for fill-image

comment:3 Changed 4 years ago by bastiK

In 9009/josm:

see #12104 - mappaint: activate "fill areas only around their inner edges"

comment:4 Changed 4 years ago by Klumbumbus

(I connected your commits to this collective ticket.)

Changed 4 years ago by Klumbumbus

Attachment: partialfill.png added

comment:5 Changed 4 years ago by Klumbumbus

There is a bug with multipolygons

What steps will reproduce the problem?

  1. unclosed multipolygon (missing part or incomplete downloaded)
  2. partial fill is drawn between first and last point (for outer and also for inner)

Please provide any additional information below. Attach a screenshot if possible.

I didn't check fill image


Revision: 9015
Repository Root: http://josm.openstreetmap.de/svn
Relative URL: ^/trunk
Last Changed Author: Don-vip
Last Changed Date: 2015-11-16 03:23:57 +0100 (Mon, 16 Nov 2015)
Build-Date: 2015-11-16 02:34:08
URL: http://josm.openstreetmap.de/svn/trunk
Repository UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last Changed Rev: 9015

Identification: JOSM/1.5 (9015 de) Windows 7 32-Bit
Memory Usage: 344 MB / 742 MB (150 MB allocated, but free)
Java version: 1.8.0_65, Oracle Corporation, Java HotSpot(TM) Client VM
VM arguments: [-Djava.security.manager, -Djava.security.policy=file:C:\Program Files\Java\jre1.8.0_65\lib\security\javaws.policy, -DtrustProxy=true, -Djnlpx.home=<java.home>\bin, -Djnlpx.origFilenameArg=C:\Program Files\josm-latest-bla.jnlp, -Djnlpx.remove=true, -Djava.util.Arrays.useLegacyMergeSort=true, -Djnlpx.heapsize=256m,768m, -Djnlpx.splashport=51181, -Djnlpx.jvm=<java.home>\bin\javaw.exe, -Djnlpx.vmargs=LURqYXZhLnV0aWwuQXJyYXlzLnVzZUxlZ2FjeU1lcmdlU29ydD10cnVlAA==]
Dataset consistency test: No problems found

comment:6 Changed 4 years ago by naoliv

Shouldn't setting resource://styles/standard/elemstyles.mapcss:boolean:partial_fill to false (in JOSM's advanced prefs) get the old behavior back?

comment:7 Changed 4 years ago by bastiK

This would fix the problem of closed incomplete multipolygons. But I'm not sure if it would break anything, Don-vip?

  • src/org/openstreetmap/josm/data/osm/visitor/paint/relations/Multipolygon.java

     
    229229                    }
    230230                }
    231231            }
    232             if (!initial) { // fix #7593
    233                 poly.closePath();
    234             }
    235232            for (PolyData inner : inners) {
    236233                appendInner(inner.poly);
    237234            }

comment:8 Changed 4 years ago by bastiK

In 9016/josm:

mapcss: minor improvement for drawing of unclosed areas (see #12104)

comment:9 in reply to:  6 Changed 4 years ago by Klumbumbus

Replying to naoliv:

Shouldn't setting resource://styles/standard/elemstyles.mapcss:boolean:partial_fill to false (in JOSM's advanced prefs) get the old behavior back?

Yes, however it somehow only works when you use the style settings gui https://josm.openstreetmap.de/attachment/wiki/Help/Dialog/MapPaint/style-settings.png

comment:10 in reply to:  7 Changed 4 years ago by Don-vip

Replying to bastiK:

This would fix the problem of closed incomplete multipolygons. But I'm not sure if it would break anything, Don-vip?

I don't remember. Looking at #7593 it involved purging MP members, you can give it a try :)

comment:11 Changed 4 years ago by Don-vip

Keywords: fill area multipolygon added

comment:12 Changed 4 years ago by bastiK

In 9017/josm:

mapcss: fix partial fill for incomplete multipolygons (see #12104)

Changed 4 years ago by Klumbumbus

Attachment: linecap.png added

comment:13 Changed 4 years ago by Klumbumbus

The linecap is a bit strange in some cases. I think atleast the linecap for closed inners should be fixed.


comment:14 Changed 4 years ago by bastiK

In 9032/josm:

mapcss / partial fill: multipolygon inners now closed loop without first & last node visible (see #12104)

Changed 4 years ago by bastiK

Attachment: area-unclosed-closed.png added

comment:15 in reply to:  13 ; Changed 4 years ago by bastiK

Replying to Klumbumbus:

I think atleast the linecap for closed inners should be fixed.

Done.

The linecap is a bit strange in some cases.

Yes, but at the moment, I don't know how to fix this.

For large incomplete multipolygons (riverbank, forest), the area-margin is sometimes on the wrong side. Without any additional information, there is no way to tell for sure, what is inner and what is outer. One way to deal with this, would be to draw a margin on both sides, as long as the area is not closed:


It would make a clear distinction between closed and "almost closed" ways.

I'm not really sure about this and would like to hear your opinions.

Changed 4 years ago by Klumbumbus

Attachment: z.png added

comment:16 in reply to:  15 ; Changed 4 years ago by Klumbumbus

Replying to bastiK:

For large incomplete multipolygons (riverbank, forest), the area-margin is sometimes on the wrong side. Without any additional information, there is no way to tell for sure, what is inner and what is outer. One way to deal with this, would be to draw a margin on both sides, as long as the area is not closed:

I think this would be good, becasue it would avoid other strange renderings like in the following picture.
And since the width is half on each side it should not be mixed up with a line where two closed areas touch each other.


comment:17 Changed 4 years ago by RicoElectrico

My humble opinion is that this fill style should not be turned on by default for users. While it's nice to have, it should be opt-in. Style options are quite deep buried anyway.
Also, there is no logic to discriminate areas for which it makes no sense, like buildings.

comment:18 in reply to:  17 ; Changed 4 years ago by bastiK

Replying to RicoElectrico:

My humble opinion is that this fill style should not be turned on by default for users. While it's nice to have, it should be opt-in. Style options are quite deep buried anyway.

Thanks for your input! Could you give practical reasons, why you prefer the complete fill to the new style?

Also, there is no logic to discriminate areas for which it makes no sense, like buildings.

We could turn partial fill off for buildings and other selected features. Technically, this would be not a problem. So far, I like that everything is consistent, but there is certainly room for optimization and fine tuning.

comment:19 in reply to:  18 Changed 4 years ago by Klumbumbus

Replying to bastiK:

Replying to RicoElectrico:

My humble opinion is that this fill style should not be turned on by default for users. While it's nice to have, it should be opt-in. Style options are quite deep buried anyway.

Thanks for your input! Could you give practical reasons, why you prefer the complete fill to the new style?

IMO the mapview is more confusing, because there are more lines.

comment:20 Changed 4 years ago by Don-vip

+1 to exclude buildings, it is confusing in urban areas:


Last edited 4 years ago by Don-vip (previous) (diff)

Changed 4 years ago by Don-vip

Attachment: buildings.PNG added

Changed 4 years ago by bastiK

Attachment: buildings-filled.png added

comment:21 Changed 4 years ago by bastiK

Filled version:

This is an interesting example:

First, there are multiple nested areas stacked on top of each other ("ADM" building inside medical faculty building inside university). The partial fill version sure looks more complicated, but the question should be, which view makes it easier to see the structure of areas and understand the data without clicking through each object. For example take the area with the red highlight in the partial-fill image. In the filled version, I can't really tell for sure, if this is a single connected area or not. In the other image, this is quite clear, even without red highlight.

Second, the outline of the "ADM" building is not properly lined up with the other building, the ways are intersecting. This error can be seen in both versions, but in the partial fill image, this is more eye-catching because the overlapping lines are duplicated on the inside of the margin.

That said, I agree, that the partial fill is much less useful for smaller objects and I wouldn't mind turning it off for buildings, etc.

comment:22 Changed 4 years ago by Don-vip

Tomorrow I must release JOSM as new tested because of certificate issue. Can we set this new behaviour disabled by default until we are all happy with it? :)

comment:23 in reply to:  22 Changed 4 years ago by Klumbumbus

Replying to Don-vip:

Tomorrow I must release JOSM as new tested because of certificate issue. Can we set this new behaviour disabled by default until we are all happy with it? :)

+1

comment:24 Changed 4 years ago by bastiK

In 9055/josm:

see #12104 - turn off partial fill for next release

comment:25 in reply to:  22 Changed 4 years ago by bastiK

Replying to Don-vip:

Tomorrow I must release JOSM as new tested because of certificate issue. Can we set this new behaviour disabled by default until we are all happy with it? :)

Alright.. :)

comment:26 Changed 4 years ago by Don-vip

Milestone: 15.1115.12

thanks :)

comment:27 Changed 4 years ago by bastiK

In 9061/josm:

mappaint partial fill: render unclosed areas differently (margin on both sites) (see #12104)

comment:28 in reply to:  16 ; Changed 4 years ago by bastiK

Replying to Klumbumbus:

I think this would be good, becasue it would avoid other strange renderings like in the following picture.
And since the width is half on each side it should not be mixed up with a line where two closed areas touch each other.

Done. There is an Einstein pref to turn it on/off and set the relative width (currently 80%).

Replying to Don-vip:

+1 to exclude buildings, it is confusing in urban areas.

I have an alternative idea, that handles all kinds of small areas, not just buildings or what we explicitly set in the style:

Calculate the area of the unfilled black part in the center. If it is less than 50% of the total area, turn off partial fill for this object and fill it completely.

In my first test, this worked quite well and removed the strange looking artifacts in buildings, etc.

comment:29 in reply to:  28 Changed 4 years ago by Klumbumbus

Replying to bastiK:

Calculate the area of the unfilled black part in the center. If it is less than 50% of the total area, turn off partial fill for this object and fill it completely.

That sounds good!

comment:30 Changed 4 years ago by Don-vip

Generic solution are often better :)

comment:31 Changed 4 years ago by bastiK

In 9063/josm:

mapcss: partial fill - if partial fill would only leave a small gap in the center, fill area completely (see #12104)

comment:32 Changed 4 years ago by bastiK

I've implemented this idea, please have a look!

Again, there is an advanced parameter (draw.area.partial_fill_threshold) to tweak the threshold (currently at 50%). This feature is turned off by setting the parameter to something large, e.g. 1000.

Feel free to experiment with all the parameters and suggest alternative values!

comment:33 Changed 3 years ago by Klumbumbus

I tested a bit and I think the margin on both sides does not work so good as expected. There are problems especially when MPs are used. With margin on both sides:

  • you do not see which side is the inner side which makes the mapview more confusing (The case from comment:16 is in reality rare and mostly the inner side of unclosed areas is shown correct.)
  • you do not see different colors if they are not the same on both sides, see the following gif


Changed 3 years ago by Klumbumbus

Attachment: margin.gif added

Changed 3 years ago by Klumbumbus

Attachment: zoom.gif added

comment:34 Changed 3 years ago by Klumbumbus

Low zoom is also problematic when partial fill is activated and the dataset contains some uncomplete downloaded MPs, see attachment:zoom.gif

comment:35 Changed 3 years ago by bastiK

In 9071/josm:

mappaint partial fill: turn off margin on both sites for unclosed area (see #12104)

comment:36 in reply to:  33 Changed 3 years ago by bastiK

Replying to Klumbumbus:

I tested a bit and I think the margin on both sides does not work so good as expected. There are problems especially when MPs are used. With margin on both sides:

  • you do not see which side is the inner side which makes the mapview more confusing (The case from comment:16 is in reality rare and mostly the inner side of unclosed areas is shown correct.)
  • you do not see different colors if they are not the same on both sides, see the following gif

Okay, this is pretty convincing. I've turned off the margin on both sides.

comment:37 Changed 3 years ago by bastiK

In [9076]:

mapcss: partial fill - fix for [9063] - use scale instead of circum (otherwise it would depend on latitude)

comment:38 Changed 3 years ago by bastiK

In 9077/josm:

mapcss: partial fill - fix linecap (see #12104)

comment:39 in reply to:  13 ; Changed 3 years ago by bastiK

Replying to Klumbumbus:

The linecap is a bit strange in some cases.

Could you try again with [9077]?

comment:40 Changed 3 years ago by bastiK

In 9082/josm:

mapcss partial fill: remove [9061] code & turn it on again (see #12104)

Changed 3 years ago by Klumbumbus

Attachment: artefact1.png added

Changed 3 years ago by Klumbumbus

Attachment: artefact2.png added

Changed 3 years ago by Klumbumbus

Attachment: artefact3.png added

Changed 3 years ago by Klumbumbus

Attachment: partial fill test.osm added

comment:41 in reply to:  39 ; Changed 3 years ago by Klumbumbus

Replying to bastiK:

Replying to Klumbumbus:

The linecap is a bit strange in some cases.

Could you try again with [9077]?

There are some minor artefacts on lower zoom:



attachment:"partial fill test.osm" for testing

Changed 3 years ago by Klumbumbus

Attachment: artefact4.png added

comment:42 Changed 3 years ago by Klumbumbus


I tried to workaroud this issue on low zoom with open multipolygons (see also comment:34) by lowering the fill extend of open multipolygons with lower zoom. But it seems that it is not possible within mapcss, because the pseudoclasses :closed and :unclosed_multipolygon handle uncomplete and just not fully downloaded multipolygons different (which is good in other cases). Atleast I could not find a way that reduces the fill extend only for unclosed multipolygons, but not for closed multipolygons or for ways.

comment:43 in reply to:  41 ; Changed 3 years ago by bastiK

Replying to Klumbumbus:


I tried to workaroud this issue on low zoom with open multipolygons (see also comment:34) by lowering the fill extend of open multipolygons with lower zoom. But it seems that it is not possible within mapcss, because the pseudoclasses :closed and :unclosed_multipolygon handle uncomplete and just not fully downloaded multipolygons different (which is good in other cases).

Is it?

Atleast I could not find a way that reduces the fill extend only for unclosed multipolygons, but not for closed multipolygons or for ways.

Another option would be to also turn to ordinary fill for small unclosed areas. This would be like in [9063], but maybe with a different, very conservative threshold. When the user draws a building or parking lot, it shouldn't alternate between partial fill and complete fill depending on the current size of the area, this would be confusing.

Replying to Klumbumbus:

There are some minor artefacts on lower zoom

Yes, it turned out to be a rather difficult problem. So far, I could not find an algorithm, that works perfectly for all cases. The current code works well, when the extent is smaller than the distances of the end-nodes to all the other polygon edges (like in comment:13). This should be the case for downloaded incomplete multipolygons, except at low zoom.

When the user is drawing an area, there can be somewhat strange overlaps, as you show in the screenshots. At the moment, I think this is acceptable: It shouldn't happen too often and the area will be closed in the end - then it looks fine.

comment:44 in reply to:  43 ; Changed 3 years ago by Klumbumbus

Replying to bastiK:

Replying to Klumbumbus:


I tried to workaroud this issue on low zoom with open multipolygons (see also comment:34) by lowering the fill extend of open multipolygons with lower zoom. But it seems that it is not possible within mapcss, because the pseudoclasses :closed and :unclosed_multipolygon handle uncomplete and just not fully downloaded multipolygons different (which is good in other cases).

Is it?

Well, the pseudoclass :unclosed_multipolygon was once developed for validation purposes. There the difference between open multipolygons and actually complete but just uncomplete downloaded multipolygons was important. See ticket:10529#comment:68. However we could rethink the definition of these pseudoclasses. (Maybe split them up and make an own pseudoclass for :complete_downloaded, to make them independent!?)

comment:45 Changed 3 years ago by bastiK

In 9099/josm:

mapcss partial fill: move threshold parameter ([9063]) into the mapcss style
(new property fill-extent-threshold)

  • add support for this parameter when area is unclosed
  • smaller extent for unclosed areas

(see #12104)

comment:46 in reply to:  44 Changed 3 years ago by bastiK

Replying to Klumbumbus:

Well, the pseudoclass :unclosed_multipolygon was once developed for validation purposes. There the difference between open multipolygons and actually complete but just uncomplete downloaded multipolygons was important. See ticket:10529#comment:68. However we could rethink the definition of these pseudoclasses. (Maybe split them up and make an own pseudoclass for :complete_downloaded, to make them independent!?)

Thanks, I wasn't aware of this. I've added a pseudoclass :closed2, which ignores the fact that the multipolygon is downloaded completely and just checks for open ends. This is meant as an (internal) temporary solution, until we figure this out. In addition, there is a new pseudoclass :completely_downloaded in [9099], like you suggested.

As you can see, I've moved much of the code into the style sheet, so it should be easier to adapt. The extent for unclosed areas is now smaller - to visually distinguish from closed areas and (as a side effect) reduce the artifacts further.

comment:47 Changed 3 years ago by Klumbumbus

This is working very nice. Great job!
However I think we should use fill-extent-threshold: 50%; instead of 70%. The reason is that with partial fill it is much harder to see if a building way is properly aligned to the aerial imagery and with 70% a lot of "normal sized" buildings use the partial fill to early.

I think we should also make this parameter a josm_pref, so advanced users can adjust it (without editing the mapcss style and then forever use their outdated copy).

comment:48 Changed 3 years ago by bastiK

In 9103/josm:

mapcss partial fill: adjust threshold parameter for closed areas (see #12104)

comment:49 in reply to:  47 Changed 3 years ago by bastiK

Replying to Klumbumbus:

This is working very nice. Great job!
However I think we should use fill-extent-threshold: 50%; instead of 70%. The reason is that with partial fill it is much harder to see if a building way is properly aligned to the aerial imagery and with 70% a lot of "normal sized" buildings use the partial fill to early.

Okay, sure.

I think we should also make this parameter a josm_pref, so advanced users can adjust it (without editing the mapcss style and then forever use their outdated copy).

Yes, good idea.

comment:50 Changed 3 years ago by Klumbumbus

area[setting("partial_fill")]:closed2 {
    fill-extent: 25;
    fill-extent-threshold: JOSM_pref(partial_fill_extent_threshold, 50%);
}

Does not work. Do we need an additional prop() here?

comment:51 Changed 3 years ago by bastiK

Works for me, but we have to keep an eye on the performance. Preferences lookup was a reason for slowness before, it is probably better to add an intermediate cache for this.

comment:52 Changed 3 years ago by bastiK

In 9114/josm:

mapcss: make partial fill threshold an advanced preference option (see #12104)

comment:53 in reply to:  50 ; Changed 3 years ago by bastiK

Replying to Klumbumbus:

Does not work.

Okay, I had to make sure the style is actually updated, when the preference value is changed.

comment:54 in reply to:  53 Changed 3 years ago by Klumbumbus

Replying to bastiK:

Okay, I had to make sure the style is actually updated, when the preference value is changed.

So #10185 is fixed now, right?

comment:55 Changed 3 years ago by bastiK

Yes, should be. :)

comment:56 Changed 3 years ago by Don-vip

Resolution: fixed
Status: newclosed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.