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

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

Open
mieldepoche opened this issue Mar 27, 2024 · 8 comments

Comments

@mieldepoche
Copy link
Contributor

mieldepoche commented Mar 27, 2024

Tested versions

v4.3.dev.custom_build [7d151c8]

System information

linux

Issue description

the scene (root has no script). The first child has a custom icon the scene thumbnail (notice the icon)
image image
  • 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
Copy link
Contributor

theraot commented Apr 18, 2024

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

mieldepoche commented Apr 19, 2024

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
image image

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

@theraot
Copy link
Contributor

theraot commented May 2, 2024

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 Fix custom resource icons in FileSystem #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
Copy link
Member

KoBeWi commented May 2, 2024

@theraot
Copy link
Contributor

theraot commented May 2, 2024

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

KoBeWi commented May 2, 2024

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

@theraot
Copy link
Contributor

theraot commented May 25, 2024

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
Copy link
Contributor

theraot commented Jun 9, 2024

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.

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

No branches or pull requests

4 participants