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

Importer Conflict with Same Extension Targeting #86309

Open
Tracked by #91519
fire opened this issue Dec 18, 2023 · 4 comments
Open
Tracked by #91519

Importer Conflict with Same Extension Targeting #86309

fire opened this issue Dec 18, 2023 · 4 comments
Assignees
Milestone

Comments

@fire
Copy link
Member

fire commented Dec 18, 2023

Tested versions

Impacts the UFBX pull request: #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.

@adamscott adamscott added this to the 4.x milestone Dec 18, 2023
@fire
Copy link
Member Author

fire commented Dec 18, 2023

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
Copy link
Member Author

fire commented Dec 18, 2023

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 fire mentioned this issue Dec 20, 2023
27 tasks
@akien-mga akien-mga added the bug label Feb 22, 2024
@lyuma lyuma modified the milestones: 4.x, 4.3 May 5, 2024
@fire
Copy link
Member Author

fire commented May 28, 2024

I think this is resolved by #92197 checking

@lyuma
Copy link
Contributor

lyuma commented May 28, 2024

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

@fire fire moved this from Unassessed to Bad in 4.x Release Blockers May 28, 2024
@clayjohn clayjohn modified the milestones: 4.3, 4.4 Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

5 participants