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

support new streetmix metric plan #476

Closed
kfarr opened this issue Feb 23, 2024 · 4 comments · Fixed by #486
Closed

support new streetmix metric plan #476

kfarr opened this issue Feb 23, 2024 · 4 comments · Fixed by #486
Assignees

Comments

@kfarr
Copy link
Collaborator

kfarr commented Feb 23, 2024

Currently streetmix json is in imperial units. Instead it will soon offer metric units.

The key code change is:

  • For schema version >= 30 -- All units in API response are metric
  • For schema version < 30, all units in API response are imperial

More notes and sample API responses for new metric responses:
streetmix/streetmix#2900

@Algorush
Copy link
Collaborator

Algorush commented Feb 27, 2024

I suggest to change once in one place of the code all the width values ​​in the object with the data received from the streetmix API. So as not to convert to meters everywhere where necessary. @kfarr what do you think about this?
Because, the processSegments function in aframe-streetmix-parser recieve only part of recieved JSON with segments data. But the schemaVersion, as well as units, are at a higher level in recieved JSON.
That is, there will be no need to change the structure of existing code. And remove the conversion of feet to meters everywhere. And moving it to the moment the JSON response is received from the API in streetmix-loader component

@Algorush
Copy link
Collaborator

More notes and sample API responses for new metric responses:

PR #486. Tested with all cases from here streetmix/streetmix#2900

@Algorush
Copy link
Collaborator

I'll change tests also

@kfarr kfarr linked a pull request Feb 29, 2024 that will close this issue
@kfarr kfarr closed this as completed in #486 Mar 3, 2024
@kfarr
Copy link
Collaborator Author

kfarr commented Mar 4, 2024

Just sharing notes testing with a streetmix staging server providing metric responses

Testing technique, load the 3dstreet app and type the following in console:

newEl = document.createElement('a-entity')
newEl.setAttribute('streetmix-loader', 'streetmixAPIURL: https://streetmix-staging.herokuapp.com/api/v1/streets?namespacedId=295')
parentEl = document.querySelector('#street-container')
parentEl.appendChild(newEl)

Behavior on 3dstreet.app (production) using OLD parser, parsing the NEW streetmix metric API response. This is using old parser that does not support metric, therefore we expect this to look incorrect, and it does!
image

Behavior on github.3dstreet.org with NEW streetmix metric API response which supports the new metric api is correct:
image

Behavior on github.3dstreet.org with OLD streetmix imperial API response:
image

Therefore both imperial and metric responses work as expected on the new release. I think we're good!

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

Successfully merging a pull request may close this issue.

2 participants