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 PluginUITexture import option for images #36417

Closed

Conversation

RandomShaper
Copy link
Member

When you write a plugin requiring some textures for its in-editor UI, you face the problem that they need to be imported. That's a problem especially if your plugin needs to be able to load them from the very beginning (because of preloads or because it needs to display some UI as soon as it's activated).

Normally, textures in Godot are imported so they are converted to proper formats depending on the import kind and settings, and that's good. However, in this scenario there's no ambiguity: you need to plug some textures into some Controls and you just need them to work.

To meet that need, this PR adds a new import mode for images: PluginUITexture. What it does is importing the image as an ImageTexture with sensible flags for UI use (filter and mip-maps). This mode will have the original image loaded instead of any import "artifact" that is normally generated by other modes. For that, this mode is flagged as dummy.

(The mode could have been named ImageTexture, but by giving it the name of PluginUITexture it's only reasonable use case is made explicit and confusion among the other import modes is avoided.)

Finally, it's only enabled in tools builds. If a texture imported this way is attempted to be loaded in an exported game, a no-loader-found-for-this-resource error will be thrown.


This code is generously donated by IMVU.

@RandomShaper RandomShaper added enhancement topic:editor topic:plugin cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Feb 21, 2020
@RandomShaper RandomShaper added this to the 4.0 milestone Feb 21, 2020
@reduz
Copy link
Member

reduz commented Feb 21, 2020

to be honest, Its probably better to just make sure textures are imported before the plugin is loaded.

@RandomShaper RandomShaper changed the title Add PluginUITexture import option for images Add PluginUITexture import option for images Feb 21, 2020
@RandomShaper
Copy link
Member Author

That may not be easy in every situation. For instance, if some script in the plugin is loaded as a singleton that preloads some scene needing a texture for UI.

I think this change adds a lot of benefit at a very little code cost. When you see it running and you can use textures without importing them it looks super clean. For plugins containing no assets, those that have only an editor side, this makes them plug & play.

@reduz
Copy link
Member

reduz commented Feb 21, 2020

This is why the editor has code to register plugins, you can put your singleton code in there. IMO this is a problem with a badly written plugin, more than a required feature. I am against adding this kind f hack.

@reduz
Copy link
Member

reduz commented Feb 21, 2020

I think its probably easier to do have a function that does a yield until the import process is complete, so you can do whathever you want there.

@reduz
Copy link
Member

reduz commented Feb 21, 2020

think about it, if we start adding this sort of hacks for this, then many other users will start asking for hacks f or other types of resources. Lets find a proper way to do it and document it.

@ghost
Copy link

ghost commented Feb 23, 2020

IMO this is a problem with a badly written plugin

Maybe it's unrelated here, but as it is now, the first time a plugin loads it will fail to load textures.
#17483

It has to be restarted, because it seems like the import and the subsequent file it creates happens after the registration of the plugin.

I get this all the time when cloning my repositories. The first run spews errors. This is with a basic plugin that is simply using add_custom_type() and has a texture provided for an icon.

If a fix is to be had, I would also prefer it to just work without any special additional steps, like configuring dozens of texture imports with clicking and typing about the UI.

@RandomShaper
Copy link
Member Author

Maybe in a future I'll come back with something different, but in the meantime I'm just closing this PR as not of enough general interest.

@RandomShaper RandomShaper deleted the imvu/plugin_ui_texture branch February 25, 2020 19:01
@akien-mga akien-mga added archived and removed cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Feb 25, 2020
@follower
Copy link
Contributor

follower commented Jun 17, 2020

FYI: Thanks to @avencherus pointing out #39313 (comment) which suggests a potential workaround for at least some cases (specifically using a texture atlas but maybe even image texture resource might be sufficient).


TL;DR: Editor plugins shouldn't attempt to use the resource system during initialization despite documentation stating otherwise.

Suggested workarounds:

  • Delay resource loading in an editor plugin until it's known that "first reimport" has occurred. (It's currently unclear when exactly that is--but it's some point after a project scene has been opened.)

  • Avoid using the resource system at all and rely on direct loading of assets via e.g. ImageLoader from a plugin subdirectory protected by .gdignore. (Only works for purely editor-only plugins and maybe add_custom_type() icons.) [To be confirmed.]


Additional context

Some of this is already covered in my existing #17483 (comment) but here's some extra specifics...

The code around this PR is a useful pointer to the underlying issue. Specifically, the code here which enables the plugins...:

case NOTIFICATION_READY: {
{
_initializing_addons = true;
Vector<String> addons;
if (ProjectSettings::get_singleton()->has_setting("editor_plugins/enabled")) {
addons = ProjectSettings::get_singleton()->get("editor_plugins/enabled");
}
for (int i = 0; i < addons.size(); i++) {
set_addon_plugin_enabled(addons[i], true);
}
_initializing_addons = false;
}

...occurs before this comment (which appears in a number of places):

/* DO NOT LOAD SCENES HERE, WAIT FOR FILE SCANNING AND REIMPORT TO COMPLETE */

Thus any plugin initialization code cannot rely on the resource system or it will break on the first load of a project.

(Aside: I vaguely seem to recall there's a related issue I noticed with first import/loads from the Project Manager or Asset Library templates due to the same issue?)

Waiting for resource reimport

The "waiting" *cough* mentioned in the code comment above is related to the state of these...:

godot/editor/editor_node.h

Lines 390 to 392 in 02f7908

bool waiting_for_first_scan;
bool waiting_for_sources_changed;

...which affect the operation of the following methods (I don't remember the full details but I think there's a xxxx_changed signal (via a filesystem scan?) that is emitted which then triggers the reimport):

void EditorNode::_sources_changed(bool p_exist) {
if (waiting_for_first_scan) {
waiting_for_first_scan = false;
// Start preview thread now that it's safe.
if (!singleton->cmdline_export_mode) {
EditorResourcePreview::get_singleton()->start();
}
_load_docks();
if (defer_load_scene != "") {
load_scene(defer_load_scene);
defer_load_scene = "";
}
}
}

void EditorNode::_resources_reimported(const Vector<String> &p_resources) {
List<String> scenes; //will load later
int current_tab = scene_tabs->get_current_tab();
for (int i = 0; i < p_resources.size(); i++) {
String file_type = ResourceLoader::get_resource_type(p_resources[i]);
if (file_type == "PackedScene") {
scenes.push_back(p_resources[i]);
//reload later if needed, first go with normal resources
continue;
}
if (!ResourceCache::has(p_resources[i])) {
continue; //not loaded, no need to reload
}
//reload normally
Resource *resource = ResourceCache::get(p_resources[i]);
if (resource) {
resource->reload_from_file();
}
}
for (List<String>::Element *E = scenes.front(); E; E = E->next()) {
reload_scene(E->get());
}
scene_tabs->set_current_tab(current_tab);
}

So, basically, if a plugin relies on the resource loading system (during plugin initialisation, I guess) it'll be broken on first load because the resource "reimport" (i.e. first import) won't have happened.

Given that the current custom plugin documentation advises to use preload() this means anyone who follows it will have a broken first load.

When should editor plugins first attempt to use the resource system?

As I think on it, maybe part of the issue is that this advice in the documentation is incorrect when it comes to editor plugin initialization:

It’s important to deal with initialization and clean-up of resources. A good practice is to use the virtual function _enter_tree() to initialize your plugin and _exit_tree() to clean it up. Thankfully, the dialog generates these callbacks for you.

If first reimport doesn't happen until after plugin initialization then _enter_tree() isn't where resource-related initialization should occurs. (I think because the "tree" in this case is the editor tree not a scene tree.)

Obviously the resources need to get reimported before e.g. a dialog is first displayed, so any resource-related initialization should occur in a hook that's called after reimport completes.

A complication originating from per-project plugins?

Actually, that makes me think this is somewhat related to a potential design/implementation issue due to conflating the editor scene tree with the project scene tree--since addons are per-project not editor.

An existing solution already applied elsewhere

While re-researching all this I discovered 9131f70 which (maybe unintentionally?) fixes the equivalent issue for custom class icons by changing from using ResourceLoader:

godot/editor/editor_node.cpp

Lines 3694 to 3697 in 4689ece

if (FileAccess::exists(icon_path)) {
icon = ResourceLoader::load(icon_path);
if (icon.is_valid())
return icon;

to using ImageLoader:

Error err = ImageLoader::load_image(p_path, img);

Which obviously bypasses the delayed first reimport entirely.

But class_name is documented to take a string with the icon path whereas add_custom_type() takes a texture which implies use of load() or preload() for example.

An additional import related wrinkle...

I've also just realised that there's any additional issue which I mentioned in my earlier comment that tried to go down the ImageLoader route from GDScript:

(In addition, in the above patch I'm removing a more complex workaround which used Image.load() & ImageTexture.create_from_image() which results in icons always loading correctly but also generates a yellow warning about accessing an image as a resource. I think the warning might be possible to be avoided by putting the image in a directory excluded with .gdignore but by that stage I'd shaved so many yaks that I stopped before testing that.)

Inconclusion...

So, depending on where one sits, this could be an engine bug, a design bug or a documentation bug...or some combination of all three. :)


Edit: Previous issues semi-related to the issue of timing of resource imports:


{ This probably would've been better in #17483 so I'll at least put the TL;DR: there too. }

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.

None yet

4 participants