3dstreet icon indicating copy to clipboard operation
3dstreet copied to clipboard

Street geo

Open Algorush opened this issue 9 months ago β€’ 5 comments

Algorush avatar May 15 '24 19:05 Algorush

Deploy Preview for 3dstreet-core-builds ready!

Name Link
Latest commit 6501ff395f0bc06d11e6ca04f6b734a23e57a5b0
Latest deploy log https://app.netlify.com/sites/3dstreet-core-builds/deploys/664cc851b1ce8500086317b4
Deploy Preview https://deploy-preview-516--3dstreet-core-builds.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar May 15 '24 19:05 netlify[bot]

I have changed this example for testing: /examples/google-tiles/ Now you can change the component parameters like this in console:

r = document.getElementById('reference-layers')
r.setAttribute('street-geo', 'maps', 'mapbox2d')
r.setAttribute('street-geo', 'maps', 'google3d')
r.setAttribute('street-geo', 'maps', 'google3d, mapbox2d')
r.setAttribute('street-geo', 'maps', '')

The lat/long change still needs to be tested

Algorush avatar May 16 '24 03:05 Algorush

I renamed the map creation functions. Now the function has a name: MapType + "Create". Similar to the update functions: MapType + "Update". I don't know, maybe it's not good practice to do like this?

Algorush avatar May 16 '24 16:05 Algorush

Elevation is the requested elevation of the scene's origin 0 0 0. In other words, if a user has lat 30, long 30, elevation 30, then I assume the elevation is 30 meters above sea level.

But google 3d tiles takes height which is a different value. Here is a sample of how I converted elevation into height in a glitch prototype project:

          // Default is SF Fort Mason Northwest Pier Streetlight
          const demoLat = queryParams.get("lat") ?? 37.807193;
          const demoLong = queryParams.get("long") ?? -122.431964;
          const elevationHeightConstant = 32.49158 // a "height" of -26 = 6.49m elevation therefore 32.49158
          const demoHeight = queryParams.get("elevation") - elevationHeightConstant ?? -26
          console.log(demoHeight);
          console.log(demoLat, demoLong);
          this.el.setAttribute('loader-3dtiles', 'long', demoLong)
          this.el.setAttribute('loader-3dtiles', 'lat', demoLat)
          this.el.setAttribute('loader-3dtiles', 'height', demoHeight)

kfarr avatar May 16 '24 20:05 kfarr

Next:

  • documentation for the component
  • test - does it create a child with the proper component properties? does it update the child when the wrapper properties are updated? (coordinates or map type)

kfarr avatar May 17 '24 14:05 kfarr

added documentation and test based on chai lib. Test could be run from /test/browserTests/street-geo-test.html Chai library supports execution through the browser, so it is more convenient to use for testing when working with DOM, AFRAME and Three.js elements.

Algorush avatar May 18 '24 02:05 Algorush

I also added a .node.js ending to all existing tests, so that only tests with a .node.js ending would be run in test command through package.json. For now, browser tests need to be run manually. Perhaps they can also be configured to launch via gitHub CI. ... yes, chatGPT suggests that browser tests can be run in Github Actions via puppeter or playwright

Algorush avatar May 18 '24 03:05 Algorush

For tests, the main convention I saw in several projects is to mirror the directory structure and end the test filename with .spec.js or .test.js. The aframe repository for example uses .test.js convention. So like this:

3dstreet/
β”œβ”€β”€ src/
β”‚   β”œβ”€β”€ components/
β”‚   β”‚   └── street-geo.js
β”œβ”€β”€ test/
β”‚   β”œβ”€β”€ components/
β”‚   β”‚   └── street-geo.test.js

vincentfretin avatar May 18 '24 09:05 vincentfretin

Those new tests should be able to be run with mocha and jsdom, no need to spawn a browser. I see existing tests were using jsdom-global already.

vincentfretin avatar May 18 '24 09:05 vincentfretin

Those new tests should be able to be run with mocha and jsdom, no need to spawn a browser. I see existing tests were using jsdom-global already.

Hi, existing tests in 3DStreet do not use AFRAME. I had difficulty to run AFRAME in Node.js environment. And I thought that running AFRAME in a browser environment with the chai library would allow to write tests more conveniently than in Node.js environment. I have now discovered information about using AFRAME in the Node.js environment. I'll try to do it this way

Algorush avatar May 18 '24 16:05 Algorush

I tried using only jsdom, mocha. I still can't run tests for the street-geo component that using AFRAME in a Node.js environment. I was using this info in AFRAME docs about running AFRAME in Node.js, but I'm getting errors in the console when running tests:

import * as SUPER_THREE from 'super-three';
^^^^^^

SyntaxError: Cannot use import statement outside a module

that I haven't found a way to fix. I see these options so far:

  1. I can make a simplified version of the street-geo component for the test by disabling the parts of the code that access AFRAME. Something like a test mode without AFRAME functions. If we only test adding/removing entities.
  2. Use a testing environment similar to the one used when testing AFRAME: https://github.com/aframevr/aframe/tree/master/tests
  3. Use the tests in chai browser environment, perhaps it will be easier than option 2: chai + mocha + puppeter for github Actions (it’s not difficult to set up)

Another option is to further try to fix the errors in point 1, that is, change the three.js code referenced by Aframe. I already had this experience a year ago when I adapted Three.js to work on the server side. But I think it would be easier to do 2 or 3

Algorush avatar May 19 '24 03:05 Algorush

The aframe doc is not be up to date, using require('aframe/src'); only works in a webpack environment like karma-webpack because of aframe/src/lib/three.module.js that's the only code where it uses ES module and also the example in the aframe repo with npm run test:node is currently broken. Now that node 22 supports requiring ES module with require("somemodule.mjs") see https://nodejs.org/en/blog/announcements/v22-release-announce#support-requireing-synchronous-esm-graphs, I made an attempt to fix that in aframe repo in https://github.com/vincentfretin/aframe/tree/test-node and actually it works, but this can only tests basic functions because node environment lacks customElements, even with jsdom. So yeah long story short, you still need to spawn a browser via karma for example like aframe tests are using.

vincentfretin avatar May 20 '24 15:05 vincentfretin

So yeah for now executing the tests manually by accessing http://192.168.1.15:7001/test/browserTests/street-geo-test.html that is executing test/browserTests/street-geo-test-bdd.mjs is enough. (see if you can use the setTimeout(setTimeout there too.) and we can remove test/street-geo.test.js and aframe dependency. We'll look at using karma and karma-webpack in another PR @kfarr I think.

vincentfretin avatar May 20 '24 15:05 vincentfretin

FYI I made this PR https://github.com/aframevr/aframe/pull/5522

vincentfretin avatar May 21 '24 11:05 vincentfretin