search

godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
87k stars 19.52k forks source link

Importer Conflict with Same Extension Targeting #86309

Open fire opened 7 months ago

fire commented 7 months ago

Tested versions

Impacts the UFBX pull request: https://github.com/godotengine/godot/pull/81746

System information

Windows 11, Nvidia 3090

Issue description

In the current implementation of Godot Engine 4, there seems to be an issue when two importers target the same extension. The problem arises when one importer fails; instead of failing the entire import process, it skips to the next importer.

This behavior can lead to unexpected results and potential issues in the game development process. For instance, when using ufbx, there might be a need to fail the import entirely if the first importer fails, but the current system does not allow for this.

The desired behavior is to have an option to fail the entire import process if a specific importer (e.g., ufbx) fails, rather than skipping to the next available importer. This would provide more control over the import process and prevent potential issues caused by importing incorrect or incompatible data.

Steps to reproduce

  1. write an importer for a 3d scene
  2. target the same extension
  3. can't fail the entire importer chain
  4. it retrieves the last imported scn from the .godot cache

Minimal reproduction project (MRP)

Currently this requires a c++ module to reproduce. I can look into how to make a test.

fire commented 7 months ago

Here's another explanation why this is a problem.

Initially, the fbx2gltf importer is enabled for smooth conversion from FBX to glTF formats. However, there can be instances where fbx2gltf might be invalid, causing the import operation to fail.

In such cases, the model is imported using another importer ufbx.

Interestingly, even with an invalid executable, fbx2gltf manages to open the file by utilizing the cache created by ufbx!

However, we know know different interpretations of the FBX file by ufbx and fbx2gltf, leads to different scenes being generated. The scene created by ufbx is not identical to the one produced by fbx2gltf, resulting in an invalid state as the output does not match the expected result.

fire commented 7 months ago

I and @adamscott discussed a bit about possible designs:

  1. If the error code is OK and nullptr, then skip the current operation.
  2. If the error code is FAILED and nullptr, fail the entire stack of importers.
  3. We considered using existing error codes like ERR_FILE_CORRUPT, ERR_FILE_UNRECOGNIZED, or ERR_SKIP.
  4. We also contemplate creating a new error code, ERR_IMPORT_FAIL.
  5. The choice of error code depends on its use in the context.
fire commented 1 month ago

I think this is resolved by https://github.com/godotengine/godot/pull/92197 checking

lyuma commented 1 month ago

I think I understand actually... the issue is user without fbx2gltf opens a project designed for 4.1 which used fbx2gltf. Instead of failing the import, it tried to import with ufbx and all the nodepaths break

We should change it so instead of ufbx and fbx2gltf being separate importers, it should be one importer that calls the fbx2gltf codepath if importers is 1 and ufbx otherwise