-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
Do not print error when editor meta was not found as it will be reimported anyway after #86137
Conversation
That's only true because you've removed all the contents of the The error prints an actionable message for the user in this case. Which was especially important when the change was introduced, as existing projects would have imported textures but not metadata files. If the message appears under some circumstances which are valid and don't require user action, then the change should be to handle those situations gracefully rather than remove the message completely. Edit: We can also handle the entire thing differently. Instead of returning an empty dictionary we can pass a dictionary reference to the parameters and fill it, and return an error code instead. So we can differentiate between "there is meta but it's empty" and "there is no meta". The key things to keep in mind here is that we want to make sure we don't trigger reimport unless there is a reason to reimport. So missing meta may not be an invalid state, it may just mean that user didn't enable the relevant import setting. So ideally we still want to only exclude cases where everything is missing, but print the message in every other situation. |
fc91e90
to
e026915
Compare
Changed the logic to check if the file exist. The new logic right now works like this:
Although now the |
You did the exact opposite of what I was describing 🙃 The only case where we don't want an error is when |
I think you are mixing this up. I did not change the The only thing that is changed is when there is no Let us elaborate on this. What should happen for what case?
|
That's not what I was suggesting, but I did misread what kind of error we show in your bulletpoint list above. Point still stands, though, as I suggested we should show the error when the meta file is missing and we should reimport the icon without any issues if the ctex file is missing. If non-editor ctex and editor ctex are handled differently by the engine right now, and the latter errs where the former doesn't, it's a problem that needs to be addressed (but not necessarily right here and right now). The reason why we err when the meta file is missing is, as I explained, compatibility. Existing projects that predate the meta file can already have correctly imported textures which would work just fine without a reimport. The meta file is only needed to check for a reimport, it's not needed for the runtime. But we still want to tell users that they need to reimport affected textures to update them to the latest format of the metadata. If we decide that we don't care about this anymore, then sure, we can go with the way this PR is right now and just treat missing meta files as a reason to force a reimport. However, if we want to preserve this logic and address your issue at the same time, then we should check if the |
… be reimported after and no error is printed
e026915
to
4b8b080
Compare
Ahh, yes, I got you now. Thanks for explaining the details here. I did not fully understand the 'dependency' between the Behaviour now:
Looks much better. :) |
Thanks! |
Fixes: #85859
The error is printed when the editor metadata files for a particular image were not found,
which can happen when you check out a new project or clean up your
.godot
folder.But this is not really necessary, as Godot will reimport the file afterwards anyway,
because it detects that the metadata are not up to date (in this case it does not even exist).
The following is now changed:
ctex, editor.ctex, editor.meta, md5
) (happens for a newly checked out project)editor.meta
editor.ctex
ctex
md5
cc @YuriSizov I would be interested in your opinion as you have added this error line in #75949, just in case I miss something here. 🙂