Fixes related to console errors and some typings improvement in terrain branch

[HarelM]

Fixes #1180 and #1181.
@wipfli @prozessor13 Feel free to review.

[prozessor13]

Thanks for this pull request! Great Work :)

To fix the [0, 0] thing, i think there needs a change in https://github.com/maplibre/maplibre-gl-js/blob/terrain3d/src/render/terrain.ts#L329. Please try:

gl.readPixels(p.x, painter.height / devicePixelRatio - p.y - 1, 1, 1, gl.RGBA, gl.UNSIGNED_BYTE, rgba);

It looks like that the y-coordinate is calculated wrong (from 1 to height, instead of 0 to height-1), so the py - 1 should do the trick. Hopefully with this fix the map gets more stable under the mousepointer when panning in heavy tilted terrain.

[HarelM]

I'll try and report back, thanks!!

[HarelM]

@prozessor13 thanks for the input! adding a - 1 seems to solve the issue.
However, I wanted to create a test to imitate the code that runs but was unable to fully understand how webgl framebuffers work :-(
The following is the test I started for the terrain.ts file i.e. terrain.test.ts:

import Point from "@mapbox/point-geometry";
import Terrain from "./terrain";
import gl from 'gl';
import Context from "../gl/context";

describe('Terrain', () => {
    test('pointCoordiate should not return null', () => {
        const style = {
            map: {
                painter: {
                    context: new Context(gl(10, 10)),
                    width: 10,
                    height: 10
                }
            }
        } as any;
        const terrain = new Terrain(style, {} as any, {} as any);
        
        const coordinate = terrain.pointCoordinate(new Point(0,0));

        expect(coordinate).not.toBeNull();
    });
});

Any help would be appreciated in order to make the test fail when the - 1 is missing and passing when we add the - 1.
I know I should ramp up with webgl etc going forward, but that's the best I can do at the moment.
I guess there's a need to fill the _fbo and the _coordsIndex the right way, which is done by the program and not the code here, but we should be able to mock that in this test I believe...
Feel free to push a test file directly to this branch if you happen to work on it.
I'll continue tomorrow I believe.

[prozessor13]

Hmm. To get this test working i think it would be the easiest to fake all webgl classes/opjects, like: (NOTE: this code is not teststed!)

describe('Terrain', () => {
    test('pointCoordiate should not return null', () => {
        const x = null, y = null, tileID = null, framebuffers = [];
        const style = {
            map: {
                painter: {
                    height: 10,
                    context: {
                        gl: {
                            readPixels: (_x, _y) => {
                                x = _x;
                                y = _y;
                            }
                        }
                    },
                    bindFramebuffer: {
                        set: fb => {
                            framebuffers.push(fb);
                        }
                    }
                }
            }
        } as any;
        const sourceCache = {
            getTileByID: id => {
                tileID = id;
                return new OverscaledTileID(1, 0, 1, 0, 0);
            }
        } as any;
        const terrain = new Terrain(style, sourceCache, {} as any);
        terrain.getFramebuffer = fb => {
            return { framebuffer: fb };
        };
        terrain.getElevation = () => 123;
        for (let i = 0; i < 256; i++) terrain._coordsIndex[i] = i;
        
        const coordinate = terrain.pointCoordinate(new Point(0,0));

        expect(coordinate).not.toBeNull();
        // check others: x, y, tileID, framebuffers, coordinate
    });
});

[HarelM]

I've started with mocking a lot of things but I saw that I need to mock too many stuff.
I prefer not to mock a function in a class I test to make sure it works as closely to the original and only mock external dependencies.
Having said that, your approach might be faster.
P.S. _coordsIndex is being filled outside this class, right? Shouldn't it be a public and a not private member?

[prozessor13]

Yes you are right, _coordsIndex should be public.

To test the math behind terrain.pointCoordinate i think it is fine to write these mockups, and to test the whole webgl stuff i think it will much easier to write integration-tests and compare results visually. Doing this it would be easy to implement a debug flag, which then writes the coords/depth framebuffers to screen.
But the problem with integration tests are, that they are not working when terrain is enabled. Last week I debugged several hours to make integration tests working without success. I will create a ticket for this.

[HarelM]

In general, testing the whole webgl stuff in integration tests is probably the right approach, having said that, I think that in our specific case using a single framebuffer should be something we should be able to mock - not just for the sake of not mocking class function but also since it's a good reference to how to be able to mock low level things.
I agree that if it is super complicated and the test code becomes very complex it is not the right direction, but I think that using the gl package in unit tests and using actual data inside the buffers can help imitating these exact interesting scenarios in unit tests which is super powerful.
I know I personally need to better educate myself in this, but if you could help out in instructing me how to get data into a framebuffer in the test code it can help me a lot.

[prozessor13]

Ok good point! A framebuffer reads/writes data to a texture or to the screen (which is may internally a texture as well, dont know?). So the way to prepare a mock framebuffer would be:

• create a RGBAImage, with already containing the mock-data, in our case the coords-data (= rgba encoded tile-coordinates)
• create a Texture for the RGBAImage
• create a framebuffer with the size of the RGBAImage
• attach the texture to the framebuffer via colorAttachment.set

With this scenario, i think it should be theretically possible to get the pointCoordinate thing working.
Starting with an empty texture and writing data into it (via the framebuffer) has to be done via draw calls. But draw-calls are even more complex. You need shaders, vertexbuffers and so on. We can make a call to discuss this in detail, but please next week :)

