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

Unable to assign Skeleton with Import Script #95461

Open
scotmcp opened this issue Aug 13, 2024 · 10 comments · May be fixed by #100792
Open

Unable to assign Skeleton with Import Script #95461

scotmcp opened this issue Aug 13, 2024 · 10 comments · May be fixed by #100792

Comments

@scotmcp
Copy link
Contributor

scotmcp commented Aug 13, 2024

Tested versions

  • Reproducible in all 4.x versions.

System information

Godot v4.3.rc3.mono - Linux Mint 22 (Wilma) - X11 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 4060 Ti (nvidia; 535.183.01) - AMD Ryzen 7 5700X 8-Core Processor (16 Threads)

Issue description

The ConfigFile class set_value method erases key/value pair when value is set to null (this is per class docs for ConfigFile.set_value). However, some files such as .glb.import files have required keys stored with explicit values for some options.

There is already the method erase_section() and erase_section_key(), and we need the ability to write null values to key/value pairs.

Example for when we need to be able to write an explicit null:

[params]

{_subresources={
"nodes": {
"PATH:Armature/Skeleton3D": {
"rest_pose/external_animation_library": null,
"retarget/bone_map": Resource("res://explosive_bone_map.tres")
}
}
}

If this value is not set, then the entire PATH:Armature/Skeleton3D node is over written by the engine on the next import.
Thus an entry to set skeleton ex. {"nodes" : {"PATH:Armature/Skeleton3D": {"retarget/bone_map": Resource("res://bone_map.tres")}}} will also be erased and unset.

To justification that a change is needed:

If one copies and pastes a correct but partial entry, the editor will erase the incomplete entry upon next import...example:

_subresources={
"nodes": {
"PATH:Armature/Skeleton3D": {
"retarget/bone_map": Resource("res://explosive_bone_map.tres")
}
}
}

becomes:
_subresources={}

If one copies and pastes the complete correct node entry with the required null value, the editor is fine. example:

_subresources={
"nodes": {
"PATH:Armature/Skeleton3D": {
"rest_pose/external_animation_library": null,
"retarget/bone_map": Resource("res://explosive_bone_map.tres")
}
}
}

Therefore one must conclude that the Class used to read and write configuration files such as .import files, should be able to write key/value pairs that include explicit null values.

There is already the method erase_section() and erase_section_key(), and we need the ability to write null values to key/value pairs.

Steps to reproduce

@tool # Needed so it runs in editor.
extends EditorScenePostImport

var config = ConfigFile.new()

func _post_import(scene):
	set_skeleton()

func set_skeleton() -> void:
	var bone_map : BoneMap = load("res://explosive_bone_map.tres")
	config.load("res://relax.glb.import")
	var rest_pose_node : Dictionary
	rest_pose_node = {"nodes" : {"PATH:Armature/Skeleton3D": {"rest_post/external_animation_library": null}}}
	config.set_value("params", "_subresources", rest_pose_node)
	config.save("res://relax.glb.import")

Minimal reproduction project (MRP)

Animation Test.zip

EDIT: I said FBX by mistake up above, I corrected that to GLB....I am not sure if that's important or not...I would expect the import file behavior to be the same for either.

@akien-mga
Copy link
Member

The FBX code should be changed instead to accept a missing rest_pose/external_animation_library.

Changing the way ConfigFile behaves would break compat.

@akien-mga akien-mga changed the title ConfigFile erases keys when value set to null and some files require explicit null values set. FBX import settings lost due to ConfigFile erasing keys when value set to null Aug 13, 2024
@fire
Copy link
Member

fire commented Aug 13, 2024

@lyuma Could look into this.

scotmcp added a commit to scotmcp/explosive.ws-to-godot that referenced this issue Aug 13, 2024
@lyuma
Copy link
Contributor

lyuma commented Aug 14, 2024

So there are two things going on here:

One is that there's a stray null in the .import file. I have a PR which should fix this, but the null doesn't do anything and so this is a purely cosmetic fix: I don't think this issue is a release blocker.

The second problem relates to the discussion from #95413, where I advised use of ConfigFile (typoed as ConfigMap) for modifying the .import file. The issue is that the config file likely will be overwritten after the import finishes, and hence it is not possible to modify the .import from within a PostImport script (or PostImportPlugin) in the way the MRP is doing.

Unfortunately I'm not aware of a clean way to do what that MRP is trying to do. In my code, I run a script which modifies the .import files specifically when an import is not currently happening, then trigger a reimport after. There's definitely a big API gap here.

I don't have a suggestion on how to fix the MRP to work correctly, but the closest thing I can think to suggest is run the code outside the import (perhaps in a call_deferred) and then run reimport_files on EditorFileSystem. Make sure to avoid an infinite loop, since the reimport_files will run the script again.

@scotmcp
Copy link
Contributor Author

scotmcp commented Aug 14, 2024

Thank you Lyuma.

yeah, ok....doing it outside of the import process kind of defeats the point. I was trying to automate all necessary configurations and modifications to an animation library during the import process because this stuff gets written to the res file in the .godot/import folder, the setting the bones up was the last piece of this particular puzzle to fully automate it.

EDIT: Another thought, I wonder if I could run a tool script that will set the import script up prior to import? Then I could just trigger an import and the skeleton could already be in place before that happens.

EDIT2: OK, that checks out ... I replicated that workflow manually by cutting the import script configuration item and the _subresources configuration with an external text editor, saving the file, then letting godot reimport and resetting those 2 settings back to default. Then, I pasted those lines back in to the file from the external text editor, saved the file, and triggered another import....Godot set the skeleton and ran the import script with intended results.

Very kludgy, but it works for now, so thanks

@lyuma
Copy link
Contributor

lyuma commented Aug 14, 2024

doing it outside of the import process kind of defeats the point

Yeah I agree the usecase of changing settings during import makes sense. I'll try and see if there's a way we can improve the APIs in 4.4 to account for this. Though either way, EditorScenePostImport is too late because it literally is the last thing that runs.... so the way your script was written still would have to be changed.

In my opinion, EditorScenePostImportPlugin, and other such plugins that happen earlier in the import flow, would ideally have a way to adjust import options on the fly without doing a reimport. This is something I hope to make possible in 4.4

@scotmcp scotmcp changed the title FBX import settings lost due to ConfigFile erasing keys when value set to null Unable to assign Skeleton with Import Script Oct 4, 2024
@scotmcp
Copy link
Contributor Author

scotmcp commented Nov 16, 2024

doing it outside of the import process kind of defeats the point

Yeah I agree the usecase of changing settings during import makes sense. I'll try and see if there's a way we can improve the APIs in 4.4 to account for this. Though either way, EditorScenePostImport is too late because it literally is the last thing that runs.... so the way your script was written still would have to be changed.

In my opinion, EditorScenePostImportPlugin, and other such plugins that happen earlier in the import flow, would ideally have a way to adjust import options on the fly without doing a reimport. This is something I hope to make possible in 4.4

Hi Lyuma,
Have you had a chance to look into this? I don't see anything in the PRs that addresses it, but maybe I am just missing it. I am still hoping to fully automate this process so I am not doing it by hand 1600+ times.

Thanks,
Scot

@lyuma
Copy link
Contributor

lyuma commented Nov 16, 2024 via email

@scotmcp
Copy link
Contributor Author

scotmcp commented Nov 16, 2024

This is something I hope to make possible in 4.4
Been fighting with stress and I haven't gotten anywhere near progressing on my ambitious priorities that I put down for 4.4 (I work on Godot as a volunteer). If you want to increase the chance that I make this before the 4.4 feature freeze, could you propose some ideas for your dream API to change .import options during import? Like an extra function in post import plugin? Having some direction would help me focus.

Lyuma, sorry you are dealing with stress. That really sucks, thank you for what you are able to accomplish. Being the lead dev (whether in name or action) on a section of the engine I am sure has a cost to you.

Really the most important thing to me is the ability to assign a skeleton from script. The way it is now, I have to manually import the asset (model or animation), assign the skeleton through the UI, reimport, and THEN assign the import script and reimport again.

Best case be ability to import through script only

BUT

It would already be much better if I could simply assign the skeleton within the script so I don't have so many manual operations per asset (i.e. import, assign script, reimport (3 ops) vs. import, open, assign skeleton, reimport, assign script, reimport (6 ops). That alone would cut the work of importing thousands of assets in half.

Thank you for all the hard work you do.

@lyuma
Copy link
Contributor

lyuma commented Dec 24, 2024

From reading the Godot code, I did find out that you can actually call get_option_value() from the _pre_process method of EditorScenePostImportPlugin with the argument _subresources, and that will return a Dictionary which allows you to modify settings such as the BoneMap of the Armature/Skeleton3D node, without actually needing to modify .import before import. If it were better documented, I think this is pretty reasonable solution.

You do need to be careful of course when modifying such settings, and having the user go through this instead of having a convenient API will reduce the chance that someone does this unintentionally and makes a mistake, so I would be curious if this gets you unstuck.

That said, I came here to see if I could improve Godot's APIs.

So the two things I'm trying to decide between are:

  1. To make the options dict mutable (the EditorFileSystem code actually writes the .import file afterwards, so there's no technical reason that it needs to be passed as const to the importers.

I do worry there is more of a stylistic reason not to do this: it feels a bit late to modify import options, while the file is already in the process of being imported, particularly the global options since many of them take effect before even the pre_process method of EditorScenePostImportPlugin. So making it too easy for users to accidentally edit the import options (if the options dictionary is mutable), could lead to inadvertent corruption of the .import settings. It also means we could get into conflicts between plugins since the order things get modified might matter. It just doesn't feel good to rush this into 4.4

  1. Add a callback in EditorFileSystem before import, but after assigning the default project settings, so the import settings can be changed. The problem is, I don't think this helps your situation. I think the _pre_process is the perfect place to do this, since then you have access to the Node tree and you know which node is the skeleton and so on.

But if you want to set a setting like nodes/import_as_skeleton_bones (which is often useful when importing animation files, for example), that cannot be done from a post-import plugin, since that option is consumed by the GLTF import code. For this type of option, the ability to adjust import settings from EditorFileSystem might be helpful. The main problem I have here is it might imply building a whole plugin system, for a relatively niche usecase: the desired settings are not the default, but you only know the filename and nothing else, how would it know what settings to write to the .import file before the import begins, without knowing more context about the file.

Anyway, maybe I'm just making an excuse to not work on this API for 4.4. But I am really curious if the EditorScenePostImportPlugin idea I had with _subresources works for you, since that would solve most of the problems:

extends EditorScenePostImportPlugin

func _pre_process(scene: Node):
	subresources = get_option_value("_subresources")
	if not subresources.has("nodes"):
		subresources["nodes"] = {}

	var bone_map : BoneMap = load("res://explosive_bone_map.tres")

	# Possibly do some recursive search on scene to determine the actual Skeleton3D node. Sometimes it is not inside Armature.
	node_path = "Armature/Skeleton3D"
	if not subresources["nodes"].has("PATH:" + node_path):
		subresources["nodes"]["PATH:" + node_path] = {}
		# Set it in the if statement to allow the user to override after the first import.
		subresources["nodes"]["PATH:" + node_path]["retarget/bone_map"] = bone_map

Ooh I think I see the problem... resource_importer_scene.cpp pre-caches the nodes dictionary and fails if it was not assigned to begin with...

	Dictionary subresources = p_options["_subresources"];

	Dictionary node_data;
	if (subresources.has("nodes")) {
		node_data = subresources["nodes"];
	}

	Dictionary material_data;
	if (subresources.has("materials")) {
		material_data = subresources["materials"];
	}

	Dictionary animation_data;
	if (subresources.has("animations")) {
		animation_data = subresources["animations"];
	}

	Dictionary mesh_data;
	if (subresources.has("meshes")) {
		mesh_data = subresources["meshes"];
	}

I think we should simply move these if statements after the _pre_process function, and that's something I should be able to change for Godot 4.4

@lyuma lyuma linked a pull request Dec 24, 2024 that will close this issue
@scotmcp
Copy link
Contributor Author

scotmcp commented Dec 24, 2024

Hi Lyuma,
Thanks for taking the time to look into this issue. I have run into a new issue which is the same problem as the skeleton assignment. If one needs to set an external pose, this also like the skeleton needs to be performed manually. I wonder if this import workflow just needs to be overhauled to make more sense and to allow full automation from beginning to end. I am not suggesting for for now, but maybe something to consider for next release ?!?

I will look into the suggestions you made, I was kinda waiting to see what might happen first. Give me a little bit of time though, because I am working on a new workflow and I am still in Blender, and not yet to the Godot Import phase with this assets.

Happy Holidays !!!

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

Successfully merging a pull request may close this issue.

4 participants