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

added unload_current_scene to SceneTree and removed ability for change_scene_to to continue when provided null #63665

Conversation

twobitadder
Copy link
Contributor

This is the bigger half of the change mentioned in #63565 - this introduces a check for null on change_scene_to (it provides the same error of ERR_CANT_CREATE as if it were unable to instantiate the PackedScene).

In order to provide the same functionality as that code, unload_current_scene was added which just nukes current_scene. Based on my working through the code, it seems like that was originally the path if change_scene_to was given a null value.

This is my first time interacting with the source code in anything more than a minor capacity, so comments and suggestions are absolutely welcome and encouraged, of course. I wouldn't be surprised if it could use some work to be in line with spec.

Thanks!

@twobitadder twobitadder requested review from a team as code owners July 30, 2022 06:19
@Calinou Calinou added this to the 4.0 milestone Jul 30, 2022
@twobitadder twobitadder force-pushed the unload_scene_and_updated_change_behavior branch from 94b09b5 to fbeeef7 Compare July 30, 2022 06:56
@twobitadder
Copy link
Contributor Author

Case in point - just learned about clang-format. Revised files incoming.

@twobitadder twobitadder force-pushed the unload_scene_and_updated_change_behavior branch 5 times, most recently from f5d23d8 to 83fa1b8 Compare July 30, 2022 20:53
@Diddykonga
Copy link
Contributor

Diddykonga commented Jul 31, 2022

Personally I would prefer the naming remove_current_scene() instead, just because "unloading" sounds like it will remove the Scene from Load Cache, which it does not do, just a matter of semantics though.

@twobitadder
Copy link
Contributor Author

I would be more inclined to agree if it passed back the scene, so that it would be more in line with behavior from remove_child(). As it stands though, this does at least attempt to memdelete() the object in question. If others agree with the name change though, I'm happy to do it.

@twobitadder twobitadder force-pushed the unload_scene_and_updated_change_behavior branch from 83fa1b8 to 0224aad Compare August 1, 2022 03:08
@twobitadder twobitadder force-pushed the unload_scene_and_updated_change_behavior branch from 0224aad to 682b912 Compare August 1, 2022 03:13
@twobitadder
Copy link
Contributor Author

Superseded by #71105 so i'll go ahead and close!

@YuriSizov
Copy link
Contributor

Yes, sorry for that!

@twobitadder
Copy link
Contributor Author

No problem at all! It's good to have a solution and it's hard to argue against one from reduz, haha

@twobitadder twobitadder deleted the unload_scene_and_updated_change_behavior branch January 13, 2023 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants