Heatmap Fix for 3D terrain

[Samarth1696]

Fixes: #1081

There are 2 changes I have done in this PR:

1. Heatmap can now use Terrain data to get elevation, which fixes the floating heatmap issue.

2. Replaced the large single framebuffer implementation to per-tile framebuffer approach, where now each tile have its own framebuffer.

Before:
https://stackblitz.com/edit/web-platform-tqhkxm
After:
https://stackblitz.com/edit/stackblitz-starters-zmwkdx?file=index.html

The heatmap was already split into tiles, so I just had to change the implementation of the framebuffer and splitted the heatmap texture into tiles. And that worked well!!

This is the performance improvement that I got:

But I am not sure if the failing tests are acceptable and I was already expecting the heatmap render tests to fail... If you feel that we should not go with this implementation, then let me know so that I will create a new PR with just the floating heatmap fix!

Also that 4 tile tests in render tests were failing even before I started working on the issue.

Launch Checklist

• 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.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 95.27559% with 6 lines in your changes missing coverage. Please review.

Project coverage is 87.88%. Comparing base (b70ac2c) to head (a8d41f5).
Report is 1 commits behind head on main.

Files	Patch %	Lines
src/render/draw_heatmap.ts	96.72%	0 Missing and 4 partials ⚠️
src/style/style_layer/heatmap_style_layer.ts	60.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4571      +/-   ##
==========================================
- Coverage   87.90%   87.88%   -0.02%     
==========================================
  Files         247      247              
  Lines       33504    33574      +70     
  Branches     2373     2366       -7     
==========================================
+ Hits        29453    29508      +55     
- Misses       3078     3098      +20     
+ Partials      973      968       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[HarelM]

Thanks for taking the time to solve this!
Is this fix picky relevant for terrain or does it affect regular heatmap as well?
Will this be heavier in terms of memory usage?
I think the original issue with terrain is that we needed a texture pool to avoid memory issues on old iOS devices, will this create such stress as well?

[Samarth1696]

Is this fix picky relevant for terrain or does it affect regular heatmap as well?

The fix for the floating heatmap issue is specifically relevant to maps with terrain. However, the performance improvements from the per-tile framebuffer implementation benefit all heatmap renderings, including those on flat maps.

Will this be heavier in terms of memory usage?
I think the original issue with terrain is that we needed a texture pool to avoid memory issues on old iOS devices, will this create such stress as well?

The per-tile framebuffer approach does increase memory usage compared to the single large framebuffer, as we're now allocating multiple smaller textures instead of one large one. However, this increase is generally offset by two factors:
a) We only create framebuffers for visible tiles, which is typically fewer than the total number of tiles in the viewport.
b) The improved performance allows for faster rendering and potential memory savings in other areas.

After you came up with this question I tried to do chrome memory profiling between two different approach and I am not seeing any difference. Do we have any other instrumentation in place to do the memory profiling?

[HarelM]

What about releasing the buffers when the tiles are not needed any more (i.e. move the map around and zoom in and out to load more tiles)?
The previous test was simply made on an old iOS device (I think it was iPhone 6 back then), and simply see that it doesn't crash (white screen of death).

[HarelM]

@prozessor13 @kubapelc please let me know what you think of this PR, and also how it works affect the globe branch.

[kubapelc]

Hi, this looks really nice! However I have some performance concerns.

• Why does this new approach improve performance?
• The benchmark looks very fishy, the old mean time is over 3.4 ms, new mean time is 0.08 ms, which would make this 40 times faster. That does not sound like a realistic performance improvement. Maybe the benchmark is broken?
• Have you tested how this performs on a low-end or old android phone compared to the old heatmap? Mobile GPUs typically have much higher overhead when rendering to framebuffers than desktop GPUs, and have much lower memory bandwidth in general.

I think your approach is ideal for fixing the heatmaps-on-terrain bug, but I'm really concerned that this can be a performance regression for non-terrain maps. By my intuition, it should be slower than the old approach:

The per-tile textures you allocate are really large, typical tileSize is 512, and there are usually many tiles rendered, so much more pixels must be filled when rendering the kernels. The bandwidth usage when both prerendering the textures and the drawing the heatmaps from them would be much higher than the old approach. The overhead of switching FBOs many times must be paid every frame.

Contrast this with the old approach of rendering heatmaps: A single framebuffer is used, which is 1/4 the size of the screen in each axis. This is just 960x540 for a 4K screen, roughly the same amount of pixels as 2 tiles of 512x512. We only pay the overhead of switching framebuffers once per frame.

I suggest these improvements:

• Only use this new approach of rendering heatmaps when terrain is enabled.
• Use much lower texture size for the framebuffers. Go as low as you can, I think even 64x64 should work. Alternatively, use a texture atlas to avoid overhead of switching FBOs and textures (this would be much harder to implement though).
• Only re-draw the framebuffers if the heatmap layer changed - reuse the work from previous frames.

[kubapelc]

Isn't tile.fbo used for terrain's render-to-texture? How does this interact with what you draw into the tiles during heatmap rendering?

[Samarth1696]

Actually I replicated the approach of how the hillshade works. This is where it came from!
https://github.com/maplibre/maplibre-gl-js/blob/main/src/render/draw_hillshade.ts

[kubapelc]

I'm not sure why this works. Maybe @prozessor13 can clarify? Also shouldn't heatmaps use more that 8 bit precision for blending the kernel intensities?

[Samarth1696]

Yes, heatmap uses context.HALF_FLOAT which a 16-bit precision floating-point format whenever it is there. I removed gl.UNISIGNED_BYTE because as the performance was increasing I thought providing higher precision texture would be better but that was a mistake because after testing on the old Android device I explored a bit and found out that old Android devices don’t provide support for float in fragment shaders. I think that was the reason it was added before in the first place.
I think I should add gl.UNISIGNED_BYTE again.

