godotengine / godot

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

Starting from godot 3.6, materials generated from import gltf have its textures embebbed #94475

Closed JuanFdS closed 3 months ago

JuanFdS commented 3 months ago

Tested versions

System information

Windows 11 - Godot 3.6rc1

Issue description

When importing a gltf (bin + textures) in Godot 3.6rc1 I'd expect the generated material's textures to be external resources instead of embebbed resources, since this is how it was working on Godot 3.5.3 and from what I know no change was announced about it. (Or at least, have an option to toggle between both behaviors on the import screen).

I'd expect the material to work like this (this is how it works on 3.5.3). In the Albedo/texture property, there's a Load Path to where the texture is and the texture resource has a path. Screenshot 2024-07-17 122535

This is how the material is generated in 3.6rc1. The Albedo/texture property's texture doesn't have a path. Also, this material's size includes the textures, whereas the previous material is a lot smaller due to only having references to the textures. Screenshot 2024-07-17 122811

Steps to reproduce

Minimal reproduction project (MRP)

Added 2 repos so it's possible to compare them:

Godot 3.5.3 version

Github repo: https://github.com/JuanFdS/test-how-textures-are-imported-from-gltfs-godot353

Download godot 3.5.3 project as zip

Godot 3.6rc1 version

Github repo: https://github.com/JuanFdS/test-how-textures-are-imported-from-gltfs-godot36rc1

Download godot 3.6rc1 project as zip

lawnjelly commented 3 months ago

Haha at being assigned :grin: ! I know next to nothing about GLTF import lol. But I'll try and bisect when I get some time.

I had a look at the history and there haven't been any recent changes to GLTF afaik. @timothyqiu and @fire I believe have done a bit on it in the past.

It could be some external change that is causing this of course.

akien-mga commented 3 months ago

Yeah I mostly assigned you as release manager to figure out what caused the change, then you can assign who caused it ;)

fire commented 3 months ago

I think a git bisection would be a workflow to determine the commit that made textures embedded.

pablitar commented 3 months ago

Hey! After searching for a bit I found two commits that I think may have something to do with the issue:

https://github.com/godotengine/godot/pull/66856

And:

https://github.com/godotengine/godot/pull/66889

The first one seems to have caused a more serious issue with the textures not being placed in the material at all. The second one made the textures show up, but they are now embedded.

I confirmed that the previous commit to the merge of the first PR doesn't have the issue: https://github.com/godotengine/godot/commit/b3301d22c5a106c4b3586ba343434b1992b0f8db

lawnjelly commented 3 months ago

Ah many thanks for bisecting, I just about managed to replicate the bug but didn't have time today to bisect. . :+1: Pinging @LunaticInAHat

pablitar commented 3 months ago

Hey, since this is a little important for us, I took the liberty of coming up with a fix: https://github.com/godotengine/godot/pull/94482

It keeps track of the external images and loads them accordingly as textures, instead of replacing them with generic image textures.

I tried to also take into account the "sampler" flags from the GLTF.

One thing I'm worried though is that maybe the import flags of the external file should override the configuration of the GLTF?

LunaticInAHat commented 3 months ago

66856 was definitely broken; that's why it needed to be followed up with #66889 so quickly. I am sure that #66889 is responsible for the behavior being seen here, however (as I have explained a bit in #94482) some care and thought may be required to change the current behavior, without causing regressions with how the samplers work. Since filtering & repeat are set on textures in 3.x, if you want to respect the sampler settings from the GLTF, then you need to have copies of the texture that are specific to the GLTF document you are loading (or at least not shared with any other documents / references that specify different sampling)

I think that we do want to respect the GLTF settings, because artists expect to be able to get their models looking how they want them in their modelling tools, and then to be able to import them into Godot, and still have them look good. I think it would be a mistake to make people have to manually ensure consistency between those settings in Godot, and those settings in Blender, et. al.

akien-mga commented 3 months ago

Fixed by #94482.