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

Make EditorPlugin emits scene_changed when replacing empty scenes. #105

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

WhalesState
Copy link

No description provided.

@jss2a98aj
Copy link
Member

The test that failed here doesn't seem to have failed due to your changes. We should probably re-run it soon.

jss2a98aj
jss2a98aj previously approved these changes Oct 30, 2024
Copy link
Member

@jss2a98aj jss2a98aj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-running the previously failed test resulted in a pass. The changes here seem fine.

Starkium
Starkium previously approved these changes Oct 30, 2024
@salianifo
Copy link

Note for context: Although this signal doesn't have the parameter, the one ultimately emitted on EditorPlugin does include the scene's root node of the currently edited scene at the time the signal is emitted.

My only concern with this, is the other location this signal is emitted, EditorNode::_set_main_scene_state, is only called deferred. I didn't notice any problems in my testing, but it does cause a slight inconsistency when reloading the project.

Previously, when reloading the project with 2+ scenes being restored, this signal is emitted multiple times, but since it is deferred, it passes the scene that becomes active at the end for each signal.

With this change however, the first signal is emitted with the first loaded scene (since it is replacing an empty scene), and the rest with the scene that becomes active at the end.

In case that was confusing, here's a concrete example. We have 3 scenes open (Test1, Test2, and Test3). Test3 is currently active. We have a plugin that listens to this signal and prints the root node name. We reload the project and observe the following in the output:

  • Previous results:

    • Test3
    • Test3
    • Test3
  • New results:

    • Test1
    • Test3
    • Test3

Not sure it is a real problem, but it makes me wonder if there was a reason it was deferred in the first place. If there isn't a reason for it to be deferred, then we should consider making the signal not deferred in other places, or at least when loading scenes. Not sure how feasible that is though, the code for this is kind of all over the place, and I'm sure at least some of those places need it to be deferred to have everything set up correctly.

@jss2a98aj
Copy link
Member

Note for context: Although this signal doesn't have the parameter, the one ultimately emitted on EditorPlugin does include the scene's root node of the currently edited scene at the time the signal is emitted.

That is an interesting observation. It is possible that emitting it for each scene with the correct scene root, or queuing it so it is only emitted once with the most recent scene root would be correct behavior.

@WhalesState
Copy link
Author

WhalesState commented Oct 30, 2024

I have fixed this, by not calling the signal when restoring scenes, so now the other issue is not related to this fix.

	if (replacing_empty_scene && !restoring_scenes) {
		editor_data.notify_edited_scene_changed();
		emit_signal(SNAME("scene_changed"));
	}

When reloading the project with 2 opened scenes, and the first tab is active, it prints the first scene twice, and when reloading while the second tab is active, it prints the first scene then the second one, this won't cause issues in an editor plugin but we should only emit the signal once after reloading the project and restoring all the scenes, maybe it's better to open a new issue for this bug.

@WhalesState WhalesState dismissed stale reviews from Starkium and jss2a98aj via f54b8ec October 30, 2024 22:58
Copy link

@salianifo salianifo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is probably fine for now. We can address the other points in another PR later.

@Bioblaze Bioblaze merged commit fe55908 into blazium-engine:blazium-dev Oct 31, 2024
22 checks passed
@WhalesState WhalesState deleted the empty_scene_issue branch November 29, 2024 22:45
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.

8 participants