[HarelM]

Ok, I think I managed to make it work, I'm not sure I'm 100% proud of the mocking I did, but when I remove the -1 the test fails which is what I wanted to achieve...
I think this is ready for review.

[github-actions]

Bundle size report:

Size Change: +23 B
Total Size Before: 202 kB
Total Size After: 202 kB

Output file	Before	After	Change
maplibre-gl.js	193 kB	193 kB	+23 B
maplibre-gl.css	9.41 kB	9.41 kB	0 B

ℹ️ View Details

Source file	Before	After	Change
src/render/program/program_uniforms.ts	926 B	952 B	+26 B
src/ui/marker.ts	2.87 kB	2.88 kB	+15 B
src/render/painter.ts	4.02 kB	4.03 kB	+12 B
src/render/draw_fill_extrusion.ts	830 B	840 B	+10 B
src/geo/transform.ts	4.43 kB	4.43 kB	+8 B
src/render/line_atlas.ts	981 B	988 B	+7 B
src/ui/camera.ts	3 kB	3.01 kB	+7 B
src/render/program/line_program.ts	1.15 kB	1.16 kB	+6 B
src/style/load_glyph_range.ts	218 B	223 B	+5 B
src/gl/framebuffer.ts	216 B	221 B	+5 B
src/render/draw_symbol.ts	2.59 kB	2.59 kB	+5 B
src/render/draw_heatmap.ts	1.03 kB	1.04 kB	+5 B
src/ui/handler/touch_pan.ts	435 B	440 B	+5 B
src/ui/map.ts	6.33 kB	6.33 kB	+5 B
src/ui/control/fullscreen_control.ts	852 B	857 B	+5 B
src/util/web_worker.ts	39 B	43 B	+4 B
src/source/pixels_to_tile_units.ts	101 B	105 B	+4 B
src/data/pos_attributes.ts	84 B	88 B	+4 B
src/shaders/shaders.ts	1.48 kB	1.48 kB	+4 B
src/render/program/clipping_mask_program.ts	103 B	107 B	+4 B
src/ui/control/terrain_control.ts	451 B	455 B	+4 B
node_modules/@mapbox/tiny-sdf/index.js	1.1 kB	1.11 kB	+3 B
src/shaders/hillshade.vertex.glsl.g.ts	136 B	139 B	+3 B
src/render/program/debug_program.ts	188 B	191 B	+3 B
src/gl/cull_face_mode.ts	154 B	157 B	+3 B
node_modules/gl-matrix/esm/mat2.js	224 B	227 B	+3 B
src/source/vector_tile_source.ts	1.11 kB	1.11 kB	+2 B
src/source/geojson_source.ts	1.29 kB	1.29 kB	+2 B
src/data/raster_bounds_attributes.ts	96 B	98 B	+2 B
src/source/image_source.ts	1.1 kB	1.11 kB	+2 B
src/source/tile_cache.ts	555 B	557 B	+2 B
src/symbol/grid_index.ts	1.35 kB	1.35 kB	+2 B
src/render/terrain.ts	2.26 kB	2.27 kB	+2 B
src/style/load_sprite.ts	407 B	409 B	+2 B
src/shaders/circle.fragment.glsl.g.ts	408 B	410 B	+2 B
src/shaders/fill_pattern.vertex.glsl.g.ts	387 B	389 B	+2 B
src/shaders/symbol_icon.vertex.glsl.g.ts	947 B	949 B	+2 B
src/shaders/symbol_sdf.fragment.glsl.g.ts	552 B	554 B	+2 B
src/render/program/pattern.ts	614 B	616 B	+2 B
src/ui/handler_inertia.ts	822 B	824 B	+2 B
src/ui/handler/tap_recognizer.ts	532 B	534 B	+2 B
src/ui/handler/shim/drag_pan.ts	221 B	223 B	+2 B
src/ui/default_locale.ts	312 B	314 B	+2 B
src/ui/control/geolocate_control.ts	2.26 kB	2.26 kB	+2 B
node_modules/@mapbox/mapbox-gl-supported/index.js	974 B	975 B	+1 B
src/style-spec/util/deep_equal.ts	193 B	194 B	+1 B
src/util/request_manager.ts	351 B	352 B	+1 B
node_modules/gl-matrix/esm/vec2.js	225 B	226 B	+1 B
src/data/bucket.ts	194 B	195 B	+1 B
src/source/source_cache.ts	3.99 kB	3.99 kB	+1 B
src/util/global_worker_pool.ts	313 B	314 B	+1 B
src/shaders/heatmap.vertex.glsl.g.ts	368 B	369 B	+1 B
src/shaders/collision_box.fragment.glsl.g.ts	147 B	148 B	+1 B
src/shaders/collision_circle.vertex.glsl.g.ts	545 B	546 B	+1 B
src/shaders/fill_extrusion_pattern.fragment.glsl.g.ts	415 B	416 B	+1 B
src/shaders/line.vertex.glsl.g.ts	707 B	708 B	+1 B
src/shaders/line_pattern.fragment.glsl.g.ts	705 B	706 B	+1 B
src/shaders/raster.vertex.glsl.g.ts	201 B	202 B	+1 B
src/shaders/symbol_text_and_icon.fragment.glsl.g.ts	596 B	597 B	+1 B
src/render/vertex_array_object.ts	580 B	581 B	+1 B
node_modules/gl-matrix/esm/mat3.js	221 B	222 B	+1 B
src/render/program/collision_program.ts	723 B	724 B	+1 B
src/render/draw_circle.ts	612 B	613 B	+1 B
src/render/draw_line.ts	1.04 kB	1.04 kB	+1 B
src/util/throttle.ts	144 B	145 B	+1 B
src/render/texture.ts	682 B	681 B	-1 B
src/source/tile_bounds.ts	317 B	316 B	-1 B
src/symbol/projection.ts	1.82 kB	1.82 kB	-1 B
src/shaders/circle.vertex.glsl.g.ts	559 B	558 B	-1 B
src/shaders/clipping_mask.fragment.glsl.g.ts	61 B	60 B	-1 B
src/shaders/heatmap.fragment.glsl.g.ts	267 B	266 B	-1 B
src/shaders/fill.fragment.glsl.g.ts	178 B	177 B	-1 B
src/shaders/hillshade.fragment.glsl.g.ts	554 B	553 B	-1 B
src/shaders/raster.fragment.glsl.g.ts	439 B	438 B	-1 B
src/shaders/symbol_sdf.vertex.glsl.g.ts	1.01 kB	1.01 kB	-1 B
src/shaders/terrain_depth.fragment.glsl.g.ts	197 B	196 B	-1 B
src/render/program/fill_extrusion_program.ts	798 B	797 B	-1 B
src/render/program/raster_program.ts	563 B	562 B	-1 B
src/gl/vertex_buffer.ts	541 B	540 B	-1 B
src/render/draw_collision_debug.ts	1.13 kB	1.13 kB	-1 B
src/render/draw_debug.ts	1.13 kB	1.13 kB	-1 B
src/render/draw_background.ts	568 B	567 B	-1 B
src/util/primitives.ts	1.01 kB	1.01 kB	-1 B
src/ui/handler/tap_zoom.ts	369 B	368 B	-1 B
src/ui/handler_manager.ts	2.46 kB	2.46 kB	-1 B
src/ui/control/attribution_control.ts	1.09 kB	1.09 kB	-1 B
src/ui/control/scale_control.ts	742 B	741 B	-1 B
src/style/style_image.ts	126 B	124 B	-2 B
src/source/raster_tile_source.ts	912 B	910 B	-2 B
src/source/tile.ts	1.99 kB	1.99 kB	-2 B
src/util/worker_pool.ts	419 B	417 B	-2 B
src/symbol/placement.ts	4.8 kB	4.8 kB	-2 B
src/shaders/fill.vertex.glsl.g.ts	173 B	171 B	-2 B
src/shaders/hillshade_prepare.vertex.glsl.g.ts	192 B	190 B	-2 B
src/shaders/line_pattern.vertex.glsl.g.ts	789 B	787 B	-2 B
src/shaders/symbol_text_and_icon.vertex.glsl.g.ts	1.01 kB	1.01 kB	-2 B
src/render/program/fill_program.ts	567 B	565 B	-2 B
src/gl/color_mode.ts	175 B	173 B	-2 B
src/geo/edge_insets.ts	432 B	430 B	-2 B
src/ui/handler/tap_drag_zoom.ts	483 B	481 B	-2 B
src/ui/handler/shim/drag_rotate.ts	182 B	180 B	-2 B
src/util/task_queue.ts	266 B	264 B	-2 B
src/render/image_manager.ts	1.4 kB	1.4 kB	-3 B
src/source/raster_dem_tile_source.ts	970 B	967 B	-3 B
src/symbol/path_interpolator.ts	312 B	309 B	-3 B
src/symbol/cross_tile_symbol_index.ts	1.14 kB	1.13 kB	-3 B
src/shaders/_prelude.fragment.glsl.g.ts	146 B	143 B	-3 B
src/shaders/fill_extrusion_pattern.vertex.glsl.g.ts	829 B	826 B	-3 B
src/gl/index_buffer.ts	300 B	297 B	-3 B
src/ui/handler/box_zoom.ts	716 B	713 B	-3 B
src/util/debug.ts	161 B	158 B	-3 B
src/util/smart_wrap.ts	236 B	233 B	-3 B
src/util/dispatcher.ts	336 B	332 B	-4 B
src/source/source.ts	355 B	351 B	-4 B
src/render/draw_raster.ts	1.04 kB	1.04 kB	-4 B
src/render/render_to_texture.ts	868 B	864 B	-4 B
src/ui/control/navigation_control.ts	1.61 kB	1.61 kB	-4 B
src/render/program.ts	1.14 kB	1.14 kB	-5 B
src/render/program/terrain_program.ts	613 B	608 B	-5 B
src/render/program/heatmap_program.ts	560 B	555 B	-5 B
src/render/program/hillshade_program.ts	890 B	885 B	-5 B
src/render/draw_custom.ts	338 B	333 B	-5 B
src/ui/hash.ts	937 B	932 B	-5 B
src/ui/handler/shim/touch_zoom_rotate.ts	290 B	285 B	-5 B
src/style/light.ts	561 B	555 B	-6 B
src/render/glyph_manager.ts	956 B	949 B	-7 B
src/ui/handler/handler_util.ts	110 B	103 B	-7 B
src/render/draw_hillshade.ts	1.14 kB	1.13 kB	-8 B
src/render/program/symbol_program.ts	1.3 kB	1.29 kB	-9 B
src/render/draw_fill.ts	1.02 kB	1.01 kB	-11 B
src/index.ts	880 B	862 B	-18 B

