bryceco / GoMap

OpenStreetMap editor for iPhone/iPad
ISC License
301 stars 40 forks source link

Copy iD's theme #734

Closed verhovsky closed 2 months ago

verhovsky commented 3 months ago

Closes #730

Lines/areas in iD are SVG path elements. Its theme works by converting OSM tag data to a list of CSS classes for each element

https://github.com/openstreetmap/iD/blob/a38d7a1b8d50a3e358f4e0f0a5ae5a42b3d7cf58/modules/svg/tag_classes.js#L41-L168

and then a list of CSS rules for coloring those classes

https://github.com/openstreetmap/iD/tree/a38d7a1b8d50a3e358f4e0f0a5ae5a42b3d7cf58/css

Everything is either a .line or a .area, then there are 4 sub-classes: .casing, .stroke, .fill and .shadow. We ignore .shadow and .fill only applies to .area. iD implements everything as lines, the .fill is also a line, a wide, transparent one that is only on the inside of areas and it can also have a pattern of icons, like a bunch of trees for natural=wood. I didn't implement these icons and I can't see an easy way in Swift to have the fill stop after a while to keep the center of the area transparent, though this would be nice as painting entire forest areas green means that your entire screen is green all the time when you're mapping something in the woods which is unhelpful/annoying.

Converting the code that converts OSM tag data to a list of classes is easy (ChatGPT). To convert the CSS to Swift we have to extract all the CSS selectors and re-implement the relevant CSS matching rules as Swift code. I wrote a Python script to generate if statements from iD's CSS selectors which will hopefully be at least somewhat reproducible if they ever change the theme.

Other things that are not implemented:

Also, there's some bug where things that are in a large relation (a natural=wood in my case) don't get their inner lines colored sometimes. Sometimes they get colored eventually after moving around the map more, but often not. I'm guessing the relation happens to not be loaded because it's far away or something when the coloring of the node is being determined. You don't notice this with the current theme because natural=wood boundary lines are the default black line but now that they're supposed to be green you can notice it.

Marked as draft because it's slow

bryceco commented 3 months ago

Wow, this looks like it was a lot of work. I glanced through it and saw two things:

However: the RenderInfo computation should only happen once per object so once everything is loaded it shouldn't affect scrolling speed 🤷‍♂️

verhovsky commented 3 months ago

I did some timing

        let start = Date()

        let duration = Date().timeIntervalSince(start) * 1000
        print(String(format: "colored in %.6f milliseconds", duration))

and it goes through the if statements in a millisecond or less on my M1 Macbook Air (laptop)

classified in 0.025034 milliseconds
colored in 0.138998 milliseconds
classified in 0.024915 milliseconds
colored in 0.142097 milliseconds
classified in 0.133991 milliseconds
colored in 0.053048 milliseconds
classified in 0.025988 milliseconds
colored in 0.139952 milliseconds
classified in 0.025034 milliseconds
colored in 0.138998 milliseconds
classified in 0.024915 milliseconds
colored in 0.138044 milliseconds
classified in 0.025988 milliseconds
colored in 0.138998 milliseconds

so this is actually not that slow. For comparison, the current theme runs renderInfoForObject() in 0.002 ms:

got render 0.005007 ms
got render 0.002980 ms
got render 0.001907 ms
got render 0.002980 ms
got render 0.002027 ms
got render 0.002027 ms
got render 0.002027 ms

So this PR is 25-200x slower

tordans commented 3 months ago

@verhovsky did you look at the FPS test in GoMap already? There should be a flag AFAIR https://github.com/bryceco/GoMap/issues/218 .

bryceco commented 3 months ago

On my 15 Pro it gets 60fps, but I haven't tried it on my older test devices. I'm traveling so I can't report on those for a while.

bryceco commented 3 months ago

This is looking pretty nice! It seems like when zoomed in a lot the dashes are overly large. I tried scaling them down by a factor of 3 and I think it looks better.

bryceco commented 3 months ago

This footway bridge looks weird when zoomed out: https://github.com/bryceco/GoMap/assets/279734/8372427b-1d7b-45f3-b3c5-7ab2727015c2

bryceco commented 3 months ago

Waterways are now very similar to the color we use for selected objects, so we need a new selection color. image

bryceco commented 2 months ago

On my 15 Pro it gets 60fps, but I haven't tried it on my older test devices. I'm traveling so I can't report on those for a while.

I just tested this PR on an iPhone 7 and it gets 60 FPS there (although just barely, in Debug mode it's ~50 FPS, down from ~58 FPS for the current build).

verhovsky commented 2 months ago

Looking into this

Also, there's some bug where things that are in a large relation (a natural=wood in my case) don't get their inner lines colored sometimes. Sometimes they get colored eventually after moving around the map more, but often not. I'm guessing the relation happens to not be loaded because it's far away or something when the coloring of the node is being determined. You don't notice this with the current theme because natural=wood boundary lines are the default black line but now that they're supposed to be green you can notice it.

It's because here

https://github.com/bryceco/GoMap/blob/56d8ea49697f19c53aca8d28fe6d039801ab74aa/src/Shared/EditorLayer/RenderInfo.swift#L241

you're only inheriting tags for type=boundary but in my case they're type=multipolygon and the tags should be inherited, then if's an inner it needs to be a mask of transparency, if it's outer it needs to be a filled area again. There's a probably related issue with multipolygons for water, that coastline line in your screenshot is not supposed to be that ultra pure #0000ff blue color. Though even if I change it to isMultipolygon it's not fixed. That color doesn't appear in Theme.swift, it's coming from somewhere else.

bryceco commented 2 months ago

That color doesn't appear in Theme.swift, it's coming from somewhere else.

It's coming from https://github.com/bryceco/GoMap/blob/65266f948768fbf628fb3778783e9b8702433eff/src/Shared/EditorLayer/EditorMapLayer%2BOcean.swift#L592 which is the output of a large tangle of code that attempts to build a multipolygon when not all of the members of a relation have been downloaded. This is only necessary if we're going to fill the area, and the heuristics it uses aren't reliable, so it would also be acceptable to delete all that code and not fill water multipolygons unless all members are downloaded.

bryceco commented 2 months ago

After the divide-by-2 change casings are only 0.5 which I think is too small. They're really hard to see in some situations, e.g. sidewalks.

Also could you pull in the DarkMode code I filed a PR with?

bryceco commented 2 months ago

Getting rid of the classes array is awesome!

bryceco commented 2 months ago

Merged this, with a few changes: