Skip to content
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

Allow pitch up to 85 #574

Merged
merged 5 commits into from
Oct 27, 2021
Merged

Allow pitch up to 85 #574

merged 5 commits into from
Oct 27, 2021

Conversation

kibala145
Copy link
Contributor

Changed allowed maxPitch value from 60 to 85. Closes #47

@kibala145
Copy link
Contributor Author

Corrected version of #56.
The PR just changes maxPitch allowed value. No perfomance optimization for large pitch values added, by the way, should optimization be here or a separate PR?

Please tell me if I missed something.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 26, 2021

Bundle size report:

Size Change: 0 B
Total Size Before: 194 kB
Total Size After: 194 kB

Output file Before After Change
maplibre-gl.js 185 kB 185 kB 0 B
maplibre-gl.css 9.49 kB 9.49 kB 0 B
ℹ️ View Details
Source file Before After Change
rollup/build/tsc/src/ui/map.js 6.07 kB 6.07 kB +2 B

src/ui/map.ts Outdated Show resolved Hide resolved
@HarelM
Copy link
Collaborator

HarelM commented Oct 26, 2021

Thanks for this PR!
Please make sure to keep the template next time as you must acknowledge that this is not a backport of mapbox code.
Also please add a changelog item as part of this PR.

@HarelM
Copy link
Collaborator

HarelM commented Oct 26, 2021

P.S. Optimization can be done on another PR as the default wasn't changed as part of this PR and it's the user responsibility to make sure everything works as expected...

@HarelM HarelM mentioned this pull request Oct 26, 2021
@HarelM
Copy link
Collaborator

HarelM commented Oct 26, 2021

I'm not sure I have a good solution for this, but it's worth noting somehow the the max pitch change is "at your own risk"...?

@rbrundritt
Copy link
Contributor

I played around with overriding this in the past. Something worth calling out is that vector tiles that are in the distance may end up loading from an earlier zoom level. This particularly is noticeable when you setup zoom level ranges between two different layers. I wouldn't say this is a blocker, but something to be aware of. I also recall issues with the querySourceFeatures function not working correctly when pitched beyond 60 degrees in some cases, but can't seem to find the bug for this.

@kibala145
Copy link
Contributor Author

I'm not sure I have a good solution for this, but it's worth noting somehow the the max pitch change is "at your own risk"...?

Sounds good. Could you please tell me where to add this?

@HarelM
Copy link
Collaborator

HarelM commented Oct 26, 2021

I'm honestly not sure, the best idea I have is to write it as part of the change log item you added...

add "at your own risk" note
@HarelM
Copy link
Collaborator

HarelM commented Oct 26, 2021

@rbrundritt I know this is probably not a perfect solution to this issue and more work is needed but I think that adding this will allow users to start explore this, find issues and hopefully try and fix them.
The full solution is probably way more complex and if you are not changing the default you wouldn't see these issues.
So I think this is a good addition, MVP style.

@HarelM
Copy link
Collaborator

HarelM commented Oct 26, 2021

@astridx @wipfli @xabbu42 let me know what you guys think about this.
I vote for merging this.

@xabbu42
Copy link
Contributor

xabbu42 commented Oct 26, 2021

I think we should also add a warning to the documentation, as most users will not read old changelogs. Probably in https://maplibre.org/maplibre-gl-js-docs/api/map/#map#setmaxpitch.

@xabbu42
Copy link
Contributor

xabbu42 commented Oct 26, 2021

And the documentation for option.maxPitch in the source itself should also include a short warning.

@rbrundritt
Copy link
Contributor

How about a warning like this. "Max pitch values greater than 60 degrees are experimental and may result in rendering issues. If you encounter any, please raise an issue with details in the MapLibre project.".

add warning message about pitch greater than 60
@kibala145 kibala145 requested a review from HarelM October 27, 2021 10:36
@astridx
Copy link
Contributor

astridx commented Oct 27, 2021

I would also include this PR. Technically, I have no experience in working with the parameter. But since the default value remains and there is a warning, it should be fine.

@HarelM HarelM merged commit f578187 into maplibre:main Oct 27, 2021
@ghost
Copy link

ghost commented Oct 28, 2021

There's a couple of issues with high pitch values that I had found in the past; here's what I remember:

  • Can see very far into distance, so a lot of tiles are being loaded; this can pretty much cause your tile-cache to be flushed over and over and the tile-server will likely get DDoS'd.
  • Camera placement in mapbox is relative to the point where the camera is looking at; because the point is closer to horizon (= distance closer to infinity), it gets numerically unstable, so your camera can get jerky.
  • Map movement with swiping / dragging also encounters this numeric issue, but also logic errors, as even a tiny drag on the screen can result in a huge movement of the camera, as a couple of pixels towards the horizon can be huge distances; the existing navigation controls were not designed for this scenario.

@cigone-openindoor
Copy link

Screenshot 2021-11-21 at 14 46 21

So now, how could we define sky color(s) ?

@rbrundritt
Copy link
Contributor

Screenshot 2021-11-21 at 14 46 21

So now, how could we define sky color(s) ?

What I did in one demo was add a gradient background color to the map div. For example using this CSS:

#myMap {
    background: linear-gradient(0deg, rgba(36,0,0,1) 56%, rgb(249 209 89) 62%, rgb(81 151 173) 80%, rgb(2 92 179) 95%)
}

and it turn out like this:

MapSkyCSSGradient

@cigone-openindoor
Copy link

Wonderful !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

max pitch
6 participants