-
-
Notifications
You must be signed in to change notification settings - Fork 19.9k
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 the ability to see and unload Resources loaded by ResourceLoader.load_threaded_request() #86603
base: master
Are you sure you want to change the base?
Add the ability to see and unload Resources loaded by ResourceLoader.load_threaded_request() #86603
Conversation
core/io/resource_loader.cpp
Outdated
thread_load_mutex.lock(); | ||
for (KeyValue<String, ResourceLoader::LoadToken *> &kvp : user_load_tokens) { | ||
requested_paths.push_back(kvp.key); | ||
} | ||
thread_load_mutex.unlock(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thread_load_mutex.lock(); | |
for (KeyValue<String, ResourceLoader::LoadToken *> &kvp : user_load_tokens) { | |
requested_paths.push_back(kvp.key); | |
} | |
thread_load_mutex.unlock(); | |
MutexLock lock(thread_load_mutex); | |
for (KeyValue<String, ResourceLoader::LoadToken *> &kvp : user_load_tokens) { | |
requested_paths.push_back(kvp.key); | |
} |
Safer and preferred usage with trivial locking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for reviewing my PR, I've applied your suggestion!
Welcome to contributing! Will take a deeper dive into this when I have the time 🙂 |
b1cde5b
to
6342624
Compare
core/io/resource_loader.cpp
Outdated
@@ -420,6 +420,44 @@ Error ResourceLoader::load_threaded_request(const String &p_path, const String & | |||
} | |||
} | |||
|
|||
void ResourceLoader::load_threaded_cancel(const String &p_path) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm understanding correctly, this is a load_threaded_get()
without honoring reference counting. In that case, I'd suggest refactoring the code so both functions share a common implementation. There would be a new argument to ignore the ref-count and instead relentessly clear it. Moreover, since it's not actually a proper cancel, I'd name this one load_threaded_forget()
. But this leads to the question, wouldn't it be even cooler to have a proper load_threaded_cancel()
that actually stops the load? Nonetheless, that would be much harder to implement and could be an addition for the future. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello RandomShaper,
Thank you for your feedback. I have read your post and agree with pretty much the entirety of your thoughts. I'm sick right now, but I will get back into this and give a more comprehensive answer.
Refactoring the code is something I had been avoiding as not to break previously existing code, but is obviously worth considering if such a change is positively welcomed by the community.
When it comes to cancelling the actual loading process from load_threaded_request(), I will have to dive deeper into its current implementation and figure out how to do this in a thread-safe manner.
Will spend more time on this when my brain works again.
d8238f0
to
d262e83
Compare
…Loader.load_threaded_get()
d262e83
to
8e4d560
Compare
I've made a few changes following feedback from @RandomShaper and after acquiring a deeper understanding of ResourceLoader.
I have tried implemented a form of Because I hadn't actually realized before working on this that Thank you for your interest so far, and please inform me of any feedback or criticism you may have. |
In response to my own needs and inspired by this proposal, I've added 2 new methods to ResourceLoader:
Without reiterating the aforementioned thread, I believe those are core features needed to properly manage background loading of Resources in many different contexts. This will let users track and cancel Resources they haven't ended up needing and will work out of the box in existing projects.
Looking forward to your feedback and criticism.