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

New A-Frame location based #406

Merged
merged 8 commits into from
Apr 17, 2022
Merged

New A-Frame location based #406

merged 8 commits into from
Apr 17, 2022

Conversation

nickw1
Copy link
Collaborator

@nickw1 nickw1 commented Mar 14, 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?

New. hopefully improved A-Frame components for location-based, plus enhancement of the three.js location-based code.

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

How can we test it?

I have added two test applications in the examples directory, one for A-Frame and one for three.js; these are in the new-location-based directory in each case.

Summary

The existing location-based components are quite complex code-wise and (IMO) rather difficult to debug. There have been some long standing problems on some devices with location-based such as #275 which have been hard to fix. By simplifying the A-Frame location based components it is hoped that these problems can be resolved more readily.

So two new A-Frame components, gps-new-camera and gps-new-entity-place, have been created, in the new-location-based directory. To avoid duplication of code, these actually wrap the three.js location-based code, which takes care of such things as projecting lat/lon into Spherical Mercator and converting to world coordinates. So to use these components, the three.js location-based code must be included in your project.

Many of the existing properties of gps-camera have been inherited, including simulateLatitude, simulateLongitude, simulateAltitude, gpsMinDistance and positionMinAccuracy. However, minDistance and maxDistance have been removed as this functionality can be implemented using the near and far properties of the A-Frame camera.

simulateLatitude and simulateLongitude offer improved behaviour compared to the originals, as they did not appear to trigger a gps-camera-update-position event previously. Now they do, which makes it easier to implement code which works with real and simulated GPS.

A new component arjs-device-orientation-controls has also been added. This is a lightweight wrapper around the DeviceOrientationControls class within three.js location-based (which was in turn taken from three.js itself and modified). However note that this does not detect mouse movements so rotation will only work on a mobile device. The reason for this is to separate out orientation sensor movements and mouse movements; in tests I found that code in the A-Frame look-controls component was interfering with the device orientation code, and figured that (for now) it was best to separate them out.

Does this PR introduce a breaking change?

No as the new components have different names for now, gps-new-camera and gps-new-entity-place. However my proposal is, that once they have been tested on a wide range of devices, to replace gps-projected-camera and gps-projected-entity-place with these new components.

So if this happens, a few things will not work:

  • minDistance and maxDistance deprecated; use near and far as detailed above.
  • in keeping with standard A-Frame practice, the gps-camera-update-position event is now emitted by the camera entity that the gps-new-camera is attached to, rather than the window. So any code handling that event will need to be updated to take that into account.

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

Linux desktop: Firefox and Chrome latest.
Mobile: Pixel 3, Android 12, running latest Chrome. Confirmed to display objects in the correct position in-the-field, and objects move as expected when you move round the real world.

Other information

n/a

@kalwalt
Copy link
Member

kalwalt commented Mar 14, 2022

Awesome @nickw1 i want to test this as i have a bit of time!

@nickw1 nickw1 changed the title New location based New A-Frame location based Mar 15, 2022
@nickw1 nickw1 added the enhancement New feature or request label Mar 18, 2022
@kalwalt
Copy link
Member

kalwalt commented Mar 24, 2022

@nickw1 I did a simple test on Desktop pc and i got:

GET http://127.0.0.1:5500/aframe/examples/new-location-based/AR.js/three.js/build/ar-threex-location-only.js net::ERR_ABORTED 404 (Not Found)
127.0.0.1/:8          GET http://127.0.0.1:5500/aframe/examples/new-location-based/AR.js/aframe/build/aframe-ar.js net::ERR_ABORTED 404 (Not Found)

Tested under Ubuntu 20.04 with Chromium 99.0.4844.82.

@nickw1
Copy link
Collaborator Author

nickw1 commented Mar 25, 2022

@kalwalt OK will look at this later (or if not, over the weekend). Just a simple path problem I think.

@nickw1
Copy link
Collaborator Author

nickw1 commented Mar 25, 2022

@kalwalt OK the example assumes that you have the AR.js aframe directory as a subdirectory of your current directory. I will fix this.

@CarloCattano
Copy link

the new-location based example yields blank screen on Android mobile (Chrome latest) , seems to open the camera on PC but when testing on android I get a blank screen (white) , GPS is logged properly on start in the alert.
See my test at https://github.com/CarloCattano/webXRtest/tree/main/newtest

Just removed the location simulation , and only one warning is present on PC

Error with Permissions-Policy header: Unrecognized feature: 'interest-cohort'.

Must find a good way to debug mobile console somehow
I manually copied the build/*.js files from the branch

@nickw1
Copy link
Collaborator Author

nickw1 commented Mar 26, 2022

@kalwalt OK have fixed one or two bugs. Try again when you have a moment.

@kalwalt
Copy link
Member

kalwalt commented Apr 6, 2022

@nickw1 i tested again with my Desktop linux machine and i can confirm that the issue was solved, the example can load the two source files. I will test the example also with my Android device when i have time.

@nickw1
Copy link
Collaborator Author

nickw1 commented Apr 7, 2022

@kalwalt ok great! Thanks.

@kalwalt
Copy link
Member

kalwalt commented Apr 10, 2022

@nickw1 It's ok on Mobile, the code can correctly detect my GPS position; The only things that not worked for me was to manually changing the Lat Lon coordinates, i supposed that this part is not completed or maybe you get another result with a different device?

@nickw1
Copy link
Collaborator Author

nickw1 commented Apr 11, 2022

@kalwalt was the manual coordinate update on a mobile device with GPS enabled? What happens in that case is that the real GPS will override the 'fake' GPS. Does it work if you turn GPS off? It's really intended for testing on a desktop device.

However let me double check that again tomorrow.

@kalwalt
Copy link
Member

kalwalt commented Apr 11, 2022

@kalwalt was the manual coordinate update on a mobile device with GPS enabled? What happens in that case is that the real GPS will override the 'fake' GPS. Does it work if you turn GPS off? It's really intended for testing on a desktop device.

However let me double check that again tomorrow.

Hi @nickw1 thank you for the answer! i tested with GPS enabled and so maybe that was the issue. Infact it does make sense, because the manual coord update has sense if GPS is not enabled but maybe better to add some alert? I will try to turn GPS off on both mobile and desktop.

@nickw1
Copy link
Collaborator Author

nickw1 commented Apr 12, 2022

Hi @kalwalt have just tested again on my mobile device, without a GPS signal, and the manual input works fine. I have added an alert box to advise people to turn GPS off if they want to test this on a mobile device.

@kalwalt
Copy link
Member

kalwalt commented Apr 12, 2022

Perfect! Will test again!

@kalwalt
Copy link
Member

kalwalt commented Apr 16, 2022

@nickw1 i tested again and i can confirm that it work as you described. Do you need to make other improves to the code?

@nickw1
Copy link
Collaborator Author

nickw1 commented Apr 17, 2022

@kalwalt No, I think I am happy with it now.

@nickw1
Copy link
Collaborator Author

nickw1 commented Apr 17, 2022

Thanks for testing!

@amirhossein-razlighi
Copy link

Please merge this PR as soon as possible . we are all looking forward to it :)

@nickw1
Copy link
Collaborator Author

nickw1 commented Apr 17, 2022

@kalwalt ok shall I merge?

@kalwalt
Copy link
Member

kalwalt commented Apr 17, 2022

@kalwalt ok shall I merge?

Yes, It is fine for me.

@kalwalt
Copy link
Member

kalwalt commented Apr 17, 2022

We can merge this PR and after we can also merge #415. I have also other fixes in another branch for multi-markers (learner and player) that we should merge, and after that i think we can make a new release.

@nickw1
Copy link
Collaborator Author

nickw1 commented Apr 17, 2022

@kalwalt ok great! Will merge.

@nickw1 nickw1 merged commit 867be54 into dev Apr 17, 2022
@nickw1 nickw1 deleted the new-aframe-location-based branch April 24, 2022 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants