-
-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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
Fix reimporting assets with csv in the project #92320
Fix reimporting assets with csv in the project #92320
Conversation
4e18c87
to
9ad1d4d
Compare
editor/editor_file_system.cpp
Outdated
|
||
// We need to increment the counter, maybe the next file is multithreaded | ||
// and don't have the same importer. | ||
from = i + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part seems related to #85130, which I guess aims to address the same issue (albeit in another branch).
I couldn't get full confidence there that the change is correct. Would be good to re-review both PRs conjointly and figure out what this code is trying to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first tentative was exactly what was done in #85130 but the problem is that the variable 'from' is used elsewere after the Ref<ResourceImporter> importer = ResourceFormatImporter::get_singleton()->get_importer_by_name(reimport_files[from].importer);
.
That is how I see it...
In my case I had:
Index 0: threaded = false, importer = csv_translation
Index 1: threaded = true, importer = texture
Index 2: threaded = true, importer = texture
Index 3: threaded = true, importer = texture
Index 4: threaded = false, importer = scene
If I use reimport_files[i].importer
:
The first iteration, i = 0 and from = 0, _reimport_file was called directly
The second iteration, i = 1 and from = 0, it tries to start the thread because the importers of index 0 (from) and 2 (i+1) are different which is not was expected:
The thread will be started with the wrong index:
Edited: I looked more closely the comments on the #85130 from @RandomShaper and effectively, there is another potential problem that I did not saw when !importer.is_valid()
.
I'll look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Referred to this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This have been addressed in this commit: 4f55118
I should have been clearer in my comment below.
9ad1d4d
to
4f55118
Compare
This last commit included modifications to fix #85130 and the comments from @RandomShaper |
I think this could fix at least partially the issue #58131 With the fix, the preview are wrong because they are based on .import files and .godot folder but the texture are working fine: |
4f55118
to
63ccccb
Compare
63ccccb
to
760e20f
Compare
760e20f
to
bce48a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't tested but the code looks good to me (can't speak for the last line of change that's discussed though)
Sorry, @AThousandShips I'm not sure what you are referencing with:
Everything should be good, I think. |
bce48a9
to
f1099ab
Compare
Welp I merged this inadvertently because I was testing it in my But I guess it's a good way to confirm whether it's working or not, let's see how it fares from now to when I'd need to make beta3 builds. Thanks! |
This should fix #83200
The problem was that the
DebugDialogue.csv.import
is invalid according to Godot because it’s missing thepath
line. Because of this the importer was not mark multithread and a counter was not incremented between threaded and non threaded reimportation. I fixed the increment problem ineditor_file_system._reimport_file
.The problem with the invalid
.import file
was caused becauseResourceImporterCSVTranslation
is the only importer that has no extension. It does not write any file in the .godot folder. I fixed that adding a check in_get_path_and_type
before returning ERR_FILE_CORRUPT.