[3.6] Setting OBJ to use .material file only works after manually re-importing.

[drwhut]

Tested versions

• Reproducible in: 3.6
• Not reproducible in: 3.5.3

System information

Linux Mint 22

Issue description

For some reason starting from Godot 3.6, when an .obj file is imported as a scene, even if in the import options it is specified that there should be a separate .material file, the first time it is imported the .scn that is generated has the material built-in, and it does not reference the .material file even though it exists. Only after the .obj file is manually re-imported (or the .import folder is manually deleted), does the generated .scn file reference the .material file.

This behaviour is affecting the output of ResourceLoader.get_dependencies, which is how I found this issue.

Any help in figuring out what changed between 3.5.3 and 3.6 would be greatly appreciated!

Steps to reproduce

1. Open the project. The .obj file should be imported as a mesh.
2. Switch the .obj file from being imported as a mesh, to being imported as a scene.
3. Restart the editor as prompted.
4. Once the editor is back, run the scene. The output shows an empty array [], when it should be [res://red_mat.material].
5. Manually re-import the .obj file, and run the scene again. Now the output is [res://red_mat.material], as expected.

Minimal reproduction project (MRP)

DependencyBug.zip

[lawnjelly]

I know very little about how these are serialized, but this seems to be due to some minor incompatibility between 3.5 and 3.6.

I can confirm the behaviour occurs in 3.6 beta 1.

The interesting thing is, once it has happened once in 3.6, no matter if you delete the .import file, it doesn't happen again. Which suggests something in the scene save is different.

I'll try and diff the files.

Is something very early. Am bisecting, it's pre- 24th Aug 2022, and 3.5 stable was released 5th Aug 2022.

It's broken on 6th Aug 2022, but I'm having trouble bisecting further than that, as there were some build changes which are causing the build to fail at that point. I'm slightly worried about corrupting my build tree for normal work so may have to leave it to someone else to bisect beyond there.

Very likely something that has been cherry picked from 4.x to 3.6 but not 3.5.

I was suspecting #64838 and specifically #62318 , but I'm not sure as it appeared to be broken before that cherry pick (but maybe I made some mistake).

UPDATE: No I checked by manually reverting #62318 and this did not cause the change, so it must be something earlier.

[matheusmdx]

Bisecting points to #62408 as the culprit, @Razoric480

The bulding error was fixed by #61692 (in my case was a error in android detect.py about distutils.version)

[drwhut]

I would like to use 3.6 if at all possible due to the added ARM support - what would be the best way of fixing this? I'm not sure how to go about it myself due to the fact that the commit in question deals with caching sub-resources, and I'm worried about causing another regression trying to fix this one.

[lawnjelly]

I would like to use 3.6 if at all possible due to the added ARM support - what would be the best way of fixing this? I'm not sure how to go about it myself due to the fact that the commit in question deals with caching sub-resources, and I'm worried about causing another regression trying to fix this one.

Well first thing we do (as above) is ping the original author, Razoric, as they are usually best placed to fix regressions. Sometimes the original author won't be available, in which case eventually another contributor will get to it. Anyone attempting to fix has to deal with the possibility of causing knock on regressions, especially in areas such as this.

If there is a hurry, and the original author is unable to fix, sometimes we can revert commits that introduce regressions. However this is not always possible, because new PRs may have been merged in the meantime on top of the existing commit, and unravelling the code becomes problematic. You may be able to try this locally with the git revert command.

Reverting also obviously removes the functionality added, be it fixing a bug, or adding a feature. If a commit fixes a minor bug but introduces a major bug, it's more likely to get reverted.

If you need instructions for building the engine / basic git / PR workflow, these are all in the docs.

[Razoric480]

I'll try to make a bit of time to investigate. Thanks for the repro project

[Razoric480]

I'm unable to reproduce in latest 3.x

1. Open the project. The .obj file should be imported as a mesh.
2. Switch the .obj file from being imported as a mesh, to being imported as a scene.
3. Restart the editor as prompted.
4. Once the editor is back, run the scene. The output shows an empty array [], when it should be [res://red_mat.material].
5. Manually re-import the .obj file, and run the scene again. Now the output is [res://red_mat.material], as expected.

Step 4 does indeed output [res://red_mat.material] without having needed to reimport

[Razoric480]

This does fail in 3.6-stable

[akien-mga]

This does fail in 3.6-stable

There should be almost no difference between 3.6-stable and latest 3.x (8c444fb9c9ed70306630ebaf4868a6629df1296c), aside from 3 merged PRs which shouldn't be related to this. So if there's a difference it's probably toolchain related, though since this is reproducible on Linux, it's not the typical MSVC vs mingw-gcc behavior difference.

Can you check if it still fails in 3.6-stable compiled locally the same way as latest 3.x?

[Razoric480]

It could have been user-error in managing my git branches. Let me double check

[Razoric480]

It was, in fact, user error between upstream and origin. Okay, will take a bit to debug the import code when I have the chance

[drwhut]

I think I've found the cause of the issue - using the commit that introduces the issue in ResourceInteractiveLoaderBinary::poll(), the difference between the .material resource being saved externally vs internally is whether or not no_subresource_cache is true or false, and I'm guessing it's this section that determines that:

if (!no_subresource_cache) {
    r->set_path(path);
}

Going up the chain, it looks like it's being called from ResourceImporterScene::_make_external_resources, which passes the no_subresource_cache value:

if (!p_materials.has(mat)) {
    String ext_name;

    if (p_materials_as_text) {
        ext_name = p_base_path.plus_file(_make_extname(mat->get_name()) + ".tres");
    } else {
        ext_name = p_base_path.plus_file(_make_extname(mat->get_name()) + ".material");
    }

    if (p_keep_materials && FileAccess::exists(ext_name)) {
        //if exists, use it
        p_materials[mat] = ResourceLoader::load(ext_name);
    } else {
        ResourceSaver::save(ext_name, mat, ResourceSaver::FLAG_CHANGE_PATH);
        p_materials[mat] = ResourceLoader::load(ext_name, "", true); // disable loading from the cache.
    }
}

It looks like p_keep_materials is true both after the restart, and after the manual re-import, it's actually the FileAccess::exists(ext_name) that changes from false to true.

My interpretation is that since the .material file was not created on re-import, it creates it, but it then informs the loader to not use sub-resource cache, which makes sense (after all, it shouldn't be in the cache if it just got saved), except for the fact that it needs to use sub_resource caching in order to assign the material a path, which is what gives it the external reference.

[lawnjelly]

Related to #96817