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

Prevent obj importer from printing misleading error #54694

Merged
merged 1 commit into from
Nov 9, 2021

Conversation

briansemrau
Copy link
Contributor

@briansemrau briansemrau commented Nov 7, 2021

When importing .obj files with local paths to their .mtl files, a misleading error is printed.

Couldn't open MTL file 'some_file.mtl', it may not exist or not be readable.

The mtl file loads just fine anyway.

May be relevant to #28203, #36173
Cherrypickable for 3.3, 3.4, 3.x

@briansemrau briansemrau requested a review from a team as a code owner November 7, 2021 02:47
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

The new code is more clear. I'm in the middle of something so can you try doing some test cases importing objs for me and report back the results. It should do as you expect.

The code is shorter and easier to review.

@briansemrau
Copy link
Contributor Author

briansemrau commented Nov 7, 2021

I've tested with https://www.kenney.nl/assets/retro-medieval-kit (OBJ format) and this error is no longer printed. This pack doesn't have absolute paths, so I added the absolute path to the mtl into an obj file and it still outputs as expected.

@YeldhamDev YeldhamDev added this to the 4.0 milestone Nov 7, 2021
@YeldhamDev YeldhamDev requested a review from a team November 7, 2021 16:36
@fracteed
Copy link

fracteed commented Nov 8, 2021

Would really appreciate if you could also backport this to 3.4. I get many crashes when re-importing an obj with mtl in 3.3 and 3.4. I think it is related to this error spam, as I only get the crash when Godot is open and I replace an obj.

If I shut down Godot and replace the mesh, I still get this error in the console upon the auto re-import, but at least Godot doesn't crash. It is frustrating to have to shut down Godot every time I replace a mesh. Btw, it never crashes if I replace an obj without an mtl.

@briansemrau
Copy link
Contributor Author

briansemrau commented Nov 8, 2021

@fracteed If your issue is the engine crashing (losing unsaved work and requiring the engine to be restarted), then this change will not fix that. It's possible you're encountering a separate issue. Regardless, if this bug exists* in 3.3/3.4 then this might be able to be cherrypicked.

*Edit: confirmed, this is present in 3.x

@fracteed
Copy link

fracteed commented Nov 8, 2021

@briansemrau I don't lose any work when the engine crashes, as the file re-imports correctly when I then restart Godot. But the error warning about the mtl is still there in the console. For some reason having Godot open when I replace the mesh and the auto re-import happens is what causes the crash. Either way I still get that error message in the console about the mtl that you refer to in this commit.

This doesn't happen with any other reimported mesh formats, such as a plain obj, gltf or collada. It is something to do with the mtl. Even though the mtl does import correctly, so I am guessing it is something to do with this error message being triggered. The only way to really know, is if the error message is not spawned. If you can backport this to 3.4 then at the very least there is not a misleading error message in the console any more. Cheers!

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

Looks good to me. We discussed some validations tests done in one of the comments.

@fire fire added cherrypick:3.4 cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Nov 9, 2021
@akien-mga akien-mga merged commit 46780aa into godotengine:master Nov 9, 2021
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 3.5.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Nov 15, 2021
@akien-mga
Copy link
Member

Cherry-picked for 3.4.1.

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

Successfully merging this pull request may close these issues.

5 participants