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

Editor: Avoid SetSceneCommnd when importing file #28339

Merged
merged 3 commits into from
May 15, 2024

Conversation

ycw
Copy link
Contributor

@ycw ycw commented May 11, 2024

The issue: Importing 'object' scene(.json) currently performs a mix of 'appending object to default scene' and 'replacing default scene userData', this is problematic because objects in default scene may have scripts consuming userData of default scene, after importing one/multiple scenes(.json), the default scene's userData get replaced by the last imported one, e.g.:

A simple scene with a plane which has a script consuming scene.userData

Camera
Scene  <<< holding original scene userData
   Plane 

After importing scene1.json

Camera
Scene  <<< holding scene1 userData 
   Plane
   Box   <<< object from scene1

After importing scene2.json

Camera
Scene  <<< holding scene2 userData
   Plane
   Box   <<< object from scene1
   Ring  <<< object from scene2

I.e. It is possible that when users PLAY the project, it'll throw because the original scene userData is gone.


This PR replaced SetSceneCommand by AddSceneCommand, so each imported file is imported as a child of default scene, the graph for the above mentioned example would become:

Camera
Scene  <<< holding original scene userData
  Plane
- Scene1  <<< holding scene1 userData
    Box 
- Scene2  <<< holding scene2 userData
    Ring

This way, the script of Plane can even access the imported scenes' userData via scene graph traversal


If replacing scene is desired, then I suggest implement File>Open Project instead because it has better semantic.

Implemented, see #28351

editor/js/Loader.js Outdated Show resolved Hide resolved
@Mugen87 Mugen87 added this to the r165 milestone May 13, 2024
@Mugen87
Copy link
Collaborator

Mugen87 commented May 13, 2024

Do you mind rebasing the PR?

@ycw ycw force-pushed the editor-avoid-SetSceneCommand branch from 4788cad to 44062ed Compare May 14, 2024 21:37
@Mugen87 Mugen87 merged commit b3e3724 into mrdoob:dev May 15, 2024
9 of 10 checks passed
@Mugen87
Copy link
Collaborator

Mugen87 commented May 15, 2024

Do you mind rebasing the PR?

Never mind! I did it now.

@ycw
Copy link
Contributor Author

ycw commented May 15, 2024

thanks

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

Successfully merging this pull request may close these issues.

2 participants