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

Add thread guard for force_draw and update related documentation #82953

Conversation

jsjtxietian
Copy link
Contributor

Closes #79190

@jsjtxietian jsjtxietian requested a review from a team as a code owner October 7, 2023 10:56
@Chaosus Chaosus added this to the 4.2 milestone Oct 7, 2023
@akien-mga akien-mga changed the title Document force_draw must be called from main thread Document force_draw must be called from main thread Nov 9, 2023
@akien-mga
Copy link
Member

Is documentation enough to solve #79190, which is a crash? Can we add some thread guards like we have in Node methods, or there's no access to that info from servers? CC @RandomShaper

@jsjtxietian
Copy link
Contributor Author

I believe we should add a thread guard, should I add it to this pr or make another one ?

@jsjtxietian jsjtxietian force-pushed the document-forcedraw-can-only-be-called-from-main-thread branch from 9ebe808 to 97e6da9 Compare November 10, 2023 03:04
@akien-mga
Copy link
Member

akien-mga commented Nov 10, 2023

I think adding a thread guard in this PR too would make sense.

I see examples of it being done this way outside of Nodes (which have their own ERR_MAIN_THREAD_GUARD):

modules/navigation/godot_navigation_server_2d.cpp
186:    ERR_FAIL_COND_MSG(!Thread::is_main_thread(), "The SceneTree can only be parsed on the main thread. Call this function from the main thread or use call_deferred().");

Adding an error message like this would also make sense here, it could be something like:

ERR_FAIL_COND_MSG(!Thread::is_main_thread(), "Manually triggering the draw step from the RenderingServer can only be done on the main thread. Call this function from the main thread or use call_deferred().");

Needs testing to confirm that it works as expected (errors on thread, works on main thread or when using call_deferred()).

@jsjtxietian jsjtxietian changed the title Document force_draw must be called from main thread Add thread guard for force_draw and update related documentation Nov 10, 2023
@RandomShaper
Copy link
Member

I agree with @akien-mga's remarks, but I'd still need clarity on something. Is the compatibility renderer able to run on a thread (rendering/driver/threads/thread_model == RENDER_SEPARATE_THREAD (2))? In that case, it'd still be possible to call any function on any thread. In that case, I'd rather put the thread guards at the Rasterizer*GLES3 level. (For RenderingDevice renderers, the location would be in each RenderingDevice* implementation's function. After the split to RD-RDD happens, threading will be improved with guards, among other things, and I think my points here align with that plan.)

@akien-mga
Copy link
Member

If the situation is more complex than it seemed initially, and this might partially be overridden by the RenderingDeviceDriver work, we might want to stick to just adding a note in the docs as done here for 4.2.

@jsjtxietian
Copy link
Contributor Author

Is the compatibility renderer able to run on a thread (rendering/driver/threads/thread_model == RENDER_SEPARATE_THREAD (2)

Changing thread model to other option will cause crashes too.

Crash log when thread policy is single safe or unsafe:
image

when thread model is multi thread, there will be error spam and then crash:
image

@RandomShaper
Copy link
Member

Well, then let's just follow @akien-mga's idea, until everything can be addressed better.

@jsjtxietian jsjtxietian force-pushed the document-forcedraw-can-only-be-called-from-main-thread branch from 97e6da9 to 0b4a363 Compare November 10, 2023 10:50
@jsjtxietian jsjtxietian requested a review from a team as a code owner November 10, 2023 10:50
@jsjtxietian jsjtxietian force-pushed the document-forcedraw-can-only-be-called-from-main-thread branch from 0b4a363 to 0d95dea Compare November 10, 2023 10:57
force_draw must be called from main thread
@jsjtxietian jsjtxietian force-pushed the document-forcedraw-can-only-be-called-from-main-thread branch from 0d95dea to b88b84c Compare November 10, 2023 10:57
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me, unless we foresee that this extra check could have a non-negligible performance impact.

@akien-mga akien-mga modified the milestones: 4.2, 4.3 Nov 10, 2023
@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Nov 10, 2023
@akien-mga
Copy link
Member

Just in case there's a performance impact, I'm deferring this to merge after the 4.2 release, and we can cherry-pick for 4.2.x if we confirm it's a safe approach.

@YuriSizov YuriSizov merged commit dfe0f58 into godotengine:master Dec 8, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@jsjtxietian jsjtxietian deleted the document-forcedraw-can-only-be-called-from-main-thread branch December 9, 2023 16:38
@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 25, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

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.

Calling force_draw from a thread crashes compatibility renderer
6 participants