Enable property functions for more symbol paint properties #4226
Conversation
|
Ready for 👀 |
There was a problem hiding this comment.
This looks great! Thanks @anandthakker. One very quick revision and one code style opinion comment. Feel free to 
once those are taken care of.
src/render/painter.js
Outdated
| const fragmentSource = configuration.applyPragmas(definesSource + shaders.prelude.fragmentSource + definition.fragmentSource, 'fragment'); | ||
| const vertexSource = configuration.applyPragmas(definesSource + shaders.prelude.vertexSource + definition.vertexSource, 'vertex'); | ||
|
|
||
| // console.log(fragmentSource.split('\n').map((l, i) => `${i}: ${l}`).join('\n')) |
src/shaders/symbol_sdf.fragment.glsl
Outdated
| #define SDF_PX 8.0 | ||
| #define BLUR_OFFSET 1.19 | ||
| #define HALO_OFFSET 6.0 | ||
| #define GAMMA 0.105/DEVICE_PIXEL_RATIO |
There was a problem hiding this comment.
Opinion Below 👇
I find some of these constant names cryptic:
- What's the difference between
GAMMAandgamma? - What's meant by
SDF_PX? SDF pixels? - Why is
halp_blurmultiplied byBLUR_OFFSET? - What's a
buff? Is it a buffer?
I prefer a very clear constant name (ideally googleable) to a magic number, and a magic number to a cryptic constant name.
There was a problem hiding this comment.
LOL, I totally agree with you that these names are cryptic. And here I must confess, I just did a lazy/dumb transcription from draw_symbol.js (where these constants / calculations lived before this), without actually discerning what they meant. Lemme see if I can decipher them into more meaningful names.
There was a problem hiding this comment.
Thank you 🙇. If you can't come up with a meaningful name, feel free to inline.
There was a problem hiding this comment.
@lucaswoj I inlined a couple of them, but chose to leave SDF_PX and EDGE_GAMMA (my weak attempt at renaming GAMMA), because they are used in more than one place -- I did not want to inline them and then later lose track of the fact that both "8.0"s are supposed to mean the same thing. Filed a follow up ticket though: #4229
* Enable property functions for {text,icon}-opacity
* Add property-function tests for {text,icon}-opacity
* Enable property functions for *-halo-{width,blur}
* Add render tests for {text,icon}-halo-{blur,width}
* Skip halo-blur/width tests on native
* Remove stray debugging statement
* Very slightly demistify a couple of constants
* Revert unchanged render test images
Add property function support for
icon-opacitytext-opacityicon-halo-blurtext-halo-bluricon-halo-widthtext-halo-width