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

Creating AtlasTextures in Godot 4.0 beta 4 causes re-imports every time we reload, and engine runs slower #68256

Closed
LakshayaG73 opened this issue Nov 4, 2022 · 27 comments · Fixed by #68324

Comments

@LakshayaG73
Copy link

Godot version

4.0 beta 4

System information

Windows 11

Issue description

I've started using AtlasTextures on all my sprites for the massive VRAM savings.

There are three issues -:

  1. Everytime I open the project now, I see errors like this that eventually go away:

image

  1. There are reimports every time I open the project

  2. The engine seems to be running on a different setting now. It is generally slower, and specially if I alt tab, it takes it a few seconds to wake up. This happens any time I change the window, and otherwise when navigating the filesystem.

The VRAM savings are good, but slowed development is bad. Thanks for your help.

Steps to reproduce

  1. Use atlastextures in project

Minimal reproduction project

No response

@LakshayaG73
Copy link
Author

Every time I alt tab and come back, CPU usage shoots up to 30%.

@Calinou
Copy link
Member

Calinou commented Nov 4, 2022

@LakshayaG73 Please upload a minimal reproduction project to make this easier to troubleshoot.

We can't do anything here without a MRP, so the issue will have to be closed in 7 days otherwise.

@LakshayaG73
Copy link
Author

LakshayaG73 commented Nov 4, 2022

Let me try and create an MRP for this with just one atlastexture, it should be enough to show that it keeps trying to load the texture.

The issue is very evident. Every time I alt-tab, or minimize the godot window, or even play the scene, I am reloading all the textures that point to sprite atlases:

image

This is happening every time I alt tab! This cannot be normal behavior.

@LakshayaG73
Copy link
Author

MRP.zip

The project above is small but experiences all the issues as described above even with a single atlas texture. Seems like the feature is broken.

You will experience huge delays when alt tabbing and opening the project.

@LakshayaG73
Copy link
Author

I can see that this is coming from:

image

Could it be because this cache mode has been set incorrectly? Shouldn't these resources be cached somewhere?

@LakshayaG73
Copy link
Author

image

The cache mode could be set this way due to 'load from file' being called, because the image is not being loaded 'OK'.

Please let me know if this is the right path?

@AThousandShips
Copy link
Member

AThousandShips commented Nov 5, 2022

Doing some digging it looks like the hash for TextureAtlas is broken, not sure exactly what's going on but on reload the md5 hash for the atlas file is changed, this would naturally cause the editor to reimport it, and thus force the target file to be reimported, on each entering of the editor, will see if I can find where this occurs and what is going on exactly with that

It appears it occurs even if the hash is the same, no clue what's going on with that now, suspecting something is going on with group imports or related, but the issue in any case is that the file is reimported each time due to some check failing somewhere

@AThousandShips
Copy link
Member

AThousandShips commented Nov 5, 2022

Ignore all that, it turns out it was way simpler:
Files included from a group are not given a UID on import, possibly an oversight from past versions I don't know, however import files without one are always queued for import, therefore these atlas files are always reimported

I'll look at a PR for this tomorrow, some brief testing shows promise

@LakshayaG73
Copy link
Author

Thank you for your efforts, much appreciated

@AThousandShips
Copy link
Member

There seems to have been a second issue as well where under some condition (like opening the editor) the target file would be reimported on its own, and then the group would be reimported, which would more or less double the import time under those conditions, so adding a tentative check to prevent files that are groups from being imported twice

@AThousandShips
Copy link
Member

Also, for the future I recommend you not include the .godot folder in your MRPs, it can contain information about your computer that could be a security risk, not a big deal in this case but a good practice to keep in mind

@Calinou
Copy link
Member

Calinou commented Nov 6, 2022

Also, for the future I recommend you not include the .godot folder in your MRPs, it can contain information about your computer that could be a security risk, not a big deal in this case but a good practice to keep in mind

Is that actually the case? Please don't spread this kind of information without verifying it beforehand (and reporting it in a dedicated issue if it turns out to be the case).

@AThousandShips
Copy link
Member

Oh I'm sorry I thought I had seen that being pointed out before! And considering the issues with files in git repositories

