Convert 3.x ESCN skeletons, animations, and shaders upon import

[nikitalita]

Depends on #88009, #88078

The Problem

ESCN was a discrete scene format that was created by the Godot team for ease-of-export from Blender to Godot. The plugin was originally written for Godot 3.x and used its scene format, and has not since been updated.

However, while Godot still has support for importing this format, ESCN importing has been broken since Godot 4.0 was released due significant changes in the scene format that conversion was not implemented for:

• Skeletons had a Transform pose property which was relative to bone rest in 3.x , which was broken out to Vector3 pose_position; Quaternion pose_rotation; Vector3 pose_scale that are absolute in 4.x.
• Animations used to have a transform track type with transforms relative to bone rest in 3.x, which was similarly broken out into position_3d, rotation_3d, and scale types that are absolute in 4.x
• The shaders that were included in the ESCN export were of the 3.x format, which is not compatible with 4.x.

Solution

This adds the following features to the ESCN editor importer:

1. Converts 3.x Skeletons to use the new pose properties and recomputes them to be absolute rather than relative to the bone rest.
2. Converts 3.x Animations with transform tracks into position, rotation, and scale tracks that are absolute rather than relative to the bone rest.
3. Converts old shader code to 4.x-compliant shader code

Design Considerations

The reason this is a draft right now is because of the following issues that I need advice on. This will likely be broken up into seperate PRs based on the feedback.

ResourceLoader modification to allow special handlers for a single load

I had issues with converting Animations and Shaders; basically, we can't handle converting them in either of the _set() functions for these classes like we can for Skeleton3D because:

• Animation tracks are stored ini style:

  

  This means that, upon load, each of those indexed properties gets set, one at a time. We need to split the transformation track up into position, rotation, and scale tracks, and we can't arbitrarily add tracks in the middle of a load, since the index of the tracks will change.

• Shaders compile their code upon setting the code property; if the code fails to compile, code won't be set. Additionally, it will result in a very, very long error message that will obliterate the error logs and significantly confuse the user.

• Both of those class names are the same from 3.x to 4.x, so I couldn't implement another class for either of those to handle the loading either.

So, to solve these issues, I modified the ResourceLoader to allow the addition of special handlers. Basically, we can add a handler for a specified Resource class that takes in a MissingResource and returns a Resource. If we try to load a resource, and the resource loader detects that it has a handler for this resource class, it will instead load the resource as a MissingResource and, after the resource and its properties are finished loading, it will hand it off to the handler to instantiate the resource and set its properties.

I had considered modifying the ResourceLoader classes to just add the property to missing_resource_properties if it fails to be set and then just getting those properties out of the metadata, but as mentioned above, this would result in a bunch of error messages (1000+ lines in total length) every time we tried to import an ESCN, which is not ideal.

The way I have it now, I'm not really sure if this is the correct design; this sort of special handling needs to be only for a single load rather than a global handler, so I decided to only add it to the instantiated ResourceLoader classes rather than the static ResourceFormatLoader classes. This breaks the pattern a bit, as it looks like it's intended that the user doesn't use the instantiated ResourceLoader class and instead uses the ResourceFormatLoader class. I'd appreciate some advice on how to implement this such that it can be used for a single load and doesn't break the pattern here.

ShaderLanguage token stream emitting

Converting Shaders from 3.x to 4.x is a bit more complicated than how it's currently handled in the 3.x to 4.x converter. In order to do a proper conversion of a 3.x to 4.x, we need to do the following:

• Replace everything in RenamesMap3To4::shaders_renames
• SCREEN_TEXTURE, DEPTH_TEXTURE, and NORMAL_ROUGHNESS_TEXTURE were removed, so their usage necessitates adding a uniform declaration at the top of the file
• If shader_type is "particles", we need to rename the function void vertex() to void process()
• CLEARCOAT_GLOSS was changed to CLEARCOAT_ROUGHNESS and its usage was inverted (i.e. ascending values now increase the roughness rather than decreasing it). So, we need to invert all usages of CLEARCOAT_GLOSS:
  • invert all lefthand assignments:
    • CLEARCOAT_GLOSS = 5.0 / foo;
      • becomes: CLEARCOAT_ROUGHNESS = (1.0 - (5.0 / foo));,
    • CLEARCOAT_GLOSS *= 1.1;
      • becomes CLEARCOAT_ROUGHNESS = (1.0 - ((1.0 - CLEARCOAT_ROUGHNESS) * 1.1));
  • invert all righthand usages
    • foo = CLEARCOAT_GLOSS;
      • becomes: foo = (1.0 - CLEARCOAT_ROUGHNESS);
