Skip to content
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

Add some missing math expressions #5853

Open
4 of 11 tasks
anandthakker opened this issue Dec 13, 2017 · 11 comments
Open
4 of 11 tasks

Add some missing math expressions #5853

anandthakker opened this issue Dec 13, 2017 · 11 comments
Labels
api 📝 cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.) feature 🍏

Comments

@anandthakker
Copy link
Contributor

anandthakker commented Dec 13, 2017

@anandthakker anandthakker added api 📝 cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.) feature 🍏 labels Dec 13, 2017
@Scarysize
Copy link

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

@jfirebaugh
Copy link
Contributor

jfirebaugh commented Dec 13, 2017

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
Copy link
Contributor Author

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
Copy link
Contributor

jfirebaugh commented Dec 13, 2017

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

@Scarysize
Copy link

Make sense! Thanks for the explanation :)

@1ec5
Copy link
Contributor

1ec5 commented Dec 19, 2017

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

  • avg returns the arithmetic mean (average) value in an array.
  • median returns the median value in an array.
  • mode returns the most frequent value in an array.
  • stddev returns the standard deviation in an array.
  • exp returns e raised to a number.

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

@kekscom
Copy link

kekscom commented Mar 22, 2018

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
Copy link
Contributor

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
Copy link
Contributor

1ec5 commented Apr 10, 2018

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 #4119 (comment).

anandthakker added a commit that referenced this issue Apr 11, 2018
Refs #5853

* Add abs, round, floor, ceil operators

* Round away from 0

* Add entries to v8.json

* Test non-integer abs

* Edits from review
@1ec5
Copy link
Contributor

1ec5 commented Apr 21, 2018

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:

  • The buildings are actually the same height, which happens in cities with height restrictions, such as Washington, D.C.
  • The buildings in OSM are tagged with building:levels rather than height, so the Streets source assumes 3 meters per level.
  • The buildings in OSM lack floor counts or heights, so the Streets source assumes one floor or 3 meters.

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 #5853 (comment). 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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api 📝 cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.) feature 🍏
Projects
None yet
Development

No branches or pull requests

7 participants