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

fix save/load mapbox scene #440

Merged
merged 6 commits into from
Feb 22, 2024
Merged

Conversation

Algorush
Copy link
Collaborator

  • move environment and cameraRig to a-acene parent
  • fix saving mapbox component data

It turned out that I had not corrected saving the scene with the mapbox component before. I fixed the intersection scene, not mapbox. Now after these changes, saving and loading works, but there are still strange errors when loading.
I'll check further. We will also need to change json-utils.js in the 3DStreet-editor repo.
I know it's time to load json-utils.js from index.js. I did it in this PR, which probably needs some work: #352

- move environment and cameraRig to a-acene parent
- fix saving mapbox component data
Copy link

netlify bot commented Dec 29, 2023

Deploy Preview for 3dstreet-core-builds ready!

Name Link
🔨 Latest commit 0819b15
🔍 Latest deploy log https://app.netlify.com/sites/3dstreet-core-builds/deploys/65d62c908f707200086b52d8
😎 Deploy Preview https://deploy-preview-440--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.

src/json-utils.js Outdated Show resolved Hide resolved
@kfarr
Copy link
Collaborator

kfarr commented Jan 5, 2024

@Algorush

I can't quite understand what is going on here with the changes to saving the component data, sorry. That's probably ok, I don't need to understand everything.

However, it makes me think that we need a standardized manual test process for testing changes to json save / load functions. Right now I just go in there and click around and try randomly to load something, make changes, save them, and reload them. But that's not a standardized procedure and you probably test differently than I do.

Or stated differently, how would you suggest testing a change like this to make sure it doesn't adversely affect saving other components?

@Algorush
Copy link
Collaborator Author

Algorush commented Jan 5, 2024

Yes, I completely agree that it would be very useful to write tests to test all possible cases of save/load JSON scenes.

@kfarr
Copy link
Collaborator

kfarr commented Jan 15, 2024

@Algorush I think this is ready for merge except for your suggestion above:

I just looked at examples of saved JSON and only now I saw that streetmix URLs are also saved in separated form:
{'http': '//streetmix.net...'}
But now this does not affect the work, as far as I understand... Since streetmix-loader is saved as not-streetmix-loader.
So its better to use regex to check for such cases. I can make it in this branch

I'm not sure if it is needed or not to add this regex check?

@Algorush
Copy link
Collaborator Author

I'm not sure if it is needed or not to add this regex check?

not now. This may be useful when we need to save the parameters of the streetmix-loader component in JSON. Or if we save a url containing :// for some attribute for future scenes

@kfarr
Copy link
Collaborator

kfarr commented Jan 16, 2024

thanks for the update @Algorush

Here is my plan to test:

  • go the hardcoded mapbox url
  • save to cloud
  • restart app
  • load from cloud
  • see if mapbox thing displays

that is test for this PR

Then, once we are ready for a new 3dstreet "release" I should write up a flow that does a couple things like this, and testing a few other flows like importing from streetmix, etc.

@Algorush
Copy link
Collaborator Author

Algorush commented Jan 16, 2024

I ended up fixing the filterJSON code. I discovered what the issue was. Now there is no need for additional checks for mapbox or URLs. I also noticed that the ocean material in the scene is now saved and loaded correctly. And now there are no error messages when loading JSON.
I'll still test it on different scenes. But I think save/load should work as well or better

@Algorush
Copy link
Collaborator Author

thanks for the update @Algorush

Here is my plan to test:

  • go the hardcoded mapbox url
  • save to cloud
  • restart app
  • load from cloud
  • see if mapbox thing displays

that is test for this PR

Then, once we are ready for a new 3dstreet "release" I should write up a flow that does a couple things like this, and testing a few other flows like importing from streetmix, etc.

this test is currently passed. And with other scenes too. Only I noticed a strange thing in the DOM inspector after load JSON: components not-street="[object object ]"

@Algorush
Copy link
Collaborator Author

Algorush commented Feb 4, 2024

Only I noticed a strange thing in the DOM inspector after load JSON: components not-street="[object object ]"

I'll see how to fix this

@Algorush
Copy link
Collaborator Author

Algorush commented Feb 5, 2024

This PR is only about saving, it does not affect the JSON loading logic, so the functionality will be backwards compatible with previous JSON scenes

@Algorush
Copy link
Collaborator Author

About the fact that similar things appear: not-street='[object object ]'
This has happened before, so it's better to create a separate issue

@Algorush
Copy link
Collaborator Author

I changed operator ?? by ||. After this, it began to work better, the problem with splitting the URL into two parts went away.

@kfarr kfarr changed the base branch from main to mapbox-and-custom-svg-epic February 22, 2024 00:36
@kfarr
Copy link
Collaborator

kfarr commented Feb 22, 2024

merging this into a combined epic for testing all these changes together

@kfarr kfarr merged commit 7b4c56b into mapbox-and-custom-svg-epic Feb 22, 2024
6 checks passed
@kfarr kfarr deleted the fix-saving-mapbox-scene branch February 22, 2024 00:36
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.

None yet

2 participants