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

Lazily allocate RIDs for PlaceholderTextures to avoid allocating GPU resources unless used #79874

Merged
merged 1 commit into from
Aug 1, 2023

Conversation

clayjohn
Copy link
Member

Fixes: #77762

This may also fix unreported issues with PlaceholderTexture3D and the PlaceholderTextureLayered types.

The root of the issue here is that Godot instantiates resources to generate documentation in the editor. So the PlaceholderCubemapArray was being allocated on the GPU regardless of whether it was actually being used.

The get_rid() function is needed so that the texture types can actually be used. So I added it for PlaceholderTexture3D and PlaceholderTextureLayered. All other texture types lazily allocate RID like this so we don't allocate a texture on the GPU without actually using the texture.

Copy link
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

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

We indeed need to take far more care that in Godot objects, we shouldn't be doing any work in the constructor other than setting some default values.

@dsnopek
Copy link
Contributor

dsnopek commented Jul 31, 2023

Looks good to me! We're also using placeholders in the dedicated server export, and it's probably also beneficial in that context to not bother creating the RIDs (although, I guess it'd be using the dummy renderer anyway).

@YuriSizov YuriSizov merged commit 8965e24 into godotengine:master Aug 1, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@akien-mga akien-mga added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Aug 2, 2023
@YuriSizov
Copy link
Contributor

This can't be cherry-picked to 4.1 because in 4.2 we've split textures into dedicated files, but in 4.1 they are still in texture.h/cpp. A dedicated port would be required, if desired.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PlaceholderCubemapArray.new() causes memory leak with GLES3
5 participants