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

Fix group reimport bug #68324

Merged
merged 1 commit into from
Dec 5, 2022
Merged

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Nov 6, 2022

When testing files for reimport the presence of a UID causes the file to be reimported (treated as if the file were from a version prior to UIDs I assume), this however causes problem with group files, in this case TextureAtlas, as in this case no UID is written (or kept in fact) when reimporting, causing the files in a group to be reimported each time

Also at some times (when opening the editor I believe) the resulting file (the group file) can be reimported itself causing it to be reimported twice in such cases, added a check to make sure files that are group files, but not yet in the group file cache, aren't reimported twice.

Edit: Also added editor progress update for group files in reimport

Fixes #68256
Fixes #54817

@kleonc
Copy link
Member

kleonc commented Nov 6, 2022

I'd guess it also fixes #54817 which is supposed to be fixed by #60888 which... is almost identical to this PR. 😄

@AThousandShips
Copy link
Member Author

Oh damn I missed that one!

@AThousandShips AThousandShips force-pushed the group_import_uid branch 2 times, most recently from 5d7a25f to f301606 Compare November 6, 2022 17:47
@AThousandShips
Copy link
Member Author

AThousandShips commented Nov 6, 2022

Think there are some factors the other PR doesn't cover so I'll leave this up for now unless you want me to close it

Thank you for finding

@kleonc
Copy link
Member

kleonc commented Nov 6, 2022

Think there are some factors the other PR doesn't cover so I'll leave this up for now unless you want me to close it

Let @akien-mga decide what to do about this PR and #60888. Either way I think the changes need a review from @reduz.

@LakshayaG73
Copy link

LakshayaG73 commented Nov 19, 2022

Hi @akien-mga can we please get some traction on this PR?

It unblocks the basic scenarios of atlas textures which are very important for any 2D game.

@AThousandShips
Copy link
Member Author

Also added a fix to ensure group files are updated properly after reimport, previously they would only be imported if the group file itself was reimported

@LakshayaG73
Copy link

LakshayaG73 commented Nov 28, 2022

After your latest commit, I deleted the .godot folder, and that seemed to error out one specific atlas file. I reinitialized its constituents as a new atlas file and all seems to be fine.

@AThousandShips
Copy link
Member Author

Any reason you deleted the .godot folder? It would error stuff if it is not initialized again probably, as some of the parameters have changed if you are using both this and #68380, not sure what you are asking about or what the issue is.

@LakshayaG73
Copy link

I always delete the .godot folder after a fresh build. Is that not good practice?

I am not using #68380 with this, for issues I have mentioned in the PR.

I just thought I should mention this, in case you have any insight into the reason for this. It only messed with one specific file, so it could be simply due to the deletion of the folder.

Otherwise, everything is working well.

@AThousandShips
Copy link
Member Author

Got it! Suspect it's just because of the deletion if it works well after

@fire fire requested a review from a team November 28, 2022 21:22
Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

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

LGTM, I also confirmed if fixes the two linked issues.
Really great work 🏆 ! Thanks!

@akien-mga akien-mga merged commit f3e6750 into godotengine:master Dec 5, 2022
@akien-mga
Copy link
Member

Thanks!

@LakshayaG73
Copy link

Thank you again :)

@AThousandShips AThousandShips deleted the group_import_uid branch December 5, 2022 17:50
@kleonc
Copy link
Member

kleonc commented Dec 5, 2022

@akien-mga I guess #60888 can be closed then (see #68324 (comment)).

@AThousandShips
Copy link
Member Author

AThousandShips commented Jan 12, 2023

Really great work

Thank you! Sorry forgot to reply before

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