Scenes preview icon (not thumbnail image) may be wrongly set when the root node has no script

mieldepoche commented 7 months ago

Tested versions

v4.3.dev.custom_build [7d151c838]

System information

linux

Issue description

may be related to (didn't investigate) https://github.com/godotengine/godot/pull/77932

the scene (root has no script). The first child has a custom icon	the scene thumbnail (notice the icon)

if a child of a scriptless root has a custom icon, the scene thumbnail will show the child's icon as a type.
adding an empty script to the root solves the problem.

Steps to reproduce

create scene with no script on the root
add a child node with any custom icon
save it and look at the file icon

Minimal reproduction project (MRP)

any

theraot commented 6 months ago

Where is the scene thumbnail you are looking at?

I have some issues with the scene icons in FileSystem in 4.3 dev 5, I haven't been able to make a good replication project for it, and I'm guessing it is related to this.

mieldepoche commented 6 months ago

Where is the scene thumbnail you are looking at?

I'm not sure I understand what you mean, but this issue is about this specifically:

the icon is like this	but should be like this

here's an mrp that showcases the wrong icon assignment: bug-scene-icon.zip

theraot commented 6 months ago

OK, I have figured out something.

The issue is related to the order of the resources in the scene file (which is based on the generated ids). I found that for the scenes that have the issue, the first resource is NOT the script that has the icon. And when the first resource is the script that has the icon, it works correctly.

For example I might have a scene with a resource with id 1_i0o7h and the script with the icon with the id 1_w2adj, which comes after... Resulting in Godot failing to find the icon.

I have been able to fix scene files by opening them, modifying the id of the script to be before other ids. For the example above, I would, with an external editor, modify the id to - for example -0_w2adj so it comes before all other ids in the file. I have to do the modification both at the top of the file, and also below where it is referenced to match, save it, have godot load it, and save it again in Godot.

So we have that:

The ids are being generated such that the 1_ prefix is repeated. Resulting in the script sometimes not being the first resource... Since the ids are random, the script would not be the first at random (this is why I was struggling to figure out with which scenes the error showed up, and why I was struggling to make a reproduction project on my end).
Godot is relying on the script being the first resource. I believe this is introduced in #77932

So the fix could be on the id generation, on the way Godot looks for the icon, or both.

As far as I can tell, Godot is looking for the icon only on the first resource for performance. And it is not modifying the ids to keep the diff simple (also I imagine this could make the code that generates the ids harder to mantain).

Anyway, @KoBeWi I summon thee.

KoBeWi commented 6 months ago

Heh https://github.com/godotengine/godot/pull/77932/files#r1524905159

theraot commented 6 months ago

I believe that avoiding iterating over all resources is a good idea.

Btw, I have also found that if the root node does not have a script, but a child node has a script with a icon, sometimes Godot picks that one, which I do not believe is instended.

KoBeWi commented 6 months ago

I think the only reliable and performant way to achieve this is storing file icon path in EditorFileSystemCache.

theraot commented 5 months ago

I found another wrinkle: When a scene extends another scene (scene inheritance) it does not take the parent scene icon.

Addendum: In this case the scene would not list a script attached to its root node, while but the parent scene might.

theraot commented 5 months ago

I think the only reliable and performant way to achieve this is storing file icon path in EditorFileSystemCache.

If you store the type (either script or not) instead of the icon, then it should be possible to resolve the icon from there, plus, it make these easy:

Addendum: This also avoids the need of propagating changes of the script icon to the scenes, and any issues that might come from that.

godotengine / godot