[wipfli]

Thanks @HarelM for working on the blocking issues of the terrain 3d pull request. This goes in the right direction I think. Please add typings where requested.

[wipfli]

Does not seem related to this pull request at first sight. Does maptiler have terrain tiles that we could use woth the examples api key?

[HarelM]

This page no longer works as the fetch is getting an error, so I fixed it a bit, but the terrain source I used is not very good, I didn't manage to use map tiler here unfortunately...

[wipfli]

@acalcutt do you want to generate terrain tiles for a small region in the alps which we could host in the demotiles repo?

[acalcutt]

@wipfli I don't know if you still need these since you switched to the maptiler terrrain, but I made this N047E011 region mbtiles to match the location we used in the debug page

https://drive.google.com/drive/folders/1mur4b0O8_qWUWvO-S4-fb74K99bOESpZ?usp=sharing

A demo of it is here. https://stackblitz.com/edit/web-platform-fvinfs?file=index.html . You can see if you zoom out the terrain is only for that region.

[acalcutt]

I also put a N045E007 demo tile there. which i think is more swiss alps. i tried to get Matterhorn, Switzerland, but that is close to the edge of the tile.

[bdon]

@wipfli terrain tiles or raster images should work fine with the PMTiles adapter, but I haven't tested it, there might be some quirk of the current adapter that I need to fix :p but yeah, if you want to stick them into a Github repository you'll hit the size limit quickly.

