-
-
Notifications
You must be signed in to change notification settings - Fork 705
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
set defaultMaxPitch to 85 #56
Conversation
Bundle size report: Size Change: +2 B
ℹ️ View Details
|
I don't know why macos test failed but I guess it has no relation with my changes. |
CI is failing. Please fix CI for a proper review. Thanks! |
Thank you, i've seen the red cross also. |
The mac and windows CI jobs seem to be a bit flaky. I just re-ran and it's all green now. |
Well, improvement should be to accept maxPitch until 85, but limit it to 60 by default. |
I think default should remain on 60. |
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. |
I’ve double checked: |
You should change this piece of code instead of the Lines 379 to 381 in 8804495
In this case the default if (options.maxPitch != null && options.maxPitch > maxPitchThreshold) {
throw new Error(`maxPitch must be less than or equal to ${maxPitchThreshold}`);
} |
You're right. |
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 |
I suggested my changes long time ago. |
@cigone-openindoor first and foremost thanks for taking the time to do it and open this discussion! 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 : |
But in the test you fixed, the map creation is done without defining maxPitch but when asking the map for maxpirch it returns 85...? |
"defaultMaxPitch" is the maximum maxpitch the user can define, a kind of "maxmaxpitch". I just set it from 60 to 85. |
mapDefaultMaxPitch (60) |
@cigone-openindoor let me know if you plan to fix the code review comment and follow the required behavior. |
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
Working fine
I think it might be something to do with the adjustment in the radius in this loop
|
Closed in favor of #574 |
Just changed the default maxPitch.
Fix #47