• async_visible and async_hidden were removed, so these render modes need to be removed
• specular_blinn and specular_phong render modes were removed, and we can't handle these, so we just throw an error
• MODULATE is not supported (yet) in 4.x, so we have to throw an error.

As you can see, this is a bit more complicated than a simple regex search and replace would allow for, in particular the CLEARCOAT_GLOSS replacement. We at least need a token stream in order to properly detect for these conditions and modify the code.

So, in order to do this, I modified ShaderLanguage to emit a token stream:

• The function token_debug_stream() was added to emit a parsed token stream for the entire file.
• Token was modified to add position and length members so that we can emit code based on the token stream; this is neccesary primarily because the token parser can modify both identifiers and constant values before creating the token.
• The tokens TK_TAB, TK_CR ,TK_SPACE, TK_NEWLINE, TK_BLOCK_COMMENT, TK_LINE_COMMENT, and TK_PREPROC_DIRECTIVE were added and _get_token() was modified to emit them, but only if we are doing a debug parse. The reason for adding these was so that we can easily emit the new code from the token stream without obliterating the original formatting and comments

With these modifications, we're able to easily detect for the above conditions and insert and remove tokens from the token stream, and then emit code based on that token stream.

I'm a bit more confident about how I modified ShaderLanguage here but I'd still like to get feedback on how I modified ShaderLanguage here, and if there are any other 3.x to 4.x differences that I need to handle that I missed.

[AThousandShips]

These are several unrelated improvements, please make a PR for each feature (you do already have PRs for these open, these changes should be in one PR only unless the code in this depends on it, in that case please keep the details of that in its own PR and mention that this depends on that PR)

[nikitalita]

These are several unrelated improvements, please make a PR for each feature

That's the plan, they're not going to stay in this PR. This is just my working branch so I can get feedback on specific design questions.

[AThousandShips]

Thats not really how you should do things, each issue or feature should be discussed separately, and you shouldn't open a PR for your working branch but work on each feature on one branch unless they're dependent on each other 🙂, this just takes up processing and duplicates things

[nikitalita]

Ok, I have removed the extraneous commits and specified which PRs this depends on.

[lyuma]

I think this adds too much complexity to the import system which is not really the point of the importer.

If the goal is to add back compat with 3.x resources, we should do it one of two ways:

