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

Fix missing options in Project Import Defaults #94058

Merged

Conversation

Hilderin
Copy link
Contributor

@Hilderin Hilderin commented Jul 8, 2024

Some modifications were made in 04d4394 that lead to remove some options in Project Settings. Also, some options like Blender specific parameters were never in the Project Import Defaults, which I guess was an error.

The premise I took was to put every options possible for all different file extension in the Project Import Defaults. This was the case in 4.2 except for .blend.

Modifications:

  • Fixed ResourceImporterScene::get_option_visibility to check all plugins and importers. Previously, the method would return the result from get_option_visibility at the first importer, leading to only processing the first plugin or importer. Now, all are processed until a return value of false is encountered.
  • EditorSceneFormatImporterFBX2GLTF::get_option_visibility returned false for only one the ufbx options if FBX2GLTF is enabled.
  • The validations that determine if an option is available or not has been moved in get_import_options methods and always returns all the option when the path is empty (aka Project Import Defaults).
  • The missing options for FBX and GLTF are now available in Scene and Animation Library in Project Import Defaults.
  • Blender options are now available in Project Import Defaults.
  • Fixed 'Set as Default for Scene' where options not displayed were reverted.

Others found issues:

  • Edited, these options does not makes sense for Blender: When importing from Blender, the options gltf/naming_version and gltf/embedded_image_handling are not used even if they were shown in 4.2. I could add these options for Blender, at least embedded_image_handling like it was done for ufbx??
  • When changing a value in the options (Project settings and file Import options), get_option_visibility are not recalled so that means that to see the effects of EditorSceneFormatImporterBlend::get_option_visibility or EditorSceneFormatImporterFBX2GLTF::get_option_visibility, the user needs to close the panel and reopen it. That could cause error if the user change the fbx/importer from FBX2GLTF to ufbx and tries to reimport. The 2 missing options will cause an error during importation.

Project settings:

4.2
image

This PR:
image
image

FBX Import options

4.2
image

This PR
image

EDIT: stroked out the issue with Blender options.

@Hilderin Hilderin requested a review from a team as a code owner July 8, 2024 03:18
@AThousandShips AThousandShips added this to the 4.3 milestone Jul 8, 2024
@Hilderin
Copy link
Contributor Author

Hilderin commented Jul 8, 2024

I found another problem that was probably always there with the default import settings of Scene.
If you set the Default from the Import Panel, only the visible setting are save and the other are reverted. That can cause if in the problem there are different file types or if some fbx uses ufbx and others fbx2gltf. Maybe the best courses of action should be to keep the non visible options while saving the defaults?

image

@lyuma
Copy link
Contributor

lyuma commented Jul 9, 2024

Looks reasonable at first glance. Seems better than the kludgy hack I was thinking of doing. I'll have to play with it a bit.

The magic is the p_path.is_empty() || clause since project settings uses an empty path.

Maybe the best courses of action should be to keep the non visible options while saving the defaults?

Nice catch. I also believe this is correct.

One thing I noticed is that there is something odd about the fbx/embedded_image_handling when FBX2glTF is used. It would be good to verify compatibility against 4.2 imported meshes which used the gltf/embedded_image_handling since this will be a common case for users upgrading their projects.

@lyuma lyuma self-requested a review July 9, 2024 07:40
@Hilderin
Copy link
Contributor Author

Hilderin commented Jul 9, 2024

I tested in debug with the 4.2 branch code and the settings gltf/naming_version and gltf/embedded_image_handling were never used in 4.2.
The EditorSceneFormatImporterGLTF is not used for fbx file (it's the only place were these settings are used). EditorSceneFormatImporterFBX call directly GLTFDocument::generate_scene.

Maybe it was a mistake, I don't kwown why these 2 options could not have worked with fbx?!?

@Hilderin Hilderin force-pushed the fix-scene-project-importation-settings branch from e66d6f6 to 7ec80d8 Compare July 9, 2024 12:44
@Hilderin Hilderin requested a review from a team as a code owner July 9, 2024 12:44
@Hilderin
Copy link
Contributor Author

Hilderin commented Jul 9, 2024

Maybe the best courses of action should be to keep the non visible options while saving the defaults?

I pushed a fix do exactly that. When default import settings exists, I update the dictionary instead for creating a new one.

@Hilderin
Copy link
Contributor Author

Hilderin commented Jul 9, 2024

When importing from Blender, the options gltf/naming_version and gltf/embedded_image_handling are not used even if they were shown in 4.2. I could add these options for Blender, at least embedded_image_handling like it was done for ufbx??

I tested with Blender and I think embedded_image_handling does not make sense for Blender. Textures must be unpacked because the .blend is converted in .gltf which is in json, and texture cannot be in the gltf, they are extracted in the 'textures' subfolder.

@Hilderin Hilderin force-pushed the fix-scene-project-importation-settings branch from 7ec80d8 to 169e732 Compare July 9, 2024 13:33
Copy link
Contributor

@lyuma lyuma left a comment

Choose a reason for hiding this comment

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

I think it's the best change we can make for now. I'm most worried about the hardcoded file extension checks, but that was already an issue before.

We should merge this and then look into overhauling this option system some in 4.4

Comment on lines +90 to +91
r_options->push_back(ResourceImporterScene::ImportOption(PropertyInfo(Variant::INT, "gltf/naming_version", PROPERTY_HINT_ENUM, "Godot 4.1 or 4.0,Godot 4.2 or later"), 1));
r_options->push_back(ResourceImporterScene::ImportOption(PropertyInfo(Variant::INT, "gltf/embedded_image_handling", PROPERTY_HINT_ENUM, "Discard All Textures,Extract Textures,Embed as Basis Universal,Embed as Uncompressed", PROPERTY_USAGE_DEFAULT | PROPERTY_USAGE_UPDATE_ALL_IF_MODIFIED), GLTFState::HANDLE_BINARY_EXTRACT_TEXTURES));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still apprehensive about hiding these options because options like "naming version" are supposed to be used for .blend and .fbx via FBX2glTF, though if you're 100% sure they are currently being ignored in those cases, I guess it's okay.

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 confirm that these options are used only in EditorSceneFormatImporterGLTF::import_scene and this method is used only on gltf and glb files.

@akien-mga akien-mga merged commit 2380ed5 into godotengine:master Jul 17, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

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