Implement glTF compatibility system for files imported in older Godot versions

[aaronfranke]

This PR is an alternative to #84034. The code is completely different, so I'm opening a new PR.

Fixes #83429, caused by a behavior change from #80270 (the old behavior is wrong, but we must keep it for compat), and also fixes the behavior change from #81264.

I think the easiest way to explain how this works is by giving examples. Comprehensive test project: Block.zip All .glb files in this project are the exact same, just with different file names and .import files.

The "4.2" folder has .import files generated with this PR, with the glTF compatibility version set to 1.

The "4.1" folder has .import files generated in Godot 4.1. The expected behavior is that they import the same way as they did in 4.1, when the scene name (usually file name) took priority over the node name. The files in this folder do not have a gltf/compatibility_version= field, and are assigned version 0 when opened with this PR.

The NoImportFile folder does not have .import files for the .glb files. This is the case when a user brings in a new model, or wants to reset the settings by deleting the .import files. This should generate new files with the glTF compatibility version set to 1, and then import the same as the files in the 4.2 folder.

If desired, we could expose the methods to set the importer version to allow users to manually import files with an older importer version (ex: when using GLTFDocument at runtime), but the use case here is not entirely clear (modifying a file after import, when we care about preserving the exact same data, is mostly relevant in the editor).

The default value of the compatibility version is 1 in this PR (it can be incremented later), and this value is also used when generating a new .import file. However, existing files are assigned version 0.

A new method handle_compatibility_options has been added to allow importers to control what happens when loading existing files. We need to call something from EditorFileSystem to disambiguate the case of a file without a version (that should use the oldest version) from a missing file (that should use the default version).

[fire]

I have a preference for the local solution and @lyuma mentioned that too. So we need to review this pr.

[akien-mga]

This is quite hacky. GLTF specific code shouldn't spill into EditorFileSystem like this.

Why is it needed? Can't you just assign compat_version = 0 in editor_scene_importer_gltf.cpp if the option is missing?

[aaronfranke]

No, because there is no way to distinguish the case of a file without a version (that should use the oldest version) from a missing file (that should use the default version).

We could just always default to the latest version, but that won't automatically fix files from Godot 4.1.

[akien-mga]

Ok, I just had a quick look and indeed EditorFileSystem includes the default parameters in case they're missing:

	//mix with default params, in case a parameter is missing

	List<ResourceImporter::ImportOption> opts;
	importer->get_import_options(p_file, &opts);
	for (const ResourceImporter::ImportOption &E : opts) {
		if (!params.has(E.option.name)) { //this one is not present
			params[E.option.name] = E.default_value;
		}
	}

IMO that disqualifies this approach if we can't differentiate between new files with default settings, and old files missing a new setting. Adding a hack to EditorFileSystem to workaround this design constraint is not the right approach.

[aaronfranke]

EditorFileSystem is the only place we can distinguish these cases, so if we can't set the glTF setting here, then we can't do it anywhere else. By the time the import process gets to the glTF code, it is already too late. We would need to completely rethink how this code works (such as by allowing a different set of default values for existing files vs new files).

[lyuma]

