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

Godot 4.0.3 RC2 exits immediately after clicking on a Skeleton3D node #77046

Closed
uguu-org opened this issue May 14, 2023 · 9 comments · Fixed by #77074
Closed

Godot 4.0.3 RC2 exits immediately after clicking on a Skeleton3D node #77046

uguu-org opened this issue May 14, 2023 · 9 comments · Fixed by #77074

Comments

@uguu-org
Copy link

Godot version

v4.0.3.rc2.official.2ac4e3bb3

System information

Windows 10, NVIDIA GeForce GTX 745

Issue description

Godot 4.0.3 RC2 exits immediately after clicking on a Skeleton3D node. This does not happen with 4.0.3 RC1, so it appears to be a regression with RC2.

Steps to reproduce

  1. Open model.tscn in attached project.
  2. Click on Skeleton3D node
  3. Observe that Godot draw a box briefly, then exits before showing skeleton hierarchy.
    click_on_skeleton

Expected behavior (from RC1). I am having trouble recording mouse cursor, but basically I clicked on Skeleton3D on the left and the bone hierarchy shows up on the right.

godot_4_0_3_rc1.mp4

Actual behavior (from RC2). The yellow box shows up, and Godot exits abruptly, which causes the recorder to stop.

godot_4_0_3_rc2.mp4

Minimal reproduction project

skeleton_test.zip

@lyuma
Copy link
Contributor

lyuma commented May 14, 2023

cc @lyuma @fire @TokageItLab

@fire
Copy link
Member

fire commented May 14, 2023

Looking into it too.

@fire
Copy link
Member

fire commented May 14, 2023

Test plan:

  1. recreate problem
  2. get backtrace
  3. git bisect the list of changes from rc1 to rc2

@akien-mga
Copy link
Member

Probably #76592, CC @spanzeri

@akien-mga akien-mga added the bug label May 14, 2023
@akien-mga akien-mga added this to the 4.1 milestone May 14, 2023
@uguu-org
Copy link
Author

Attached is a smaller test project, click on Skeleton3D node to observe the abrupt exit.
skeleton_test_2.zip

click_on_skeleton

@spanzeri
Copy link
Contributor

I'll have a look

@spanzeri
Copy link
Contributor

#77074 this fixes the crash.
However, I am not sure about the fix at all.
As I mentioned in the PR, Control nodes seems to fire THEME_CHANGED notifications before propagating ENTER_TREE to derived classes.
Accessing theme resources (to create the editor) before enter tree has caused a lot of warnings everywhere else.
I am not completely familiar with how those system works yet (I am fairly new to the engine) and I could use someone with more expertise double checking what the correct sequence of events is (either the editor needs to be created at a different moment in time or something is not quite right with the way Controls handles ENTER_TREE)

@lyuma
Copy link
Contributor

lyuma commented May 15, 2023

Some notes from rocket chat:

The part that was confusing to me was that is_inside_tree() returns true before _notification NOTIFICATION_ENTER_TREE has fired, but this is not new behavior. (the parent class is synchronously triggering another notification)

Before the PR #76592, widgets were being constructed in the constructor. from reading other editor code, this pattern is universal or very common

So by calling create_editors after the constructor, it breaks the pattern. Supposedly, some editor constructions were moved to ENTER_TREE, not just for this node. But I haven't seen this, and Yuri confirmed that it is rare to construct widgets in this way.

The other problem with #76592: if ENTER_TREE happens multiple times (reloading or reparenting) this will leak mem.

In summary, most editor components construct everything in the constructor, so they do not need to worry about ordering between ENTER_TREE and THEME_CHANGED. Otherwise, it might be necessary to use null check instead of is_inside_tree()

From @akien-mga :

To be clear, the correct way to make editor UI afaik is:

  • Create and add children in constructor
  • Tweak the theme in THEME_CHANGED. If you do it in the constructor, you get a warning

@uguu-org
Copy link
Author

Thanks for the fix and the explanation :)

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

Successfully merging a pull request may close this issue.

5 participants