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

specify a URL to load scene from json file (v1) #352

Closed
wants to merge 6 commits into from

Conversation

Algorush
Copy link
Collaborator

No description provided.

@netlify
Copy link

netlify bot commented Sep 15, 2023

Deploy Preview for 3dstreet-core-builds ready!

Name Link
🔨 Latest commit f2c7aea
🔍 Latest deploy log https://app.netlify.com/sites/3dstreet-core-builds/deploys/655bf0518a762900086707f3
😎 Deploy Preview https://deploy-preview-352--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.

@kfarr
Copy link
Collaborator

kfarr commented Sep 21, 2023

test case:

Now that cors server-side error is fixed, I get the error

A few issues:

  • I have set a value for jsonURL in the glitch scene, but it does not load: <a-entity json-3dstreet="jsonURL: https://dev-3dstreet.web.app/scenes/b020190e-29a0-453e-8ee1-bcf1ca2a6d69.json"></a-entity>
  • Instead the workaround is to past the following into console AFRAME.scenes[0].setAttribute('json-3dstreet', 'jsonURL: https://dev-3dstreet.web.app/scenes/b020190e-29a0-453e-8ee1-bcf1ca2a6d69.json');

When using the console to open the URL, the first error I receive is:

3dstreet-pr352.js:16 Uncaught ReferenceError: createElementsFromJSON is not defined
at e.onload (3dstreet-pr352.js:16:15392)

This is because json-utils isn't loaded, however in this use case I should be able to load the scene without needing a separate file like json-utils, so it will need to be somehow incorporated into 3dstreet build

When I do include json-utils, then I get another error:

json-utils.js:564 Uncaught TypeError: Cannot read properties of null (reading 'firstChild')
at createElementsFromJSON (json-utils.js:564:28)
at e.onload (3dstreet-pr352.js:16:15392)

I don't know how to fix this error.

In summary, there are 3 issues @Algorush

  • passing a valid value for 'jsonURL' in json-3dstreet component at page load should load the scene without further action needed
  • all relevant functions to open a scene using this component should be in the 3dstreet js build
  • the scene should load and not emit an error Uncaught TypeError: Cannot read properties of null (reading 'firstChild')

@Algorush
Copy link
Collaborator Author

Algorush commented Oct 8, 2023

I made these 3 points in the new commit. Now json-utils is called through require in index.js. But now inputStreetmix and inputJSON which are used in index.html for UI elements cannot be used. I'm thiking how to best fix it? Add this functions to one of compoents or add them to index.html? Or describe index.js as a module and export functions from module.
Maybe I misunderstood you and I need to add all the relevant functions from json-utils inside the json-3dstreet component?

@kfarr
Copy link
Collaborator

kfarr commented Oct 11, 2023

But now inputStreetmix and inputJSON which are used in index.html for UI elements cannot be used. I'm thiking how to best fix it? Add this functions to one of compoents or add them to index.html?

@Algorush yes I think it's a good idea to put the inputStreetmix and inputJSON into their appropriate component counterparts as functions to be called from front-end ui.

Maybe I misunderstood you and I need to add all the relevant functions from json-utils inside the json-3dstreet component?

yes perhaps it's inputStreetmix function into streetmix-loader component and inputJSON function into json-3dstreet component for example.

@Algorush
Copy link
Collaborator Author

Algorush commented Oct 17, 2023

Will index.html in the future have #street-container and #default-street elements or create them dynamically inside a-scene at the first level? Asking because I think do I need to make check for their existing for all places where they needed

@Algorush
Copy link
Collaborator Author

And I also ask because if we place the inputStreetmix function (with the prompt request) inside the component, then it will need to be called when assigning the streetmix-loader attribute, which is called for the #default-street element

@kfarr
Copy link
Collaborator

kfarr commented Oct 18, 2023

@Algorush I think it's ok to require that certain elements exist on the page for v1

The primary use case is to allow people to bring their 3dstreet scenes to a custom html a-frame scene often hosted on glitch.com . We can create the basic glitch template that includes these elements as a starting point that people can "remix" from

@Algorush
Copy link
Collaborator Author

Algorush commented Nov 1, 2023

I moved the inputStreetmix function inside the streetmix-loader component. But still part of the code remains in index.html. This is the eventListener code for a button with a custom input prompt

- move input event listener to json-3dstreet component
- move prepareStreetContainer function to street component
- add notify messages
@Algorush
Copy link
Collaborator Author

Algorush commented Nov 20, 2023

I moved the addEventListener for the change event for the inputSteetmix URL inside the json-3dstreet system. I don't know if this is a good idea, but it works and there is less code in index.html.
The same can be done for streetmix-loader, but is there a suggestion here to abandon streetmix-loader #398? Or did I misunderstand?
I took a long time to debug this time. One or the other did not work as it should. At some point I even lost my goal, why am I doing all this :) It seems that everything worked fine before too. Sorry for offtop

@kfarr
Copy link
Collaborator

kfarr commented Jan 5, 2024

@Algorush I'm reviewing this and struggling to understand all of the code. I wonder if there are too many changes going on at the same time and we should go 1 step at a time.

Here are the core user goals:

  1. As a 3DStreet developer I want json-utils to exist only on 1 repo and be included in index.js
  2. As an A-Frame developer I want to be able to specify a 3DStreet scene ID and/or scene URL in an A-Frame component so that I can include a 3DStreet scene in my A-Frame project.

Maybe we start with a very simple PR that does #1 only and then a separate PR that does #2 only?

@Algorush
Copy link
Collaborator Author

Algorush commented Jan 6, 2024

Maybe we start with a very simple PR that does #1 only and then a separate PR that does #2 only?

Yes, I agree that this will be better.

@Algorush
Copy link
Collaborator Author

Algorush commented Feb 1, 2024

Ok, I will gradually transfer the changes from this PR to the new PRs for more convenient. This will take some time.

@kfarr
Copy link
Collaborator

kfarr commented Feb 4, 2024

Closing this for now, we are splitting this into separate tasks

@kfarr kfarr closed this Feb 4, 2024
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 this pull request may close these issues.

2 participants