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

some save / load issues #263

Closed
kfarr opened this issue Apr 8, 2023 · 11 comments
Closed

some save / load issues #263

kfarr opened this issue Apr 8, 2023 · 11 comments
Assignees

Comments

@kfarr
Copy link
Collaborator

kfarr commented Apr 8, 2023

  1. files should be versioned - loaders may need to use different parsers depending on versions to maintain backward compatibility (we don't want angry users saying their older files no longer load when we optimize the format in the future)
  2. If I save a json street file and then reload it, I can no longer change the mixin to another object in the editor (see video below)
  3. files should include title
  4. deleting sky is not persisted if sky deleted and reloaded. instead, "Environment" node should be included in save file. (move the current 3d street scene node down one level under a parent of 3d street layers) When reloading respect the state of the environment node from the saved file.
  5. 2D Layers should also be included as a separate node in the file

changing mixin after reload issue video:
https://user-images.githubusercontent.com/470477/230750937-742f1152-4fbd-4b12-b6ec-1b78f34e13a7.mov

@github-project-automation github-project-automation bot moved this to Not Ready To Do in 3DStreet Dev Tracking Apr 8, 2023
@kfarr kfarr moved this from Not Ready To Do to To Do in 3DStreet Dev Tracking Apr 8, 2023
@Algorush
Copy link
Collaborator

Algorush commented May 19, 2023

I have a few questions.

  1. That is, there must be json-utils.js versions and file versions. At the same time, there should be backward compatibility for older versions of JSON files.
  2. I fixed it, but another problem appeared - the Scene graph on the left panel is not updated. That is, the added entities are not visible. The scene graph is updated only after the editor is reloaded. I'm working on it... It's strange that AFRAME.INSPECTOR.close() and open() don't work.
  3. title should be from Streetmix-loader -> Name ?
  4. What is 2D Layers? JSON from Streetmix API? Example: https://streetmix.net/api/v1/streets/89745730-5346-11e9-87b5-e5a177d820ab

@kfarr kfarr moved this from To Do to In progress in 3DStreet Dev Tracking May 20, 2023
@kfarr
Copy link
Collaborator Author

kfarr commented May 21, 2023

Thanks @Algorush , here are some answers:

  1. That is, there must be json-utils.js versions and file versions. At the same time, there should be backward compatibility for older versions of JSON files.

Yes, for example we could say our current version is unversioned and will be unsupported in the future, but the next will be v1. When a file is saved in v1 the json includes metadata such as "version": "v1" as a sibling to "data": ... and the loader looks for this v1 in order to parse it or raise an error. If we make breaking changes in the future then we save new files as v2 and there is a separate if / then statement to use the v2 loader, etc.

  1. I fixed it, but another problem appeared - the Scene graph on the left panel is not updated. That is, the added entities are not visible. The scene graph is updated only after the editor is reloaded. I'm working on it... It's strange that AFRAME.INSPECTOR.close() and open() don't work.

Let's tackle that as a separate issue 3DStreet/3dstreet-editor#99

  1. title should be from Streetmix-loader -> Name ?

Eventually there will be scene title to be included in the filename such as "scenetitle.3dstreet.json". For now it could be "scene.3dstreet.json"

  1. What is 2D Layers? JSON from Streetmix API? Example: https://streetmix.net/api/v1/streets/89745730-5346-11e9-87b5-e5a177d820ab

2D Layers are intended to be maps layer or other 2d images (png/jpg/svg) used as scene references such as CAD file. You can see an example here: https://github.3dstreet.org/mapbox.html

@Algorush
Copy link
Collaborator

Ok, thank you for explanation.
I also want to do an optimization for the content of the JSON file. Now an element's components can contain attributes found in the mixin that is set for the element. And for duplicate elements (e.g. Tree, Utility-pole) the JSON will retain many duplicate attributes, such as src with the full glb path. I want to make a validation that will not save the attributes that are in the element's mixin

@kfarr
Copy link
Collaborator Author

kfarr commented May 24, 2023

Hey @Algorush yes probably the most important is to not store the gltf model, geometry or material data if they are already defined in the mixin

@kfarr
Copy link
Collaborator Author

kfarr commented Jun 2, 2023

Transposed comment from discord

Kieran said: "4. deleting sky is not persisted if sky deleted and reloaded. instead, "Environment" node should be included in save file. (move the current 3d street scene node down one level under a parent of 3d street layers) When reloading respect the state of the environment node from the saved file."

@Algorush says: Maybe here you meant to move the 'environment' node one level down, that is, make it a child of street-container? The node with the 'street' component is already a child of the '3D-street-layer'. Or I misunderstood?

There should be 4 nodes something like: 3dstreet, 2dstreet, environment, viewer

@Algorush says: And a question about '2D-Layers' node. Should it be included in the JSON adjacent to the street-container node? That is: {
title: 'scene',
version: '1.0',
data: {
[
{"element":"a-entity","id":"street-container", ... },
{"element":"a-entity","id":"layers-2d", ... }
]
}
}

Yes, and also environment and viewer

@Algorush
Copy link
Collaborator

Algorush commented Jun 3, 2023

should I also change the structure of index.html i.e. move the environment and 2d-layers nodes inside the street-container? Or only in JSON and leave the index.html structure as it is?

@kfarr kfarr moved this from In progress to For Review in 3DStreet Dev Tracking Jun 6, 2023
@kfarr
Copy link
Collaborator Author

kfarr commented Jun 6, 2023

for review #287

@kfarr
Copy link
Collaborator Author

kfarr commented Jun 7, 2023

@Algorush in theory there should be 4 nodes at the same level:
environment
2d-layers
street-container (another name for 3d layer)
viewer

So no they shouldn't be inside street-container

@kfarr
Copy link
Collaborator Author

kfarr commented Jun 7, 2023

here's another view of intended node structure

├── scene
│   ├── street-container (aka 3d layer)
│   │   ├── streets
│   │   ├── buildings
│   ├── environment
│   ├── 2d layers
│   ├── viewer

@Algorush
Copy link
Collaborator

Algorush commented Jun 7, 2023

Ok, understood. Thanks for explanation

@kfarr
Copy link
Collaborator Author

kfarr commented Jun 10, 2023

closing this one, making new issues for next steps such as #292 and #291 and #295

@kfarr kfarr closed this as completed Jun 10, 2023
@github-project-automation github-project-automation bot moved this from For Review to Done in 3DStreet Dev Tracking Jun 10, 2023
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

No branches or pull requests

2 participants