1. expose them with a conversion utility (and perhaps the escn importer could invoke this conversion utility code automatically, but this way it isn't adding complexity to the import system itself). This code will have a maintenance cost to maintain.
2. fix data in the resource loader or the property setters.

My personal opinion is that if this is to be done automatically, it should be done in the resources itself, which you attempted to explain why it wasn't possible, but I think it should be made to be possible. For example, we have discussed a compatibility system. We know which Godot version the resources originated from, so perhaps we can add the missing parts to the resource loader so that we can adapt the properties. These would be a good usecase for such a compatibility system.

Animation tracks are stored ini style:

This means that, upon load, each of those indexed properties gets set, one at a time. We need to split the transformation track up into position, rotation, and scale tracks, and we can't arbitrarily add tracks in the middle of a load, since the index of the tracks will change.

Yes, but you will know how much the index changes: you can store an offset that starts at 0 and increases by 2 for every transform track. In principle, the indices could be offset as the properties are set. It certainly is challenging and may be impossible without some changes to the core resource loader, but I do think we should be willing to look outside of the box rather than adding a lot of complexity to the import system.

Shaders compile their code upon setting the code property; if the code fails to compile, code won't be set. Additionally, it will result in a very, very long error message that will obliterate the error logs and significantly confuse the user.

What if when the code fails to compile, it could attempt to run it through the above converter and use that converted code if it succeeds?

Both of those class names are the same from 3.x to 4.x, so I couldn't implement another class for either of those to handle the loading either.

Yeah I don't think we want to add compatibility code especially at runtime. Load-time or editor-time conversion could be supported only if TOOLS_ENABLED, for example.

Do note also that if we do go this route, we need to make sure mesh compatibility issues are solved .

related issues:

• Skeleton3D: Convert 3.x skeletons #87050 will fix skeletons upgraded on 3.x
• ArrayMeshes converted from Godot 3.x display incorrectly in 4.0 #63550 meshes are broken on upgrade from 3.x - I think this would be a blocker. Do we need to solve this as well?

[nikitalita]

So what we talked about in the rocketchat was that the best course of action for modifying the resource loading to accommodate this would be to instead add void _start_load(int format, bool binary) and void _finish_load(int format, bool binary) virtual methods (that are NOPs by default) to Resource to accommodate this. the binary and text resource loaders would call _start_load() when the sub/main resource is instantiated, before any properties are loaded, and _finish_load() after all the properties have been read and set on the instanced resource.

For Animation, the main problem is that the transform tracks need to be split into seperate tracks, and all the properties for the tracks are set one at a time.

So for Animation, the steps would be:

1. if _start_load() is called, set a flag in the metadata
2. in _set() in Animation.cpp, if the _start_load() flag is set and if we read in a track with a transform type, store it and all of its subsequent properties in the resource metadata
3. once _finish_load() is called, get those tracks out of the metadata, and then split them into position_3d, rotation_3d, and scale_3d tracks, and then clear the metadata of the flag and the transform track data.

This would still entail some steps in the scene importer; we would still have to recompute the animation frames to be absolute rather than relative to the skeleton rest, but we already do plenty of that in the scene importer for other scene types.

For Shader, the main issues are:

1. the code is compiled as soon as the code property is set, which does not allow for fixing it after the fact,
2. failed compilations will spit out 1000+ line error messages
3. We don't want to auto-convert any shaders that aren't actually from Godot 3.x.

For Shader, the steps would be:

1. If _start_load() is called, AND the format value is binary format 3 or text format 2 (i.e. the resource format versions used for all Godot 3.x releases), then we set a flag in the metadata
2. in set_code() in Shader.cpp, if the flag is set:
   1. instead of compiling right away, first run the code through the ShaderLanguage parser (this is done to avoid printing the huge error messages that accompany failed shader compiles)
   2. If the ShaderLanguage parser fails, THEN we do some additional tests to see if it is in fact a 3.x shader (i.e. checking for old identifiers)
   3. If our tests indiciate that it is a 3.x shader, we run it through the code converter
   4. If the code conversion succeeds, then we compile and set the code property.

So, these steps would only be run on Shaders that are embeded in other resources (i.e. would not automatically be run for standalone gdshader files), and would only be run if the resource is in a deprecated format. This will prevent the "Unity shader auto-upgrade" shenanigans that @lyuma expressed their wishes in avoiding.

[fire]

Needs a rebase.

[AThousandShips]

I forgot this one earlier

[Ivorforce]

Can someone explain to me why this is adding compatibility handling code to setters and getters? The get_meta call isn't cheap, so checking it on call might incur a noticeable runtime cost.
I would expect compatibility code to be handled in code where it is conceivable that we might be dealing with old resources (e.g. import), and not in code that could be called on a frame-by-frame basis (e.g. the setters / getters).
cc @lyuma, I'm reading above you have asked specifically to pull the compat code outside the importer?

[nikitalita]

Can someone explain to me why this is adding compatibility handling code to setters and getters? The get_meta call isn't cheap, so checking it on call might incur a noticeable runtime cost.

I can replace it with member variables, for some reason I was gunshy about adding additional members when I originally wrote it

[Ivorforce]

That might be better (especially if done as a bit field).
But by separation of concerns, I would still much prefer if compat handling code was done outside runtime code. Compat code is something we only need to deal with very rarely, so it shouldn't increase the complexity of the runtime code, which we need to fully understand and be prepared to actively maintain.

[nikitalita]

That might be better (especially if done as a bit field). But by separation of concerns, I would still much prefer if compat handling code was done outside runtime code. Compat code is something we only need to deal with very rarely, so it shouldn't increase the complexity of the runtime code, which we need to fully understand and be prepared to actively maintain.

This is how we handle compatibility in nearly all other instances though; for example, look at the ArrayMesh code.

[Ivorforce]

This is how we handle compatibility in nearly all other instances though; for example, look at the ArrayMesh code.

I'm having a look, and yea, I do see a similar pattern. I'm not convinced this is the right approach to compatibility handling, but I haven't been there when that code was written.
I'll think about it a bit and ask other maintainers for their opinions.

[lyuma]

Sorry that I made a mistake with my comment 20 months ago. I genuinely forgot about this PR, and I didn't hear anything for a long time. Given that 4.x has been out a long time at this point, I question the value of introducing conversion for 3.x assets at this point.

Can someone explain to me why this is adding compatibility handling code to setters and getters? The get_meta call isn't cheap, so checking it on call might incur a noticeable runtime cost.

Yeah I wasn't expecting runtime overhead here. My comment was building off the other PR #87050 which was a few lines of conversion code in a standalone property setter, which would not get run except if loading an old asset. If you see how that PR is written, that is the extent to which I expected code to be added to property setters.

Also, I was making the assumption that the property name had changed between 3.x and 4.x and that the conversion could be contained within the property setter... I did not want or expect detection code to be necessary in order to facilitate performing conversions in the property setter.

My expectation was that we would ultimately need some conversion tool, since this mimics the project conversion tool we used for things that changed between 3.x and 4.x.

I feel really bad because this is a huge amount of work, but I did not anticipate thousands of lines of code being added to facilitate shader migration, nor that there would be a flag on animation tracks to convert them at runtime... even if it may be the only option, it's not something I was thinking about when I made that comment.

If we do move forward with this, it might be good to gate any complex conversion code that could have a runtime cost behind TOOLS_ENABLED

If you want to chat more, I'd be happy to talk about options, insofar as I am actually responsible for the areas involved (of course, Animation and Rendering changes would have to be approved by their respective teams)

Also, a lot of the animation and asset maintainers are having a bi-weekly PR review meeting tomorrow in just under 12 hours. The information to join will be in the #animation channel in rocket chat if you happen to be available.

[lyuma]

This is adding code in runtime which I am not happy with.

It's also doing conversions in such a way that we now have an additional relative_to_rest property internally on Animations which we will have to continue to support across the entirety of the Animation system and tooling indefinitely.

Clearly @TokageItLab would need to give the final call on this, but given the complexity the animation system already has, I'm uncertain if this is a good idea. I really don't like that this introduces a strange interaction between the track cache and the Animation keyframes but only if a legacy flag is in use, and one that can never be removed.

When Godot 5.0 comes out, no doubt there will still be Animation resources floating around with this flag that will have the same problem. It also means that the animation track editor might be broken for these animations, since the actual tracks in the resource are rest-relative. How would we even know which Animation's are running this track conversion during every playback, and which aren't?

I understand the problem though... this conversion cannot be done during import because the skeleton rest is not known until runtime, and in fact the Animation resource could be loaded independently of the tscn with the Skeleton3D and only attached later on, so Godot would have no way to adapt the Animation resource for the associated Skeleton3D.

That said, if we have a way of knowing which Skeleton3D corresponds to the given Animation, then I think I would prefer if the escn importer should do the work of converting the legacy animation tracks to not be rest-relative.

[nikitalita]

That said, if we have a way of knowing which Skeleton3D corresponds to the given Animation, then I think I would prefer if the escn importer should do the work of converting the legacy animation tracks to not be rest-relative.

This is originally how I had this setup, but I think I misconstrued your original comment a while back.

While this method has the added benefit of supporting older TSCNs as well, I understand the concerns about runtime complexity. So, would it be enough to simply remove the ADD_PROPERTY macro (and the property declaration in _get_property_list), and move the conversion code from the mixer to animation.cpp and only call it from the escn importer?

[TokageItLab]

I believe adding this as compatibility code is not acceptable. Relative animation was removed in 4.0.

In the past, after discussing with reduz, the alternative provided at that time was _post_process_key_value(), and it is possible to make relative animation work in a custom module using this.

Supporting this would be akin to shipping that custom module in core, but considering the circumstances under which it was provided as a custom module, this would likely be rejected.

Please note that what can be provided as compatibility code is resource conversion, such as converting animation keys from relative to absolute animation. Runtime-based solutions are almost never permitted.

[lyuma]

if we can do the track conversion here using an explicit function call instead of player->advance(0) and remove all the code about caches, then I'd feel a lot less bad about the runtime cost issue.

But it doesn't change that this is bleeding importer complexity into the animation system.

[lyuma]

Unused variable is_3x

It's not my area, but I want to say that given we're already at line 2030, the Shader converter is extremely complex, and I am skeptical if this amount of code is justifiable especially in runtime builds. I did not expect thousands of lines of code to be added to every build of Godot editor and runtime. My guess is this will create too great a maintainability burden to include in the core engine.

The ultimate call here goes to the rendering maintainers since they will shoulder the cost of maintaining this compatibility code.

[nikitalita]

It was originally a lot simpler. The reason for the complexity is that I wanted this to be a declarative as possible so that it could become a model for writing future conversion tools for future API breaks. Additionally, I knew that this might be seen as a maintenance burden, so I added a bunch of tests for it that were written such so that if features changed in a way that would break the converter, they'd fail and point to an easy way to fix it, thus reducing the maintenance burden.

[nikitalita]

To be honest with you, I’m leaning towards closing this PR. Nobody here is very enthusiastic about what this enables and most have concerns about runtime and maintenance costs, which is understandable, but which would be an impediment to merging without a large amount of rewriting. I’m currently using it in production on my fork, it runs fine, and I don’t really have a problem with just rebasing on top of master for the foreseeable future. Unless anyone here really wants this, I’m just going to close it.

[Ivorforce]

Please don't close the PR! We are definitely interested in features that help developers migrate from 3.x to 4.x, and we really appreciate your work on the ESCN migration. I'm sorry if reviews or reactions didn't come off this way.

We're currently discussing among maintainers how we want to handle backwards compatibility code-wise. This seems not to be trivial, but we'll get back to you soon. Hopefully, we'll find a solution that addresses these concerns (reducing runtime code complexity) without creating much additional work for you.

(PS. if you do decide to close the PR, that would also be OK. It's possible that someone else will pick it up and finalize it, and credit will be given to you on merge)

[nikitalita]

@Ivorforce thank you for saying so! I’m willing to continue working on it as long as maintainers are interested in it.

[nikitalita]

To sum up what we discussed during the meeting:

• move code off hot path in update_caches in AnimationMixer to something cold
• Ensure that that code is moved to animation_mixer.compat.inc to reduce maintenance burden
• see if we can somehow ensure that the animation does get recalced whenever an old 3.x animation is loaded so that we don’t have to serialize the relative_to_rest property

[lyuma]

Took some notes from the animation meeting.
[EDIT: I see you also took notes, @nikitalita . Thanks! I guess we can keep both sets here in case one of us missed something]

Something we all agree on is we need to move conversion code out of Animation and AnimationMixer. We do not want the relative to rest functionality to be reintroduced.

Need to work with core maintainers to come up with an acceptable place to perform offline conversion during import of escn or converting 3.x assets. Because the animation code needs access to the Skeleton3D node to determine rest poses, this cannot be done inside a property setter alone. Maybe can be done in some compatibility conversion code outside of the animation code itself.

There was discussion of maybe putting conversion code in .compat.inc. but how exactly this is done needs consensus with core maintainers.
We also need to make sure we surround any code we add with TOOLS_ENABLED and is_editor_hint checks.

[nikitalita]

I've rebased this and updated the animation code to not use the metadata when updating transform tracks. I have not yet updated it to not perform animation caching on the hot path; please let me know when you have an answer for where to put this.

Also, I think it might be best if this got split into multiple PRs since this covers two separate areas, shader and animation code. Would that be preferable?

[TokageItLab]

Since this PR still contains many lines that should be removed, I suggest creating a new PR. In that PR, you should only modify files under editor/project_upgrade/ and avoid writing migration functionality within each class except for _set() and _get().