[acalcutt]

I tried to convert my terrainrgb mbtiles file to pmtiles here ( https://wifidb.net/demo/pmtiles/jaxa_terrainrgb_N047E011_0-12.pmtiles )

But I tried to use it by combining the maplibre pmtiles demo page with the terrain demo page and it did not seem to work.
https://stackblitz.com/edit/web-platform-d2icls

Did I do anything wrong in that demo page?

[acalcutt]

Seemed like it had converted ok...or at least saw all the tiles

pmtiles-convert jaxa_terrainrgb_N047E011_0-12.mbtiles jaxa_terrainrgb_N047E011_0-12.pmtiles
compression: disabled
Num tiles: 341
Num unique tiles: 341
Num leaves: 0

[acalcutt]

actually, I think it does work. I threw it on my domain where the pmtiles file is located and it looks like it worked there
https://wifidb.net/demo/pmtiles/

[bdon]

@acalcutt The pmtiles-convert and your 2nd demo link seem to work fine.

For your stackblitz demo, it might be failing since you're doing a cross-origin request from stackblitz.com to your server, but your server is not returning CORS headers. The error message I'm seeing in Safari isn't great at indicating this.

[HarelM]

I feel like the "add typings" is a pay back for all the "add typings" I wrote in the past :-)
I've fixed them and answered all the other conversations.

[wipfli]

Haha yes indeed