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

Expose ResourceImporter::get_format_version() in the GDExtension API for EditorImportPlugins #10658

Open
HiddenJesse opened this issue Sep 5, 2024 · 8 comments

Comments

@HiddenJesse
Copy link

HiddenJesse commented Sep 5, 2024

Describe the project you are working on

In our projects, we have an animation and palettization system that is presently relying heavily on custom EditorImportPlugins to process png and ascii files into Godot internal data formats and structures.

Describe the problem or limitation you are having in your project

As we're performing active development on these importers and the file formats they support, we're running into an issue where we need all files imported by a given EditorImportPlugin to be reimported because of changes in how data is processed. For example: If I improve our palettization algorithm, then all the resulting palettized images need to be reimported to include the improvements.

Right now, I have to manually reimport all the files, which is not going to be tenable for long term development.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

The improvement I would propose is exposing an override of ResourceImporter::get_format_version() in the GDExtension public API as something that came be overridden by EditorImportPlugins. If that format version function was exposed, we could make use of functionality that already exists in editor_file_system.cpp.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

The implementation would just need to follow the paradigm set by things like EditorImportPlugin::get_importer_name() or EditorImportPlugin::get_priority(). Both of which are functions that are overridden by EditorImportPlugin from ResourceImporter

If this enhancement will not be used often, can it be worked around with a few lines of script?

The alternative to this would be having a GDScript or some other editor plugin redo the work of scanning the resource directory, checking version numbers, and triggering reimports. This would likely have to be triggered by a keyboard shortcut as I don't believe there is any way to hook into the editor's events for when it periodically rescans the resource directory.

Is there a reason why this should be core and not an add-on in the asset library?

This is an augmentation to the core EditorInputPlugin core class and is specifically an issue with the GDExtension system, which I imagine at least some of the asset library add-ons rely on.

It's also just... such a small easy change.

@Calinou Calinou changed the title Expose ResourceImporter::get_format_version() in the GDExtension API for EditorImportPlugins Expose ResourceImporter::get_format_version() in the GDExtension API for EditorImportPlugins Sep 5, 2024
@kromenak
Copy link

kromenak commented Sep 5, 2024

This seems like a somewhat important feature for an asset importer. Imagine that you've imported hundreds of assets, but then the logic for the importer changes. Ideally, you'd want the system to detect this and re-import all the previously imported assets.

The easiest way to achieve that is with a version number on the importer. If an asset was imported with an older version of the importer, it should be re-imported.

Since it appears that the functionality already exists in the engine, but just isn't exposed to importer plugins, I think this would be super valuable to do!

@dsnopek
Copy link

dsnopek commented Sep 6, 2024

I have no objections to this from the GDExtension side. It should be fairly easy to add - just a couple lines to EditorImportPlugin setting up a new _get_format_version virtual function.

However, I don't personally know much about the import system. The @godotengine/import team will need to speak whether or not it makes sense to expose this.

@fire
Copy link
Member

fire commented Sep 7, 2024

A brief answer is the import system reimports based on a hash of resource path to a godot internal format like .res or .scn.

Also the designs to add a full version database has been vetoed. #5945

Let me get back to you.

@HiddenJesse
Copy link
Author

Original poster hoping to contribute to the conversation (and advocate further for the feature):

To reiterate something for the original post, my impression is that the modification of the GDExtension API is the only thing that needs to happen. That the rest of the system is already setup to work correctly within the core of Godot's logic.

If a ResourceImporter, like an EditorImportPlugin, provide a get_format_version() with a non-zero result, my understanding is the following would happen.

On Import Success:

  • The version number of an importer is pulled using ResourceImporter::get_format_version()
  • The version number is written into the .import file. (@ line 2347 in EditorFileSystem on Master)

On EditorFileSystem::_test_for_reimport

  • The .import file is opened and parsed using VariantParser::parse_tag_assign_eof (@ line 505 in EditorFileSystem on Master)
  • The version number from the file is read and stored in a local variable
  • The version number is then checked against the version number returned by ResourceImporter::get_format_version() to check if a reimport is needed (@ line 565)

The only situation where this would break down is if there was no .import file, but I think for EditorImportPlugins, there will always be a .import file, since it also stores the the "importer=" data that lets the Godot engine remember what importer was used for a file in the first place.

So yeah... it's all there, and it seems like the internal systems were designed for situations of shifting format and import rules in mind. It just needs to be exposed through the GDExtension API

@fire
Copy link
Member

fire commented Sep 7, 2024

If I remember correctly this is affects the priority of the importer. Like if you have png1_importer and png2_importer you'll be able to set priority.

If you want to add a new importer like png3_importer, png4_importer, png5_importer, ..., every time you want to rescan the entire filesystem for an upgrade, this proposal should do it.

any way to hook into the editor's events for when it periodically rescans the resource directory.

However I would probably use https://docs.godotengine.org/en/stable/classes/class_editorfilesystem.html#class-editorfilesystem-method-update-file

Edited: Anyways this should be exposed, but exposing get_format_version() doesn't solve your problem of given a particular format version force update the entire project.

@HiddenJesse
Copy link
Author

HiddenJesse commented Sep 7, 2024

To clarify, I'm not adding a bunch of new importers. I understand that adding a "new" importer would have us manually assign any files to the new importer, and that is a design decision I entirely agree with.

We're more "updating" an existing importer. Specifically, to make use of your example, improving the algorithm behind png2_importer to get better results. It's still png2_importer, it's just now it handles XYZ situation better.

But still, for your edited addition, yay for thinking it should be exposed! Thank you for your vote of agreement!

@dsnopek
Copy link

dsnopek commented Sep 10, 2024

Given how easy this is to add, I threw together PR godotengine/godot#96802 which exposes this, but I haven't tested it.

Please give it a try and let me know if it works for you!

@HiddenJesse
Copy link
Author

@dsnopek Happy to! I'll try to give it all a test run before EOD.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants