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 overriding file info of another file when reimport creates extra files #85922

Merged

Conversation

Listwon
Copy link
Contributor

@Listwon Listwon commented Dec 8, 2023

Fixes #80132, fixes #75525, fixes #77978, fixes #83154

Special thanks to @MetRiko and @gonzelvis for helping me figure out the exact repro steps. They had issues with *.glb files and repro turned out to be exactly the same as in #80132.

EditorFileSystem::_reimport_file calls _find_file(p_file, &fs, cpos) to get the file index in filesystem (fs). The problem is - if reimport creates extra fiiles, the filesystem gets invalidated and the next usage of cpos is incorrect - _reimport_file updates wrong file info leading to duplicated uid, dependencies etc. The uid inside created file is still correct at this point, but the data in fs->files[cpos] is invalid, so saving the scene after assigning the affected resource causes the resource to save with wrong uid.

Adding _find_file(p_file, &fs, cpos) just before the next line with cpos refreshes the cpos index so now it points to a proper file in fs->files.

@AThousandShips AThousandShips added this to the 4.3 milestone Dec 8, 2023
@akien-mga akien-mga requested review from KoBeWi and a team December 8, 2023 14:35
@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 8, 2023
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

The issue is confusing to reproduce and the fix is not obvious. I'd add a comment next to the new _find_file() explaining why is it called.

Other than that looks ok.

@Listwon Listwon force-pushed the fix-uid-corruption-on-reimport-80132 branch from 747150e to 69e7fcf Compare December 8, 2023 16:40
@Listwon
Copy link
Contributor Author

Listwon commented Dec 8, 2023

Thanks! I've just added the comment.

@MetRiko
Copy link

MetRiko commented Dec 8, 2023

@KoBeWi
Here are some of my thoughts on this PR to show it might be little more important than it looks like.

Every time I was trying to do something with Godot 4.x (personal projects, game jams etc.), I was always reaching the point where random scenes were getting corrupted. It was a constant thing for me especially in the 3D projects.

I was trying to fix scenes with the "Fix dependency" but there never was any error.
I also looked into these "corrupted" .tscns with the text editor but then again.. I wasn't able to spot anything wrong.

After long investigation of this issue (with the huge help of @gonzelvis) we were able to found the exact source of those problems and 100% working repro steps that could lead into corrupted scenes (and more).

In short.. this PR fixes problem which indirectly could cause:

  • disappearing set references to resources inside nodes,
  • invisible meshes on the scene (but the instance of such .tscn has visible mesh),
  • scene is invalid/corrupt but works in the runtime,
  • duplicated UIDs for two different resources,
  • resource UID is changed on the saving project,
  • restarting projects breaks resource references,
  • and many more issues related to importing/creating resources (especially the extracted resources from .glb files).

I don't have time to examine it further, but after what I learnt when I was investigating this bug I think this probably fixes the following issues (you can see most of those issues are related to glTF and/or MeshInstance3D):
#80132, #68672, #77978, #75970, #63513, #83575, #79545, #75525, #83200, #83154, #82275, #80536, #79779, #81671, #77978, #79275, #85850, #73916, #54774

Those above would be worth to check first but there might be the chance this also can fix some of those issues:
#84981, #82652, #82422, #82702, #76236, #77007, #76691