@AThousandShips
Copy link
Member

I had understood that file paths external to the res:// filespace was an issue, and there is the reference to the executable in there somewhere

@kleonc
Copy link
Member

kleonc commented Nov 6, 2022

Is that actually the case?

@Calinou I've briefly checked the files from the MRP and this can be found in the .godot\editor\project_metadata.cfg:

[editor_metadata]

executable_path="C:/Users/User/Downloads/Godot_v4.0-beta4_win64.exe/Godot_v4.0-beta4_win64.exe"

@Calinou
Copy link
Member

Calinou commented Nov 6, 2022

The metadata storage in question was added by 2f15106, likely for use by the Godot Rider plugin.

@van800 Do you think we could make it a relative path if the executable is located within %USERPROFILE% (Windows) or $HOME (macOS/Linux) (or any subdirectory of it)? This prevents the username from being stored in the path. The path can be stored as absolute if the executable is saved outside the home directory, since it won't contain the username.

@van800
Copy link
Contributor

van800 commented Nov 6, 2022

I am under the impression that .godot content including project_metadata.cfg is not supposed to be added to the vcs. If so, then what is the issue with the username?

@Calinou
Copy link
Member

Calinou commented Nov 6, 2022

I am under the impression that .godot content including project_metadata.cfg is not supposed to be added to the vcs. If so, then what is the issue with the username?

It's not supposed to be committed to version control, but some people still include it in ZIP archives they create to upload as minimal reproduction projects here (or on other places).

Including the username is problematic for people who use their real name as their OS username and wish to keep their online identity private. Not everyone can change their OS username (e.g. on corporate/university-owned machines). Even when changing the OS username is possible, it requires recreating a new and moving everything, which is a time-consuming process. This was discussed in another issue/proposal, but I can't find it right now.

@Zireael07
Copy link
Contributor

We discussed it in #29674 (I managed to find it by searching for "privacy")

@LakshayaG73
Copy link
Author

Sorry to derail this conversation a bit...

Regarding this issue. The PR from @AThousandShips works great.

There is one more thing that I've noticed. Which may be by design, but I'm not sure. I've noticed that the width of the atlastexture doesn't go below 2048:

image

Can't help but wonder about the extra dead space here as well. Is this done on purpose?

@kleonc
Copy link
Member

kleonc commented Nov 6, 2022

There is one more thing that I've noticed. Which may be by design, but I'm not sure. I've noticed that the width of the atlastexture doesn't go below 2048:

@LakshayaG73 You mean it doesn't go above 2048? That's indeed the default value which is currently used for the max width by the atlas packer:

static void chart_pack(Vector<Chart> &charts, int &r_width, int &r_height, int p_atlas_max_size = 2048, int p_cell_resolution = 4);

Can't help but wonder about the extra dead space here as well. Is this done on purpose?

Is there an extra dead space though? Looking at the preview you've shown it seems like it's just zoomed out and the preview's transparent background is rendered way bigger then it should (not just behind the actual image) as this rectangle has approximately 2048 / 18556 aspect ratio:
JhDzc1O

@LakshayaG73
Copy link
Author

whoops. you're right. thanks for the clarification!

@LakshayaG73
Copy link
Author

There is one more issue here it seems.

If I use a Mesh to pack these textures together, like I've done in the image above, and try to add the frames to a spriteframes, some frames show up as empty. The reason is that some frames are simply not added to the mesh. This does not occur if you select 'region' when creating the AtlasTexture.

Mesh2D, created with the same frames:
image

Region, created with the same frames:
image

@LakshayaG73
Copy link
Author

The same frames are being skipped every time. Perhaps they are being considered duplicates?

@kleonc
Copy link
Member

kleonc commented Nov 6, 2022

The same frames are being skipped every time. Perhaps they are being considered duplicates?

@LakshayaG73 No idea. But for sure it seems like a seperate problem. So please open a new issue and provide all needed details in there (and an MRP so it could be investigated properly).

@LakshayaG73
Copy link
Author

#68350

@LakshayaG73
Copy link
Author

Could you please remove the "needs testing" label from this @Calinou as there is a PR out. Thank you.

@akien-mga akien-mga added this to the 4.0 milestone Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants