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

Three location based example fix #409

Merged
merged 3 commits into from
Mar 18, 2022
Merged

Conversation

nickw1
Copy link
Collaborator

@nickw1 nickw1 commented Mar 18, 2022

⚠️ All PRs have to be done versus 'dev' branch, so be aware of that, or we'll close your issue ⚠️

What kind of change does this PR introduce?

Bugfix for three.js location-based example.

Can it be referenced to an Issue? If so what is the issue # ?
#376

How can we test it?

Available online at:
https://hikar.org/nex/three.js/examples/location-based/

Summary

three.js location-based example had some bugs, e.g. in some circumstances it tried to start real and fake GPS simultaneously.

I have also made the example easier to test so that the four test boxes are added adjacent to the initial GPS position, or the fake position if fake GPS is used. I have added in commented-out code to initialise fake GPS, which can be turned on simply by uncommenting.

Does this PR introduce a breaking change?

No

Please TEST your PR before proposing it. Specify here what device you have used for tests, version of OS and version of Browser

Chrome, Firefox Linux desktop
Chrome on Android 12 / Pixel 3

Other information

@kalwalt would it be ok to merge this into dev, and also into master, as soon as possible? The current example does not seem to be working correctly, this one is easier to understand and definitely works (for me) on desktop and mobile, and using both fake GPS and a GPS simulator (Location Guard) on desktop.

This is separate to my recent PR #406.

@kalwalt
Copy link
Member

kalwalt commented Mar 18, 2022

Hi @nickw1 i will test the code when i have a bit of time, but i took a quick look and the code seems to me to be fine, much better than before.
I have also this PR #407 that fix a bug in nft aframe examples, remove the double libs(nft and not) and add a simple action github script to check if the code is well formatted. I think we need to improve this last but i think it's a nice and useful tool.

@kalwalt
Copy link
Member

kalwalt commented Mar 18, 2022

I did a quick test on my Android device and on desktop too ( Android Oppo A72 and ubuntu 20.04). I can confirm that on Mobile with GPS enabled i can see the four Boxes, without Gps enabled and uncommenting line 31 in index.js i can see the four boxes again. On desktop i can see the four boxes moving the scene with the mouse. Well done @nickw1 !! 🙌 👍
We can merge in dev if you have nothing else to do, and after we can merge #407 and soon merging in master to make another release 3.4.0-alpha-rc2

@nickw1
Copy link
Collaborator Author

nickw1 commented Mar 18, 2022

@kalwalt OK great! Will merge into dev to get the example updated.

@nickw1 nickw1 merged commit a26ff06 into dev Mar 18, 2022
@nickw1
Copy link
Collaborator Author

nickw1 commented Mar 18, 2022

Agree it makes sense to remove the double build libs (i.e #407) btw.

@kalwalt
Copy link
Member

kalwalt commented Mar 18, 2022

Agree it makes sense to remove the double build libs (i.e #407) btw.

Yes, unfortunately only for three.js. there was a bug with aframe and i solved leaving the double builds. I will add more details about this issue in the PR.

@nickw1
Copy link
Collaborator Author

nickw1 commented Mar 19, 2022

I'm going to do a new PR request btw, for the updates to the three.js location based that I made as part of the new A-Frame components. I think this would make sense to include in the upcoming rc2 release if possible, as it's not a breaking change but does include some useful new features such as a GPS error handling event and a more consistent API for fake and real GPS updates (i.e. fake GPS updates generate the gpsupdate event as well as real ones).

@nickw1 nickw1 deleted the three-location-based-example-fix branch March 19, 2022 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants