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

Prevent error at creating Control nodes in threads #77766

Closed

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jun 2, 2023

This PR makes the Control skip theme cache initialization if it's created in a thread. The theme properties are not accessible anyway, so I think it's fine if they are not correct. We could maybe document that accessing theme in thread is not possible (but other properties should be fine).

Fixes #77764

@KoBeWi KoBeWi added this to the 4.1 milestone Jun 2, 2023
@KoBeWi KoBeWi requested a review from a team as a code owner June 2, 2023 10:14
@YuriSizov
Copy link
Contributor

YuriSizov commented Jun 2, 2023

I don't think it's appropriate to allow creating incorrectly initialized GUI nodes under specific conditions. As far as I understand Juan, Control nodes are not supported outside of the main thread. So adding workarounds here just to suppress valid errors is wrong. It's also completely hidden from the user.

The theme properties are not accessible anyway

They are, from the project theme and from the default theme.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jun 2, 2023

I mean they are not accessible from the thread, because you will get the error that this PR prevents. The problem was that the blocked methods were called automatically, beyond user control.

@YuriSizov
Copy link
Contributor

YuriSizov commented Jun 2, 2023

It's a part of the initialization logic, it's not supposed to be within user's control. It's there to ensure that nodes are correct and can be used. Same thing can happen with any other methods of GUI nodes during the initialization step. If GUI API is not supported in threads, then we should not expect the creation of such nodes to be allowed either.

I mean they are not accessible from the thread, because you will get the error

That said, I don't see why this shouldn't work from threads in principle.

@akien-mga akien-mga requested a review from RandomShaper June 2, 2023 11:39
@RandomShaper
Copy link
Member

I don't think it's appropriate to allow creating incorrectly initialized GUI nodes under specific conditions. As far as I understand Juan, Control nodes are not supported outside of the main thread.

I think that's accurate.

That said, I don't see why this shouldn't work from threads in principle.

The problem is that the Control is caching stuff. If we allow a node to be written to within group (threaded) processing, the code must be updated for proper sync. For an example, see Node2D::_is_xform_dirty() and pull the thread from there.

@YuriSizov
Copy link
Contributor

YuriSizov commented Jun 7, 2023

The problem is that the Control is caching stuff. If we allow a node to be written to within group (threaded) processing, the code must be updated for proper sync. For an example, see Node2D::_is_xform_dirty() and pull the thread from there.

Hmm, I see. Yes, theme property getters have a cache and can write to it in some cases. So these operations would need to be remade with MT in mind, and then calling get_theme_xxx from a thread should be possible?

As far as I understand, the biggest concern with threads in controls is computing sizing, and these methods aren't related to that.

@RandomShaper
Copy link
Member

That's right.

However, my take is to keep GUI single-threaded for the time being, since, as you can see, otherwise the implications can start to grow big and we're not even done with adapting the elements that need it for the main use to MT.

I'm pretty sure @reduz's design choice also matches this idea.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jun 9, 2023

Closing in favor of #78000
It allows the user to intentionally enable unsafe code, and more importantly also fixes the error, so it's better.

@KoBeWi KoBeWi closed this Jun 9, 2023
@KoBeWi KoBeWi added the archived label Jun 9, 2023
@KoBeWi KoBeWi removed this from the 4.1 milestone Jun 9, 2023
@KoBeWi KoBeWi deleted the controls_from_another_dimension branch June 9, 2023 13:51
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.

Creating themable Control nodes in thread results in an error
3 participants