mapbox / mapbox-gl-js

Interactive, thoroughly customizable maps in the browser, powered by vector tiles and WebGL
https://docs.mapbox.com/mapbox-gl-js/
Other
11.25k stars 2.23k forks source link

Add some missing math expressions #5853

Open anandthakker opened 6 years ago

anandthakker commented 6 years ago
Scarysize commented 6 years ago

Would be a random expression something which fits into this issue?

jfirebaugh commented 6 years ago

I don't think we should allow expressions to be non-deterministic. That would produce unpredictable results across tiles and complicate static rendering and caching. If you want a "random" value, generate it when you generate your data and make it a feature property.

anandthakker commented 6 years ago

I don't think we should allow expressions to be non-deterministic. That would produce unpredictable results across tiles and complicate static rendering and caching.

Hm, yeah, I agree that the static rendering / caching issues are problematic, but would unpredictable rendering across tiles be problematic?

An (admittedly weird) alternative would be to provide (a) a ["get-random-integer", index: number] that (in spirit) just looks up an entry in a (static) random number table, and (b) some sort of hashing expression that users could use to convert ["id"] or ["get", "some-property"] into an index to use with (a).

jfirebaugh commented 6 years ago

would unpredictable rendering across tiles be problematic?

If random was used in any fill or line property, a likely expectation would be that it produces the same value for an entire logical feature, when in fact it will not match up when that feature is split across tiles.

Other problematic behaviors:

Scarysize commented 6 years ago

Make sense! Thanks for the explanation :)

1ec5 commented 6 years ago

In addition to the operators mentioned above, the following expression operators would improve compatibility with NSExpression, which would benefit the iOS and macOS SDKs:

mapbox/mapbox-gl-native#10726 synthesizes avg, exp, abs, trunc, floor, and ceiling based on existing operators, but the others require native support.

kekscom commented 6 years ago

A practical use case would be variance to 'fill-extrusion-height' in order to avoid z-fighting for colored roof faces. It would be more problematic to add this to data but effectively it is a fix for something the engine doesn't seem to handle properly.

samanpwbb commented 6 years ago

Basic unit conversion from feet to meters or vice-versa would benefit tremendously from this change. Here's what happens now:

screen shot 2018-04-10 at 1 53 10 pm
1ec5 commented 6 years ago

round would be sufficient for number formatting in English, but almost any other language would require a dedicated number formatting operator: #4119. Implementing round without format-number would only slightly simplify the gargantuan expression in https://github.com/mapbox/mapbox-gl-js/issues/4119#issuecomment-373265865.

1ec5 commented 6 years ago

A practical use case would be variance to 'fill-extrusion-height' in order to avoid z-fighting for colored roof faces.

Along these lines, a random operator would enable a style author to dither the heights of neighboring building extrusions, making them more distinct than they would be if their roofs were completely level with each other. This is an approach taken by F4 Map, among others. (F4 further mitigates the problem by supporting non-level roof shapes: #3998.)

The Mapbox Streets source often reports identical height values on adjacent building features for any of the following reasons:

It isn’t necessarily appropriate for the Streets source to dither the values. This field might be used for other purposes. Moreover, the amount of dithering might need to vary based on the extrusion layer’s opacity or the camera’s zoom level or pitch.

If random was used in any fill or line property, a likely expectation would be that it produces the same value for an entire logical feature, when in fact it will not match up when that feature is split across tiles.

For the building height dithering use case, It’s already problematic to split a building feature across tiles for other reasons. This is why the 3D Buildings tileset has buildings span tile boundaries instead of being split across them. The lack of a random operator can be worked around by hashing on the feature’s ID or the feature’s other properties. This requires each adjacent feature to have a different ID, which fortunately is the case in the 3D Buildings tileset.

Perhaps this workaround points to a way we can implement a random operator without all the problems mentioned in https://github.com/mapbox/mapbox-gl-js/issues/5853#issuecomment-351452558. The style author probably doesn’t expect the random number generator to be cryptographically strong in any way, so we could implement it in a way that would be unique per feature, thus increasing stability across zoom levels and even (if based on feature IDs) across tile boundaries.

skorasaurus commented 6 years ago

would unpredictable rendering across tiles be problematic?

If random was used in any fill or line property, a likely expectation would be that it produces the same value for an entire logical feature, when in fact it will not match up when that feature is split across tiles.

Other problematic behaviors:

* It'll produce different values for the same feature at different zoom levels

* It'll produce different values for the same feature in the same tile, if relayout is triggered

I think the random can be used effectively to create slight variation when it's applied in a minimal manner. For example some times you want minimal variation to your map to prevent it from appearing 'sterile' or too consistent; @1ec5 makes some great points above regarding 3d buildings where they all look exactly the same. Perhaps you want to introduce some slight variation in building height, or color, but not too much.