set defaultMaxPitch to 85

[cigone-openindoor]

Just changed the default maxPitch.

Fix #47

[github-actions]

Bundle size report:

Size Change: +2 B
Total Size Before: 201 kB
Total Size After: 201 kB

Output file	Before	After	Change
mapbox-gl.js	196 kB	196 kB	+2 B
mapbox-gl.css	4.62 kB	4.62 kB	0 B

ℹ️ View Details

Source file	Before	After	Change
src/ui/map.js	6.51 kB	6.51 kB	+3 B

[cigone-openindoor]

I don't know why macos test failed but I guess it has no relation with my changes.

[curran]

CI is failing. Please fix CI for a proper review. Thanks!

[clement-igonet]

Thank you, i've seen the red cross also.
It's a mac specific issue.
I've no way to check further.
Any advice to solve this blocker ?

[msbarry]

The mac and windows CI jobs seem to be a bit flaky. I just re-ran and it's all green now.

[clement-igonet]

Well, improvement should be to accept maxPitch until 85, but limit it to 60 by default.
I'll check it...

[klokan]

I think default should remain on 60.

[JannikGM]

This should remain a very niche feature until properly implemented, unlike in mapbox 2.0 where it's a first-class citizen (and where users are encouraged to use larger pitch ranges).

Therefore, we probably want to warn (i.e. document somewhere) that higher-than 60 degree pitch can lead to excessive amount of tiles and thereby tile cache trashing.
We should also document that it can also break navigation (try navigating using mouse-drag towards horizon).
We should document that this feature should not be used for standard maps (it's only really "functional" for specific situations).

[clement-igonet]

I’ve double checked:
maxPitch = 60 by default,
but can rise 85 if explicitly defined like that.

[Joxit]

👍 @clement-igonet

You should change this piece of code instead of the defaultMaxPitch.

maplibre-gl-js/src/ui/map.js

Lines 379 to 381 in 8804495

	if (options.maxPitch != null && options.maxPitch > defaultMaxPitch) {
	throw new Error(`maxPitch must be less than or equal to ${defaultMaxPitch}`);
	}

In this case the default maxPitch value will remain 60, and you add a new variable like maxPitchThreshold = 85 and in this piece of code will look like

 if (options.maxPitch != null && options.maxPitch > maxPitchThreshold) { 
     throw new Error(`maxPitch must be less than or equal to ${maxPitchThreshold}`); 
 } 

[clement-igonet]

You're right.
However, it's just a matter of variable name, coming from mapbox original code.
I do not aim to go further than fixing the original issue.
You're welcome to apply your improvements in your own M.R.

[rbrundritt]

To better handle larger pitch values, Mapbox also modified how tiles are loaded in the top half of the screen (load zoomed out tiles), for better performance. Here is the PR for that: mapbox/mapbox-gl-js#8975

[cigone-openindoor]

I suggested my changes long time ago.
If nobody else approve (3/3), merge could block if someone else change same piece of code.
So what ?
Please, approve or kill my M.R.

[HarelM]

@cigone-openindoor first and foremost thanks for taking the time to do it and open this discussion!
However, I'm a little bit confused by this thread.
From the initial comment and the code my understanding is that the default for anyone not specifying maxPitch will (after this is merged) be 85.
If this is the case, I agree with @klokan and @Joxit that this is not the required behavior.
In my opinion, I think that if someone wants to explicitly change the maxPitch to be, say 85, it should be allowed using the map options parameter - i.e. this means that someone that is writing the code understands that this can cause performance issues, and also this can be documented as part of the definition of maxPitch in the map options parameters documentation.
I think the default should be kept as it is today at 60.

Please let me know if I misunderstood the changes and definition of this PR.

[cigone-openindoor]

@cigone-openindoor first and foremost thanks for taking the time to do it and open this discussion!
However, I'm a little bit confused by this thread.
From the initial comment and the code my understanding is that the default for anyone not specifying maxPitch will (after this is merged) be 85.
If this is the case, I agree with @klokan and @Joxit that this is not the required behavior.
In my opinion, I think that if someone wants to explicitly change the maxPitch to be, say 85, it should be allowed using the map options parameter - i.e. this means that someone that is writing the code understands that this can cause performance issues, and also this can be documented as part of the definition of maxPitch in the map options parameters documentation.
I think the default should be kept as it is today at 60.

Please let me know if I misunderstood the changes and definition of this PR.

There is a misunderstanding I'm victim of, from the very beginning :
The default max value remains 60, not 85.
To define 85 as a max, it has to be explicitly define by the user.
I know that variable names are confusing. It comes from the original mapbox code, not mine.

[HarelM]

But in the test you fixed, the map creation is done without defining maxPitch but when asking the map for maxpirch it returns 85...?
I'm probably missing out something here...
I would advise to change a bit more in terms of variavles and function names to avoid this confusion...
If the mapbox code is confusing, let's change it... :-)

[cigone-openindoor]

"defaultMaxPitch" is the maximum maxpitch the user can define, a kind of "maxmaxpitch". I just set it from 60 to 85.
maxpitch is still initialized at 60.

[HarelM]

mapDefaultMaxPitch (60)
userDedinedMaxPitch (85)
I think are good names for these variables...

[Joxit]

Hi there,

So, I tried this PR, and as I said earlier, you must add a new variable maxPitchThreshold, this is not a naming issue but a change in behavior.
After this PR, the default max value will be 85, and not 60.

[HarelM]

@cigone-openindoor let me know if you plan to fix the code review comment and follow the required behavior.

[rbrundritt]

I've been experimenting with this and think I found a key bug. When the pitch is greater than 70 degrees, queryRenderedFeatures stops work with some layers appear to stop working. In my test app I have the following:

Not working

• Circle layer (Vector tiles source)

Working fine

• Polygon extrusion layer (Vector tiles source)
• Polygon layer (Vector tiles source)
• Symbol layer (GeoJSON source)

I think it might be something to do with the adjustment in the radius in this loop

maplibre-gl-js/src/style/style_layer/circle_style_layer.ts

Line 65 in e78ad79

for (const ring of geometry) {

[HarelM]

Closed in favor of #574