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

A couple of enhancements to user-visible threading semantics #86957

Merged
merged 2 commits into from
Jan 11, 2024

Conversation

RandomShaper
Copy link
Member

Kept in two commits because, regardless both related to threading, the changes are heavily unrelated otherwise.

I'd appreciate especially careful reviewing of my GDVIRTUAL changes, as this is the first time I deal with that.

@RandomShaper
Copy link
Member Author

CC @SlugFiller

@RandomShaper RandomShaper changed the title A couple of fixes to user-visible threading semantics A couple of enhancements to user-visible threading semantics Jan 8, 2024
@SlugFiller
Copy link
Contributor

Cool! Nice to see things are happening quickly. Here's hoping it gets merged soon.

@YuriSizov
Copy link
Contributor

Here's hoping it gets merged soon.

Did you test it, do you approve it? 🙃

@SlugFiller
Copy link
Contributor

With just the first commit, I hit the same deadlock in running ResourceSaver.save in parallel.

With both commits, and _can_import_threaded set to return false, it works perfectly. However, if I don't implement _can_import_threaded, Godot poofs on import. So something is definitely not right about the GDVIRTUAL_CALL.

@RandomShaper
Copy link
Member Author

@SlugFiller, thank you for the feedback. I'm wondering, what happens if you override but return true?

@SlugFiller
Copy link
Contributor

When returning true it works with single, or deadlocks on ResourceSaver.save if done in parallel. If I don't override it, it poofs even on single.

@RandomShaper
Copy link
Member Author

I've fixed a very silly mistake causing infinite recursion if the method is not overridden. Please test again with false. The problem with parallel saving will have to be investigated and fixed separately as it's not in the scope of this PR.

@SlugFiller
Copy link
Contributor

Poofing is gone, and returning false works for importing 3 scenes at once. Seems great.

doc/classes/EditorImportPlugin.xml Outdated Show resolved Hide resolved
@akien-mga akien-mga merged commit cab2749 into godotengine:master Jan 11, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@RandomShaper RandomShaper deleted the mt_mends branch January 11, 2024 17: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.

4 participants