Skip to content

Conversation

@NathanMOlson
Copy link
Collaborator

@NathanMOlson NathanMOlson commented Apr 11, 2025

Resolves #5666 (Hypsometric Tint from terrain-RGB tiles). Supporting style spec changes are here: https://github.com/NathanMOlson/maplibre-style-spec/tree/color_relief

Demo here: https://nathanmolson.github.io/color_relief

The shader uses a binary search to find the adjacent color stops from the color ramp, which means that larger color palettes take longer to render (O(log(n))).

Examples:

cr_linz
cr_geo
cr_iso
cr_rainbow
cr_cat

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

@NathanMOlson
Copy link
Collaborator Author

Benchmarks: ColorReliefLoad takes approximately the same amount of time as HillshadeLoad:

bench_hillshade
bench_cr

@NathanMOlson NathanMOlson marked this pull request as ready for review May 15, 2025 18:46
@HarelM
Copy link
Collaborator

HarelM commented May 17, 2025

Thanks for this!
I've just released a new version of the style spec, can you use it here so I can review the code with more ease (it has test failures comments).

@mwilsnd
Copy link
Collaborator

mwilsnd commented May 19, 2025

I didn't move away from the colorramp texture for performance reasons, but to support sharp color breaks (see "LINZ" and "categorical" style examples in the demo).

Right, I see your newer reply to Harel from earlier explaining the additional requirement. Looking at the defined stops for the LINZ and categorical ramps, your decision makes sense.

@NathanMOlson
Copy link
Collaborator Author

The shaders look reasonable to me, though I'm wondering more about the limitations of a texture driven approach.

The current approach could swap the uniform array for a texture without any conceptual changes. The main reason I chose a uniform array instead of a texture is for code clarity (I use a UniformColorArray instead of a Texture, which is basically just a binary blob. I think this better conveys the code intent, both in TypeScript and in the shader.) I also expect it to perform better.

I do see one advantage to using a texture, which is that the minimum GL_MAX_TEXTURE_SIZE is 64, compared to the minimum GL_MAX_FRAGMENT_UNIFORM_VECTORS, which is 16. I'm not sure how likely we are to see such low values for GL_MAX_FRAGMENT_UNIFORM_VECTORS, all the hardware I have access to has GL_MAX_FRAGMENT_UNIFORM_VECTORS = 1024.

So it feels like a trade-off between "code clarity and performance in typical use-cases and environments", vs "correctness on very limited hardware or color ramps with a huge number of colors".

@mwilsnd
Copy link
Collaborator

mwilsnd commented May 19, 2025

So it feels like a trade-off between "code clarity and performance in typical use-cases and environments", vs "correctness on very limited hardware or color ramps with a huge number of colors".

I agree, and also expect there are devices still in use which run closer to the lower bound of these limits. It may be worthwhile to examine mobile device hardware surveys and inspect the feature sets supported by devices in the longer tail end of active support. WebGL 2, featuring a much more useful limit, offers a decent filter on supported minimums, but I'd be wary of anything that still requires original WebGL.

@NathanMOlson NathanMOlson marked this pull request as draft May 19, 2025 21:36
@NathanMOlson
Copy link
Collaborator Author

Marked as draft while I create a version that uses textures for comparison.

@NathanMOlson
Copy link
Collaborator Author

NathanMOlson commented May 20, 2025

I got a version working with textures (#5913). For clarity, you can see the differences here: NathanMOlson#5.

Here are the benchmarks for the version using uniforms:
uniform256
uniform2

And using textures:
texture256
texture2

The version using uniforms scores 2% faster on a 2-color color ramp, and 13% faster on a 256-color color ramp.

I think we should probably use the texture version so we can support WebGL1 on low-end devices. I'll start cleaning that version up to replace this PR unless I hear otherwise.

@NathanMOlson
Copy link
Collaborator Author

The newest version that uses textures is even closer in performance:
image
image

@NathanMOlson
Copy link
Collaborator Author

Superceded by #5913.

@HarelM
Copy link
Collaborator

HarelM commented May 20, 2025

I'm a bit lost with all the branches and PRs.
Please let me know which one to review and close the others.

@NathanMOlson
Copy link
Collaborator Author

@HarelM please review #5913.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hypsometric Tint from terrain-RGB tiles

3 participants