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

Added basic support for custom resource savers and loaders #19501

Merged
merged 1 commit into from
Dec 16, 2018

Conversation

Zylann
Copy link
Contributor

@Zylann Zylann commented Jun 11, 2018

Adds support for custom loaders and savers, so you can add new file types for Godot to load and save using ResourceLoader and ResourceSaver.

Jump here for updated proposal --> #19501 (comment)


Old proposal:

It basically exposes an internal API which was however unfit for the scripting API, but changing this core API could be more work, so a compromise is taken:
Custom loaders and savers are nodes, which you can add as autoloads.
Plugins defining new filetypes can then add those autoloads automatically when the get activated and deactivated (however, see #19499).

Notes:

  • I only exposed the bare minimum, there are more methods that could be exposed but I either don't understand them or maybe they aren't needed for now. They can be exposed in the future.
  • I used multiple inheritance, but it should be OK because one of the classes is an interface (it doesn't contain data and has only virtual methods).
  • When you save a custom resource inheriting Resource, Godot still defaults to .tres in the save dialog, so you have to select the custom extension manually. I would like to make it default to the custom extension (or even remove the others), but I couldn't find how.
  • If I use remove_autoload_singleton when my plugin exits the tree, Godot now crashes everytime on exit with errors about the scene tree. I think EditorPlugin needs an additional function to know when it's disabled or enabled, as opposed to just being added or removed from the tree, because there is no clean way to differentiate this from an editor launch or exit (can't use is_plugin_enabled because it isn't updated in time) @vnen
  • Here is the test project I used:
    CustomResourceLoader.zip


#include "scene/main/node.h"

class CustomResourceLoader : public Node, public ResourceFormatLoader {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple inheritance fucks up the godot cast system on some platforms due to pointer adjustment, so it cant be used. Maybe instead, the system should be modified for ResourceFormatLoader and ResourceFormatSaver to inherit Reference

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more in detail, using multiple inheritance breaks godot type system when disabling rtti

Copy link
Contributor Author

@Zylann Zylann Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can implement with composition then. The thing is, I'd really like to have this feature in 3.1 because it makes my plugin really hard to use.
If we do the Reference change, how will they be registered if we don't make them autoloads?

@Zylann
Copy link
Contributor Author

Zylann commented Jun 22, 2018

I worked on making ResourceFormatLoader a Reference, and added a way to register loaders and savers into the project settings through an EditorPlugin API (not in the PR yet, but I can push a WIP if you want). However, I still had the following problems:

  • A plugin needs to know when it was enabled or disabled, because currently _exit_tree and _enter_tree are ambiguous. For example, if you want to cleanup an autoload when the plugin gets disabled by using _exit_tree, it would prevent it from staying in ProjectSettings when the user just closes the editor. So I added enable_plugin and disable_plugin to do those registrations.

  • I need to store the list of custom loaders and savers into a setting in ProjectSettings. I've been trying to save it to `project.godot, which seemed to work, however Godot doesn't find the setting when I load either the editor or the game. What do I have to do for this setting to be recognized?


I also thought about some alternatives:

  • Promoting autoload to not be limited to nodes. This would allow singletons like custom loaders to exist using the same API. However, it still requires plugin makers to write [boilerplate] code to register them, and the editor has to run this code at the right time. Which leads me to another idea below...

  • Instead of requiring the plugin maker to implement callbacks to register their autoloads and loaders with code, we could have a .cfg next to the plugin that Godot will parse (in editor or in game) in addition to project settings, and process them as if they were part of ProjectSettings. Easier to use, and also doesn't requires the editor to run for activation, it's plug-and-play on filesystem level.

  • If not any of the above, what I could do would be to not have extra settings in ProjectSettings, and use a single autoload node (still registered by the plugin) whose sole purpose will be to register/cleanup loaders and savers using _enter_tree and _exit_tree. That would need to happen before the game (or the editor) loads any scene though, and will be subject to ordering if other autoloads need to use the loader/saver.

I'd need some feedback on which approach I should take, since I've spent a few nights on this already, I don't want to spend too much time in the wrong direction ;)

@reduz
Copy link
Member

reduz commented Jun 23, 2018

Promoting autoload to not be limited to nodes. This would allow singletons like custom loaders to exist using the same API. However, it still requires plugin makers to write [boilerplate] code to register them, and the editor has to run this code at the right time. Which leads me to another idea below...

format savers and loaders could be just another thing different that autoload, should not be a problem.

Instead of requiring the plugin maker to implement callbacks to register their autoloads and loaders with code, we could have a .cfg next to the plugin that Godot will parse (in editor or in game)

In game this is not possible, because Godot will never do project filesystem scanning on startup, this does need to be registered in the editor at some point, and I think when plugin is disabled/enabled is probably the best time.

@Zylann
Copy link
Contributor Author

Zylann commented Jun 23, 2018

In game this is not possible, because Godot will never do project filesystem scanning on startup

Godot does not need to do that because the addons folder is fixed and obeys to specific rules (res://addons/<addon_name>/file.cfg), so scanning would be very limited to a single folder and only on the referenced addons (so no scanning actually).

format savers and loaders could be just another thing different that autoload, should not be a problem.

Ok, but how do I make the setting recognized? What I tried so far didn't work, ProjectSettings says no to me.
Should I also implement a UI for users to add them manually etc?

@vnen
Copy link
Member

vnen commented Jun 24, 2018

I've been trying to save it to `project.godot, which seemed to work, however Godot doesn't find the setting when I load either the editor or the game. What do I have to do for this setting to be recognized?

I think it should "just work", but maybe there's some bug?. Are you adding using the ProjectSettings API or manually with a text editor?

It could just be stored in project.godot like the autoloads and be registered in main.cpp (at least in game). Since it's a file loaded early anyway, you wouldn't need to export more files, convert them to binary on export, and search the addons folder on startup.

@Zylann
Copy link
Contributor Author

Zylann commented Jun 24, 2018

@vnen I'm adding the settings with ProjectSettings::set("loaders", loaders);, and they do get saved by Godot. However I get nothing when I load back the project (ProjectSettings::get_singleton()->has_setting("loaders") return false)

@Zylann
Copy link
Contributor Author

Zylann commented Jun 25, 2018

I updated the PR with the new approach:

  • ResourceFormatLoader and ResourceFormatSaver are now references. I left the existing logic, replacing it with references. Let me know if more should be done.
  • It's now possible to remove loaders and savers, though in practice only custom ones will do this
  • Added enable_plugin and disable_plugin so plugins can register loaders or autoloads properly (see above conversastion about why)
  • ProjectSettings can now have a list of enabled custom loaders and savers, under loaders/enabled and savers/enabled, which are PoolStringArrays containing paths to their scripts.
  • Added CustomResourceLoader and CustomResourceSaver which you can inherit with a script to define new loaders and savers. Register them in project settings using new functions of EditorPlugin.
  • Custom loaders and savers get registered and unregistered from main.cpp, and should work both in editor and in game.

Tested so far using this project:
CustomResourceLoader.zip
And also tested in my terrain plugin.

Extra notes:

  • Because the current approach involves running plugin code, there is a small drawback: the only way for a plugin to make its loaders, savers and autoloads available is for the user to go through the project settings and click Enable. Conversely, the only way for plugins to clean them up properly is to also go through the UI and click Disable. Other situations will not work, such as:
    • Deleting the plugin from file browser without disabling it first
    • Overwriting the plugin with a new version of it which might have new singletons / might have removed them
  • GDNative had a way to define custom loaders and savers too through a GDNative-only API. This PR adds the feature for anything that can access the script API, so it's kindof two ways of doing the same thing now.
  • clang-format complains, but when I run it, nothing changes

@vnen it looks like adding the project setting within a category made it work. Somehow it didn't when it was outside.

@Zylann Zylann force-pushed the custom_loaders branch 5 times, most recently from 7f46c17 to b47e6d8 Compare June 26, 2018 01:35
@mhilbrunner
Copy link
Member

Formatting needs to be fixed :)

@Zylann
Copy link
Contributor Author

Zylann commented Jul 17, 2018

@mhilbrunner but when I run clang-format on register_scene_types.cpp, nothing changes :(
...
I think that's because the order of #includes makes the formatter unhappy AFTER it gets merged... my local copy is fine...

@vnen
Copy link
Member

vnen commented Jul 17, 2018

@Zylann depending on the clang-format version it gives different results. It's simpler to apply the patch that Travis gives just to make it pass.

@Zylann
Copy link
Contributor Author

Zylann commented Jul 17, 2018

@vnen see, just rebased and the errors went away :)

@Zylann
Copy link
Contributor Author

Zylann commented Jul 26, 2018

Rebased again and updated.

@vnen
Copy link
Member

vnen commented Jul 27, 2018

[...] given we now have named classes, we could take advantage of this system and get those added directly and automatically by checking inheritance from ResourceFormatLoader and ResourceFormatSaver.. so registration can be completely transparent.

That might be simpler, but also (as @Zylann said) it's not possible to disable the loaders/savers (though it might not be really needed). The major issue for plugins is that named classes have no namespace, so they have to use some unique naming convention.

I thought you didn't want a full-project parse at some point (or editor-only?)

This would happen only in editor. The inheritance of named script classes are already checked and saved in the project.godot file. There's not much difference in checking a list of registered loaders/savers and checking the list of script classes.

@Zylann
Copy link
Contributor Author

Zylann commented Jul 27, 2018

I like the fact it would reduce the registration boilerplate, but what should I remove from this PR, and how should we do registration?

Waiting for @reduz to have a look since he appears to have an idea how to complete this PR

@Zylann
Copy link
Contributor Author

Zylann commented Dec 1, 2018

I updated the PR:

  • Now registration is done through global class names (class_name in GDScript)
  • Tested with the following project: CustomLoaders.zip
  • I'm not sure if I should keep CustomResourceLoader and saver rather than just implement this in the common base class?

@Zylann Zylann force-pushed the custom_loaders branch 2 times, most recently from 97e3413 to 5892bb5 Compare December 2, 2018 02:26
@akien-mga
Copy link
Member

Moving to 3.2 milestone as we're about to enter the beta stage and release freeze for Godot 3.1. Only bug fixes against issues of the 3.1 milestone (or enhancements to core features of the 3.1 roadmap) will be considered until 3.1-stable is released.

Note: If it's merged after 3.1-stable is released, this change might be considered for cherry-picking into a later 3.1.x release.

@akien-mga akien-mga modified the milestones: 3.1, 3.2 Dec 12, 2018
@@ -0,0 +1,70 @@
#include "custom_resource_loader.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have the Godot copyright header (with the filename on line 2, see other existing files). Same with other added files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed those files now.

@Zylann
Copy link
Contributor Author

Zylann commented Dec 15, 2018

I removed CustomResourceLoader and CustomResourceSaver, and added their functionality directly in ResourceFormatLoader and ResourceFormatSaver.
I'm writing docs for them now.

@Zylann
Copy link
Contributor Author

Zylann commented Dec 15, 2018

Docs written for ResourceFormatLoader and ResourceFormatSaver. The doctool also generated doc files for all the loader and savers in the engine, although with empty contents.

@akien-mga akien-mga modified the milestones: 3.2, 3.1 Dec 16, 2018
@akien-mga akien-mga merged commit 9df7ed5 into godotengine:master Dec 16, 2018
@akien-mga
Copy link
Member

Thanks!

akien-mga added a commit to akien-mga/godot that referenced this pull request Jan 10, 2019
Some used 'is_valid()' checks, others not. Validity is already checked in 'unref()',
and 'remove_resource_format_*()' has an ERR_FAIL condition on 'is_null()' already
(which shouldn't happen since we're only unregistering things that we previously
registered.

Also add missing GDCLASS statement in ResourceFormatLoaderVideoStreamGDNative,
missed in godotengine#20552 which was last amended before godotengine#19501 was merged.
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.

5 participants