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

feat: send geolocation as adapter #10232

Closed
wants to merge 59 commits into from

Conversation

behnammodi
Copy link
Contributor

@behnammodi behnammodi commented Dec 21, 2020

  • This PR can help developers to customize geolocation behavior.
  • We don't have any visual changes.
  • All test cases compatible with these changes.
  • I need your opinion and then add a document.

So now we have:

const myGeolocation: Geolocation = {
   clearWatch: ...,
   getCurrentPosition: ...,
   watchPosition: ...,
};

const geolocateControl = new GeolocateControl({
   geolocation: myGeolocation,
});

map.addControl(geolocateControl);

I'll say welcome to your awesome feedback :)

@CLAassistant
Copy link

CLAassistant commented Dec 21, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
6 out of 14 committers have signed the CLA.

✅ behnammodi
✅ mourner
✅ andycalder
✅ asheemmamoowala
✅ anderswi
✅ kawsndriy
❌ arindam1993
❌ ansis
❌ mpulkki-mapbox
❌ ryanhamley
❌ ChrisLoer
❌ thefifthisa
❌ karimnaaji
❌ astojilj
You have signed the CLA already but the status is still pending? Let us recheck it.

@andrewharvey
Copy link
Collaborator

What's the motivation or use case for this? Isn't window.navigator.geolocation really the only geolocation provider? How else would it be provided if not window.navigator.geolocation?

@behnammodi
Copy link
Contributor Author

behnammodi commented Dec 21, 2020

@andrewharvey You're right but this PR related to #10234 (comment), by this PR we can override the geolocation function.

for example, we want to show a popup UI before getting the user's location or ignore some coordinates

@behnammodi
Copy link
Contributor Author

@andrewharvey Also imagine we going to enhance user location by their behavior and we don't want to relying on native geolocation.

@ImanMh
Copy link

ImanMh commented Dec 23, 2020

What's the motivation or use case for this? Isn't window.navigator.geolocation really the only geolocation provider? How else would it be provided if not window.navigator.geolocation?

Same issue here, I want to build a PWA wrapped in a native app. In this case I prefer native app to provide GPS coordinates but with this implementation mapbox goes directly to the browser APIs.

@behnammodi
Copy link
Contributor Author

@ImanMh Thanks, It's an awesome use case

* add changelog and bump package version

* Update style-spec changelog and bump package version

* bump to dev versions

Co-authored-by: Arindam Bose <[email protected]>
@behnammodi
Copy link
Contributor Author

@andrewharvey what is your opinion?

)

* Add an option to disable terrain vertex morphing
* Disable vertex morphing in render tests
* Fix free camera render test
@ansis
Copy link
Contributor

ansis commented Jan 11, 2021

@mourner this looks good to me. Thoughts?

mourner and others added 12 commits January 11, 2021 18:24
* return a promise in "once" if no listener provided

* fix ambiguity in map.once docs
…ties (mapbox#10272)

* update style spec

* multiple min/max

* edit validation, add test cases

* fix linting error

* add min/max to array element spec

* fix error messages

* Remove render test

* Replace render test line-dasharray/unusual-cases/negative-values by unit test

* Use new test fixturest

Co-authored-by: Karim Naaji <[email protected]>
* fix blurry map-pitched labels on terrain

* update render tests
* Fix JSDoc lint warnings

* Fix new JSDoc warnings
…0308)

* Revert "Hardcode fallback token in release test page (mapbox#10306)"

This reverts commit f352188.

* Revert 6a2531e
…apbox#10258)

* Fix issue mapbox#10186: not preserving drawn order while using terrain render cache

- Revise terrain render loop to be more explicit and easier to follow from the
  painter render passes:
  - Remove any painter modifications from render() function
  - Extract terrain draw depth to reduce branching and add explicit call from
    painter.render()
  - Make it explicit that all rendering of terrain happens in the translucent
    render pass

- Update terrain.render() to work as follow:
  - In render cache mode:
    . Draw all layers of sequential draped and add to render cache
    . Switch to interleaved render mode (not cached) for other layers

- Add debug/10186 sample code for simple reproduction case

* Update 3d playground with more styles to test from

* Add helper function to calculate render cache efficiency

* Update render test baseline with correct terrain drawn order

* Update 3d playground example
- Add mapbox/satellite-streets-v11 style
- Add back mistakenly removed fill-extrusion layer

* Add render tests

* layerEnd -> lastDrawnLayerIndex

* Self review: Comment + renaming

* Check render cache efficiency on style order changes

* Add more information on render cache efficiency warning

* Fix wrong variable naming and unit test to not warn

* Add unit tests

* Calculate list of sequential draped render batches

* Consume draped layer batches

* Create getter for style._order to allow split with drape first order

* Move isLayerDraped to style

* Minor tweak

* Load list of draped first layers when terrain is enabled

* Assign FBOs to renderable terrain batches

* Don't render empty batches

* Fix bug not clearing raster fade if any source is not RasterTileSource

Minor syntax tweak

* Use draped render batches in terrain.render()

* Simplify render loop

* Move terrain depth render earlier to reduce framebuffer switches

* Update terrain-debug to add custom layer for debugging

* Add map.options.optimizeForTerrain and update unit tests

* Update render tests to account for map.options.optimizeForTerrain

* Fix Flow + Lint + render tests

* Remove extraneous flag

* Add documentation and update comments

* Add warning and guidance on low render cache efficiency

* Revert using cached mode always
This should be addressed in separate PR, needs to take into
account video source type and feature state changes in order
to invalidate the render cache.

* Address review comments
Thanks @arindam1993
Ryan Hamley and others added 7 commits February 4, 2021 16:52
…ed (mapbox#10347)

* Do not depend on underlaying height property to assess flat roofs

* Add regression render test

* Remove extraneous tiles

* Add test sample page for fill-extrusion querying on terrain

* Add fill extrusion querying as part of release testing

* Fix getLoadedBucket when not overzoomed when using client data sources

* Remove dependency on 'type' feature property

Co-authored-by: Vladimir Agafonkin <[email protected]>

* Address review comments

* Address review feedback: Trim down example size

* Address @arindam1993's comment

Co-authored-by: Vladimir Agafonkin <[email protected]>
@asheemmamoowala
Copy link
Contributor

so we need a rebase by me or somebody else?

@behnammodi it's best if you can rebase and verify the changes locally before merging the changes.

@behnammodi
Copy link
Contributor Author

@asheemmamoowala All right, I will do it

astojilj and others added 6 commits February 11, 2021 11:16
* use native ES modules for unit tests

* update CircleCI Node image to v14.15

* bump yarn cache key on Circle CI

* expose bundles as CommonJS in Node

* upgrade postcss and use CJS for config

* convert harness and some scripts to esm

* fix lint and flow

* make sure testem runs with native esm

* testem fixups

* use custom json loader to support named exports

* use a valid access token in unit tests

* work around testem refusing to load cjs configs

* fix lint

* fix testem runs on CI hopefully

* fix webpack build
@behnammodi
Copy link
Contributor Author

@asheemmamoowala I did it, but I need someone to help me for 4 failed stage

@karimnaaji
Copy link
Contributor

karimnaaji commented Feb 18, 2021

@behnammodi , I rebased your branch off of main in https://github.com/mapbox/mapbox-gl-js/tree/karim/behnammodi-pr10232-rebased.

The CI job did not pass on unit tests and build (render tests failure is unrelated), https://app.circleci.com/pipelines/github/mapbox/mapbox-gl-js/5920/workflows/4ebe7ae1-c43f-425e-bbcc-2e62cf6c29d0.

Could you try the following:

git remote add upstream [email protected]:mapbox/mapbox-gl-js
git fetch upstream
git checkout upstream/karim/behnammodi-pr10232-rebased
git checkout -b geolocation-as-adapter-rebased
git push origin main

And then open up a new PR so the discussion can continue on a clean diff? We'll close this PR then.

@behnammodi
Copy link
Contributor Author

@karimnaaji Thank you for your help, I did it #10400

@karimnaaji
Copy link
Contributor

@behnammodi Sounds good, thanks the diff look more approachable now! Closing this PR and I'll tag whoever was involved in reviewing this one there.

@karimnaaji karimnaaji closed this Feb 19, 2021
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.