Remove paint class support

[anandthakker]

Prerequisites:

• Merge upstream PR Smart setStyle (part 1) #3621 and then rebase here
• -test-suite: merge Ignore paint-class tests in gl-js mapbox-gl-test-suite#178 (Ignore paint class tests in gl-js)
• -style-spec: merge Drop paint classes mapbox-gl-style-spec#576 (update docs and spec)
• Fix bug wherein unsetting and resetting fill-outline-color causes TypeError to be thrown Fix bug wherein unsetting and resetting fill-outline-color causes TypeError to be thrown #3657
• Add setStyle examples to the docs #3660

UPDATE: Removed #3662 (comment) from the prerequisites, in light of setSprite being (a) orthogonal to paint classes, and (b) a pretty complicated / significant lift.

[anandthakker]

Hmm... what is the right thing to do in -style-spec alongside this change?

Issue: paint.* is referenced in the spec definition and the docs

Options:

1. Remove from spec & docs, bump to v9
2. Leave in spec--with deprecation notice--and update docs to point to declass for usage of paint.*.
3. Something else?

[anandthakker]

I guess now that I've written that, I'm leaning towards option 2

[anandthakker]

Also TODO: remove classes tests from the test-suite. Updating top of ticket to reflect this and the -style-spec change.

[anandthakker]

Rebased off of master instead of being branched off an open PR

[anandthakker]

Rebased onto master to fixup minor merge conflict

[anandthakker]

@jfirebaugh @lucaswoj updated top of ticket w/ the prerequisites we discussed.

[averas]

I use paint classes quite extensively and just stumbled across this. Do you think it's justified to mention in the documentation that paint class support will be deprecated so that we who use it start migrating away from it and new users stay away from it?

[lucaswoj]

Absolutely @averas. I'll do that ASAP.

[anandthakker]

Deprecation flagged here: #4038

[anandthakker]

At this point, all of the technical blockers for merging this have been cleared -- that is, I believe it is now possible to losslessly replace 100% of the use cases of paint classes with map.setStyle().

So, question: should we look to merge this now (after a rebase/resolve), or should we consider adding setStyle examples to the docs (https://github.com/mapbox/mapbox-gl-js/issues/3660) to be a prerequisite for merging this PR?

cc @lucaswoj @jfirebaugh

[lucaswoj]

I support considering adding Map#setStyle examples to the docs (#3660) to be a prerequisite for merging this PR.

Doing so will give us more confidence that our proposed alternative is sound, give our users more resources to make the transition, and give support more materials to reference.

[averas]

I am currently migrating away from paint classes in my own applications so I thought I'd share the challenges that I've hit with the new smart setStyle() (which by the way has tons of other benefits):

Keeping GeoJSON sources in sync when the user switches style

I would say that it's fairly common that you have "base layers" that constitute the backdrop of your map, and on top of that you have some "custom layers" containing data that you visualise on your map. Sometimes different backdrops, such as with higher contrast, or with a satellite raster fits better, and a common use case is allowing the user to flip the backdrop while retaining the custom layers.

I used to fulfil this use case quite straightforwardly using paint classes. Any mutations to my "custom layers" would only require that I updated a single set of sources which allowed me to easily implement tons of filtering etc.

With smart setStyle() this is not as straight forward, as you need to manually keep your sources in sync, either by copying the sources from the style you're about to replace, to the style you're replacing it with, or by maintaining the "single-source-of-truth" outside of the style sources.

Except that copying sources is tedious I think it may become a performance issue as well. It isn't currently so for me, but if you copy a 70-80MB GeoJSON back and fourth between styles it may become one (even if you pass the GeoJSON by reference and really don't copy it per se, I would imagine Mapbox GL JS still would have to parse it every time).

It's not just sources, sometimes you want to keep custom layers in sync as well

Some of the layers I have in my applications have their paint properties set dynamically based on user interaction (for example the color stops of a fill). This requires me to copy such layers much like with the sources described above. When I had paint classes I simply let such multi-style-layers have paint properties defined without classes (so they were active regardless of class).

[anandthakker]

Thanks for this helpful feedback, @averas !

Keeping GeoJSON sources in sync when the user switches style

Except that copying sources is tedious I think it may become a performance issue as well. It isn't currently so for me, but if you copy a 70-80MB GeoJSON back and fourth between styles it may become one.

Interesting point -- I agree that this is an issue that isn't well addressed by the current setStyle API. I'll file a separate ticket.

[anandthakker]

Followed up on the GeoJSON source syncing issue here: #4006 (comment)

[indus]

The deprecation of paint classes hit me hard and for me the new setStyle is not a sufficient replacement when you deal with toggleable layers and realtime data at the same time.

[anandthakker]

Sorry to hear that, @indus. Our intention was for "smart setStyle" to be sufficient (thus our waiting until it supported changes to sprite:, for example); if there are specific use cases where it's not, please do open a new issue!