And for testing purposes, the easiest repro steps for the bug, that this PR is fixing, are the following:

  1. Prepare any .glb. If you don't have any then exported default cube from Blender will work for the repro too:
  • Open Blender, select default cube, export it as .glb (with enabled option "Limit to: Selected Objects" in the export settings).
  1. Create empty project with Godot 4.2, create scene with any type of node and save it.
  2. Put .glb inside the project -> open it with double click to open glTF importing window
  3. Export meshes from this .glb in any way you want, preferable is:
  • Press "Actions..." -> "Set Mesh Save Paths" -> "Select Current Folder" (.tres/.res setting doesn't matter) -> "Set Paths" -> "Reimport"
  1. Double click on the any created meshes (exported from .glb) in the filesystem to refresh editor (DON'T SAVE ANYTHING!)
  • Now you can see one of those meshes has the SCENE icon instead of MESH icon (what is weird)
  1. Check UID of this mesh with wrong icon ("RMB-> Copy UID" and paste it somewhere)
  2. Add this mesh to the scene (for example by drag and dropping it on the scene)
  3. Save the scene with 3D node (just press ctrl+s)
  • You can see icon for this mesh resource has been changed to the valid mesh icon
  1. Check UID of this mesh resource again and compare with previous one:
  • UID for this resource has been changed!
  • This UID is exactly the same as the .glb file has!

Now using such corrupted mesh with duplicated UID anywhere will lead to many different problems like corrupting scenes.

@KoBeWi
Copy link
Member

KoBeWi commented Dec 11, 2023

@MetRiko I checked these issues and was able to confirm 2 more that this PR fixes (already added them in the OP). I also closed 2 issues as already resolved/duplicate.
Other issues seem unrelated, though in some cases I think this PR does not fix things automatically; it's more to prevent the bugs from happening, which makes it more difficult to test as a fix.

@akien-mga akien-mga merged commit e551672 into godotengine:master Dec 11, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks! Great work tracking this down 🥇

@MetRiko
Copy link

MetRiko commented Dec 11, 2023

@KoBeWi

Other issues seem unrelated, though in some cases I think this PR does not fix things automatically

That's exactly what I meant by:

this PR fixes problem which indirectly could cause

and by

Now using such corrupted mesh with duplicated UID anywhere will lead to many different problems like corrupting scenes.

This will not fix directly problems caused by duplicated UIDs but will prevent UID's duplication. There is no reason in "fixing" bugs caused by using resources with duplicated UIDs because engine is not even suppose to have duplicated UIDs anywhere at all. Each resource should always have one unique UID right? So there is no point in fixing those anomalies, however it would be nice to have at least a good readable error/message that will point out duplicated UIDs and how to fix it.

@lyuma
Copy link
Contributor

lyuma commented Dec 12, 2023

Wow!! Thanks for fixing this! I think you and I may have discovered the problem independently around the same time, Wednesday last week. If I had submitted my discovery as a pull request, it might have saved you the time of doing this. Based on my diagnosis/understanding, this fix is correct.

The issue only affected files beginning with the letters "r" through "z" due to another problem in EditorFileSystem where it tries to keep directories sorted, but compares all strings against the full path starting with "res://", so most files were simply appended to the end of the directory, which would then not affect the cpos. (My workaround was to extract all embedded resources into a subdirectory, so as to not affect cpos.)

because engine is not even suppose to have duplicated UIDs anywhere at all

What happens if the user manually copies and pastes a file elsewhere in the project, and this file has a particular UID embedded in its .res header?

@MetRiko
Copy link

MetRiko commented Dec 13, 2023

@lyuma

What happens if the user manually copies and pastes a file elsewhere in the project, and this file has a particular UID embedded in its .res header?

Let's say you have file Entity.tscn and you will create EntityCopy.tscn by duplicating it outside Godot editor (in OS file explorer or with commands). In that case Entity.tscn and EntityCopy.tscn will have the same UID.

Now if you will:

  • remove UID part from EntityCopy.tscn with text editor,
  • open EntityCopy.tscn inside Godot and press ctrl+s,

then EntityCopy.tscn will get the same UID as before (still duplicated).

However if you will:

  • remove UID part from EntityCopy.tscn with text editor,
  • press "Project->Reload Current Project",
  • open EntityCopy.tscn inside Godot and press ctrl+s,

then new UID for EntityCopy.tscn will be generated.

For the references.. let's say you used EntityCopy.tscn inside World.tscn.
If you will open World.tscn and save it (ctrl+s), Godot will try to update ext_resource header with Entity.tscn automatically (you don't have to reload project). Depend on the situation, the header will be updated differently. There are 5 cases worth to mention:

  • uid=*, path=* - nothing will be updated.
  • ---, path=* - uid property will be added and its value will be taken from the file pointed by path.
  • uid="", path=* - uid will be taken from the file pointed by path.
  • uid=*, --- - SCENE CORRUPTED! You cannot open such scene within the Godot.
  • uid=*, path="" - path will be determined based on the uid.

(where uid=* means property with proper path, uid="", property with empty path and ---, property removed entirely).

I think this should be raised as a separated issue/proposal.

@clayjohn clayjohn added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 20, 2024
@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 24, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 24, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.4.

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