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

Import assets before loading editor plugins #57270

Conversation

TechnoPorg
Copy link
Contributor

Hopefully fixes #36713!
This time, the addons folder is scanned first by the EditorFileSystem, then addons (including import plugins) are loaded, then the EditorFileSystem is updated and scans the rest of the project. This approach should prevent the regressions present in #52344: #52968 and #52995. I've tested this with an admittedly thrown together project, and I'm not experiencing them. I don't have an MRP for #36713 that works in 4.0 alpha, so I'm marking this PR as draft since I haven't tested whether it fixes what it's supposed to fix 😕 . If someone can test it, that would be great, but I probably should be keeping a separate 3.x clone on my computer anyways to use.

@YuriSizov YuriSizov added this to the 4.0 milestone Jan 26, 2022
@YuriSizov YuriSizov requested review from a team January 26, 2022 19:33
@TechnoPorg TechnoPorg force-pushed the import-assets-first-but-not-broken branch 2 times, most recently from 1e70a02 to c7e6433 Compare January 26, 2022 22:09
@akien-mga
Copy link
Member

That's an interesting approach that should indeed solve most situations.

We need to check whether it's actually mandatory for editor plugins to reside in the res://adons folder though. If not, then this wouldn't work for plugins which have been placed somewhere else (either from a manual download or ad hoc plugins written for the project directly).

@TechnoPorg
Copy link
Contributor Author

If not, then this wouldn't work for plugins which have been placed somewhere else (either from a manual download or ad hoc plugins written for the project directly).

Yeah, that occurred to me after submitting this PR as well. I believe this is in fact the case, based on a tweet Juan made a while ago: https://twitter.com/reduzio/status/1418659366438309896?s=20

@TechnoPorg TechnoPorg force-pushed the import-assets-first-but-not-broken branch from c7e6433 to 04d5135 Compare January 28, 2022 18:24
@TechnoPorg
Copy link
Contributor Author

I've changed my approach, to be more flexible with addon locations. I'd be surprised if things still work properly after this, though, since I don't fully understand the code here. Feedback from plugin wizards would be appreciated.

@reduz
Copy link
Member

reduz commented Feb 7, 2022

The idea in general is that addons for these kinds of things should slowly be deprecated in favor of extensions, and there should be a way for scripts to be able to do this beforehand too. I think this needs a bit more discussion so it can be done in a cleaner way.

@TechnoPorg
Copy link
Contributor Author

The idea in general is that addons for these kinds of things should slowly be deprecated in favor of extensions, and there should be a way for scripts to be able to do this beforehand too. I think this needs a bit more discussion so it can be done in a cleaner way.

Do you mean that editor plugins with custom importers should be deprecated in favor of native extensions? If so, I think assets should still be loaded first, in case a native extension tool loads an asset. Sorry in advance for not understanding this better.

@TechnoPorg TechnoPorg force-pushed the import-assets-first-but-not-broken branch 2 times, most recently from 653ae15 to b6867b8 Compare February 10, 2022 02:52
Take 2: This time, the addons folders are scanned first by the EditorFileSystem, then addons (including import plugins) are loaded, then the EditorFileSystem is updated and scans the rest of the project. This approach should prevent the regressions present in 1963c63.
@TechnoPorg TechnoPorg force-pushed the import-assets-first-but-not-broken branch from b6867b8 to 14ecad4 Compare February 27, 2022 23:32
@TechnoPorg
Copy link
Contributor Author

I'm wondering if a better approach might be to treat the filesystem like an optional cache, if the first scan isn't complete. What I mean by that is, when a resource is being loaded, check to see if it's been imported, but if it hasn't, and the first scan isn't complete, then try to import it. I don't believe this would cause any more slowdown than the approach currently here, since all assets used by editor plugins would have to be imported first anyways.

@TechnoPorg
Copy link
Contributor Author

Since this doesn't seem to be a great solution, I'm going to close my pull request. If there's anything different I can do in the future to help fix the bug, please let me know!

@TechnoPorg TechnoPorg closed this Sep 20, 2022
@TechnoPorg TechnoPorg deleted the import-assets-first-but-not-broken branch October 10, 2022 02:27
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.

Assets should be imported before running plugins and tool scripts
4 participants