What aaron did is similar to the code we wrote in 3.x (hope this doesn't bring up any stressful memories for Akien) d92a172
Since this was kind of a last minute change there, it didn't get proper design review, so it shouldn't really justify the approach.... in my mind editor_file_system could be the correct place to override defaults for newly imported files, but ideally an API would be added so each importer can assign its own defaults.

Aha!!! I remember now. The place it should be done is the project setting Import Defaults, since those shouldn't apply retroactively but instead should override only newly imported files, right?

[lyuma]

I'm not suggesting to add new APIs in this release: I'm suggesting it as future work in 4.3+ to permit cleaning up the importer-specific data being added to editor_file_system.cpp by making an api call instead to ask for defaults (even if this requires going through project settings to change import defaults)

[reduz]

I would add a new API to ResourceIimporter like this:
void ResourceImporter::handle_compatibility(Ref<ConfigFile> p_config_file) (pased like this, no const ref, so you can get it modified).
which does nothing by default
but in the scene importer you propagate all the way to EditorSceneFormatImporterGLTF::handle_compatibility
where you can do all the hacking you want.

[lyuma]

I think this is the best of the proposed approaches, and so I'll tentatively approve this because the code looks fine in a technical sense.

So the question really comes down to if this change looks ok from a maintainability standpoint for at least the 4.2.x versions.

I think the rough edges related to gltf constants in editor_file_system.cpp here can be improved in the future in 4.3 (or even 4.2.1) by adding some virtual function that the gltf plugin can override.

[YuriSizov]

In my humble opinion, we should not ignore the existing versioning system for importers. Even if you don't want to rely on it for this particular fix, we must respect that it exists and it signifies different revisions of each importer. So all resource importers that can create GLTF documents must have their version bumped.

ResourceImporterScene needs to have its version bumped to 2. If any other resource importer creates GLTF documents, its version must also be increased. The logic they abstract is made different in 4.2, the output of those importers is different. So their version must reflect that.

[aaronfranke]

@YuriSizov In that case, this is what #84034 accomplishes. I am good with either solution.

[YuriSizov]

I mean, even if we go with this PR, we need to increment the version to signify the changes. We don't need to use it if the import team doesn't like it, but we should still increment what get_format_version returns in the relevant importers.

[reduz]

Great work with this! I left some feedback.

[reduz]

I would add a new API to ResourceIimporter like this:
void ResourceImporter::handle_compatibility(Ref<ConfigFile> p_config_file) (pased like this, no const ref, so you can get it modified).
which does nothing by default
but in the scene importer you propagate all the way to EditorSceneFormatImporterGLTF::handle_compatibility
where you can do all the hacking you want.

[reduz]

To be completely honest, I would not call it compatibility_version but simply some flag such as "use_compatibility_naming". Because you may want to have new features in the GLTF2 supported at some point while users would not want to miss on those ones, while keeping this compatibility setting on.

[lyuma]

use_compatibility_naming makes it sound like a boolean, but we might want future changes to naming which would lead to needing yet another boolean. In fact, aaron has some open PRs which might also impact node naming but were put on hold due to this exact problem.

on the other hand, if we can solve the gltf node id problem before then then name compatibility might not be as big a problem in the future and a boolean would be sufficient

[reduz]

if you want to do this its fine, but then I think it should be called naming_scheme_version or something like that, not be a general GLTF version thing.

[YuriSizov]

I really don't understand why we complicate it so much. We have one user-facing interface: the importer. Whatever it does, it produces a different output in Godot 4.2 compared to Godot 4.1. That's all that really matters, one version increment.

This only needs to exist to allow a smooth transition to 4.2. If users want to reimport their assets in 4.2 and use new features from 4.2+, then it's fine if we fully upgrade them to use the most up to date format. Users will have to update their affected scenes, but it will be clear there and then, because they are manually reimporting those assets.

We don't need to support individual changes so granularly, it's not going to scale for years to come. And there is no reason why someone would want to have naming scheme 4 with something else version 2 with something else 10 in Godot 4.8. It should only exist to support legacy settings during the transition, and then it should be up to user to accept the need to migrate or not use any new features.

And we don't have to think about all the permutations.

[aaronfranke]

I prefer compatibility_version, because what matters here is like Yuri said, providing compatibility for older Godot versions. There is no reason to use the names from 4.1 and the "something else" from 4.8 together, so I don't think it's a benefit to have a separate setting for names vs other things.

[reduz]

@aaronfranke the problem in this case, is that conceptually, this only really matters for names, so I think doing futurology on what may change that may require tweaking this again, and that it does in a way where you still care about the names to me is going into speculative territory. This is why I would keep this option strictly for naming, which is what we know may change in the future and should not affect anything else, hence why I suggest naming_scheme_version instead and do the dedicated solution to the problem you know you have instead.

[YuriSizov]

The problem that we have is that the output of the importer has changed. The specifics aren't really important to that outcome. There is no practical need to have the granularity that you suggest, and nobody asked for it. It's speculative to assume that people would need to change back and forth naming scheme specifically. All users need right now is for their existing scenes to open in Godot 4.2 without issues and crashes.

[aaronfranke]

I updated this PR to add ResourceImporter::handle_compatibility_options like @reduz suggested, and I like this version a lot.

[reduz]

I think this one is the way to go, but I stand that we should rename compatibility_version to naming_scheme_version. I do not see this very specific compatibility behavior for anything else in the future and exposing this in the GLTF API and import file with a generic name makes it harder to understand what it exactly is for.

[reduz]

Its great now to me!

[YuriSizov]

Thanks!