maplibre / maplibre-gl-js

MapLibre GL JS - Interactive vector tile maps in the browser
https://maplibre.org/maplibre-gl-js/docs/
Other
6.24k stars 680 forks source link

TypeError: Cannot read properties of undefined (reading 'sky') #4340

Closed oterral closed 1 month ago

oterral commented 1 month ago

The error comes from the line https://github.com/maplibre/maplibre-gl-js/blob/main/src/render/painter.ts#L412

The stylesheet object doesn't exist when we call map.redraw() before the style is loaded. I guess the line should be

if (this.style.sky) drawSky(this, this.style.sky);

maplibre-gl-js version: 4.5.0

browser: all

Steps to Trigger Behavior

  1. Create a map
  2. Call map.redraw()

Link to Demonstration

https://codepen.io/oterral/pen/PovvbJx

Expected Behavior

No error about sky property

Actual Behavior

A TypeError is triggered in the console

HarelM commented 1 month ago

Why calling redraw before the style is loaded?

oterral commented 1 month ago

We do this in https://github.com/geoblocks/ol-maplibre-layer . More precisely we call redraw on each render frame.

oterral commented 1 month ago

What is weird, is this.style exists so why testing this.style.stylesheet.sky and don't reuse it in renderSky ?

HarelM commented 1 month ago

I wanted sky to alway be initialized to avoid these if statements. stylesheet.sky is the sky specification and not the sky object...

HarelM commented 1 month ago

See here: https://github.com/maplibre/maplibre-gl-js/pull/3645#discussion_r1650162557

oterral commented 1 month ago

ah ok so may be just a beffer "if" condition will get rid of the error without changing anything to the logic:

if (this.style.stylesheet?.sky) drawSky(this, this.style.sky);
HarelM commented 1 month ago

That might work 😃 I have no real objections. Would be interesting to understand how other parts are working so that they don't fail like that...

am2222 commented 1 month ago

I faced similar error using maplibre with react-map-gl with https://api.maptiler.com/maps/streets/style.json mapStyle. I think the older styles might be missing sky properties?

HarelM commented 1 month ago

We have tested old styles that obviously don't have sky properties, and they work. It seems to be related to how this library is being used, which we didn't take into consideration like the usage of redraw before map load event, and possibly other scenarios.

am2222 commented 1 month ago

@HarelM thanks for the response! So I noticed downgrading its version to 4.5.1 solves the issue.

HarelM commented 1 month ago

This feature was introduced as part of 4.5.0 version, downgrading to previous versions will probably solve this.

UberMouse commented 1 month ago

We hit the same issue in our application. Where we manage custom 3d layers synchronisation, after creating/deleting the custom layers we call map.redraw() to paint it (though tbh I don't think we actually need to do that) and that causes the issue. We run a fork of maplibre to address #3983 so I applied the null safety operator to the if check outlined above and the issue did stop happening.

birdofpreyru commented 1 month ago

I've also bumped into this issue just now. I had no time to carefully investigate it yet, but it feels like it happens reliably when the map component is removed from DOM. I am also using this library indirectly, through https://github.com/visgl/react-map-gl — it as well might be a bug there.

birdofpreyru commented 1 month ago

@HarelM

  1. I checked that adding ? into if (this.style.stylesheet?.sky) drawSky(this, this.style.sky);, suggested by @oterral fixes the problem for me.
  2. I think, what happens is the following (I am not familiar with Maplibre codebase, I just had a quick look at it) — when the map component is unmounted all layers are removed first, while the Map object may still exist. At that point the style object is replaced by an empty Style, which has no stylesheet field attached (it seems to be attached only after some data layers are loaded); at that point the map is already marked as dirty, and might be re-rendered, which fails at that point because it assumes that stylesheet is always attached when the map is rendered.
birdofpreyru commented 1 month ago

Can we get this patched ASAP? Do you want me to create a PR?

HarelM commented 1 month ago

Please open a PR, yes.

birdofpreyru commented 1 month ago

Here you are: #4431