[kubapelc]

It looks like current heatmap code checks whether 16 bit floats are supported and only uses them if they are, I think that is a good approach.

I also checked the render-to-texture and terrain code again, and I was wrong, tile.fbo is not used by terrain at all. But it is used by hillshade. What if you enable both heatmaps and hillshade?

If I understand your changes, both layers now use the same framebuffer object, both attach a texture to in (into the same attachment slot) and both could draw different things into the fbo, which could collide.

If that is really the case, I suggest using a different fbo for heatmaps. But I'm not sure how to best manage them, current tile.fbo seems to be deleted in raster_dem_tile_source.ts, so it is very much tied to hillshade.

[Samarth1696]

what if to use HashMap in layer.heatmapFbo? It will access the fbo based on the tilekey.

[kubapelc]

Yes, that sound like a good place!

[kubapelc]

As for globe integration, if this approach is only used for terrain in the end, then no additional work would be needed since globe doesn't support terrain (yet). Otherwise, adapting this to render on a sphere should be easy, but making sure that kernel sizes are consistent across latitudes on a globe would probably take a few hours.

[Samarth1696]

The benchmark looks very fishy, the old mean time is over 3.4 ms, new mean time is 0.08 ms, which would make this 40 times faster. That does not sound like a realistic performance improvement. Maybe the benchmark is broken?

Hmm.. yeah it maybe broken but I am not sure of it..

Have you tested how this performs on a low-end or old android phone compared to the old heatmap? Mobile GPUs typically have much higher overhead when rendering to framebuffers than desktop GPUs, and have much lower memory bandwidth in general.

This is running on Nexus One:

bandicam.2024-08-17.18-48-01-830.mp4

bandicam.2024-08-17.18-49-42-459.mp4

OLD APPROACH:

bandicam.2024-08-17.19-18-22-807.mp4

both of the approach gives white screen of death when refresh:

Only use this new approach of rendering heatmaps when terrain is enabled.

Then why use it for the terrain also if it does not benefit much?

Use much lower texture size for the framebuffers. Go as low as you can, I think even 64x64 should work. Alternatively, use a texture atlas to avoid overhead of switching FBOs and textures (this would be much harder to implement though).

Seems interesting.. we can work on a new PR to implement this method.

[kubapelc]

I missed a key fact about your approach, that when it is used alongside terrain, each tile will get rendered only when terrain prerenders a given tile to texture, and that terrain will then cache the finished colormap for the tile. So no rendering work needs to be done after this point. This would indeed make your approach very performant!

I'm still not sure how this would behave when terrain is disabled, and still think it might be best to use the old approach for that case, since it is performant and flexible, handling dynamic changes. I think you can disregard my other comments about lowering texture size and only redrawing when something changes, although it would probably be a good idea to see how this behaves with dynamic heatmaps, such as the example heatmap, which changes kernel intensity depending on zoom level. EDIT: your example already has dynamic kernel sizes, and it works well there.

Then why use it for the terrain also if it does not benefit much?

I think you approach is the best way to make heatmaps follow the terrain mesh. Alternative would be use the old approach and draw kernels with a mesh that would sample the heightmap and use Z-testing, which would probably not work very well at tile borders and produce artifact, since the heatmap FBO is so much smaller than the screen.

EDIT: maybe heatmaps should be added to the LAYERS table in render_to_texture.ts for the first paragraph to actually apply?

[PhungVanHoa]

heatmap-color not working in demo

[Samarth1696]

heatmap-color not working in demo

I am unable to reproduce this. Can you provide with the reproduction steps?

heatmap.mp4

[Samarth1696]

Thank you for the insightful review and feedback, @kubapelc! Your comments have been very helpful in understanding the implications of this approach, especially regarding terrain integration.
I will update my PR based on the changes needed and will let you know.

Also should we implement texture altas? I think it will benefit the heatmap as it can reduce the overhead but it would be difficult to implement as I have not worked with it before. I will need quite a good amount of time for it.
But does adding texture atlas impact much?

[kubapelc]

I think you can skip the texture atlas thing, it really would be difficult to implement and also has some drawbacks. I think it should not be needed, terrain already uses a single texture per tile instead of atlasing and it seems to be fine.

[PhungVanHoa]

heatmap-color not working in demo

I am unable to reproduce this. Can you provide with the reproduction steps?

heatmap.mp4

Sorry for lack of information. I mean the heatmap color doesn't change when i modify the heatmap-color option

[Samarth1696]

heatmap-color not working in demo

I am unable to reproduce this. Can you provide with the reproduction steps?
heatmap.mp4

Sorry for lack of information. I mean the heatmap color doesn't change when i modify the heatmap-color option

Maybe can you compare between the old approach and new approach to get the difference?

[HarelM]

I like the separation to different flows, it makes the previous code that worked being not affected, which is great.
I think I see some similarities between the flows that maybe share some methods.
I've added a few comments where I saw easy changes, but there might be more.
I would advise before further refactoring, add some render tests so that you'd know you are not breaking anything with the code changes.
These render tests are needed anyway for this PR to be merged, so better sooner than later :-)
Thanks!!

[Samarth1696]

Is the build size ok?

[HarelM]

Yes, please increase the build size.

[Samarth1696]

I think the build size is not needed to change now.

[HarelM]

The code looks good, The original issue
prozessor13#46 is talking about tile border issues, have you seen problem at the border of the tiles?

[Samarth1696]

I have not got problem as such for now.

[HarelM]

THANKS!!

[HarelM]

@Samarth1696 @kubapelc Can you help me merge this code to the globe branch?
I'm having a hard time reconciling the conflicts...

[kubapelc]

I'll take a look at it!