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

Keep looking when a preview plugin returns an empty image. #96260

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

basicer
Copy link
Contributor

@basicer basicer commented Aug 29, 2024

The documentation for EditorResourcePreviewGenerator::_generate says that "Returning an empty texture is an OK way to fail and let another generator take care."

This patch enables that behavior.

@basicer basicer force-pushed the preview-keep-looking branch from 4ec3c3b to 975039e Compare August 29, 2024 16:29
@basicer basicer requested a review from kleonc August 29, 2024 17:02
Copy link
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

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

The behavior matches the documentation now indeed. 👍

Returning an empty texture is an OK way to fail and let another generator take care.

Not fully sure whether we indeed want to prevent zero-size previews though. cc @KoBeWi

@KoBeWi
Copy link
Member

KoBeWi commented Aug 29, 2024

Well, zero-size textures are a convenient way to make empty previews, though not sure if there is use-case for that.
The is_valid() check is enough and we can adjust the description to mention null instead of empty image, but it's fine either way probably.

@basicer
Copy link
Contributor Author

basicer commented Aug 30, 2024

Well, zero-size textures are a convenient way to make empty previews, though not sure if there is use-case for that. The is_valid() check is enough and we can adjust the description to mention null instead of empty image, but it's fine either way probably.

I could see it going either way. Im not too picky as along as I can write a previewer for PackedScene when the builtin one returns null.

@akien-mga
Copy link
Member

I believe "empty texture" in the docs was referring to Ref<Texture2D>(), which would indeed be better described as "null".

IMO it's clearer if the way to bail out is to return null instead of also skipping a zero-sized valid texture. If users generate such a texture with no dimensions, I would expect it to be intentional (e.g. to disable the preview as a hack, as mentioned).

@basicer basicer force-pushed the preview-keep-looking branch from 975039e to 4ec3c3b Compare August 31, 2024 02:02
@basicer
Copy link
Contributor Author

basicer commented Aug 31, 2024

Seems like consensus is to allow zero size previews, so I reverted it to the original null-check only version.

@kleonc
Copy link
Member

kleonc commented Aug 31, 2024

Seems like consensus is to allow zero size previews

Indeed.

so I reverted it to the original null-check only version.

Which makes my first comment hold. The docs could be clarified by changing "Returning an empty texture (...)" to "Returning [code]null[/code] (...)".

Returning an empty texture is an OK way to fail and let another generator take care.

Returning an empty texture is an OK way to fail and let another generator take care.

@basicer basicer requested a review from a team as a code owner September 1, 2024 05:58
@kleonc
Copy link
Member

kleonc commented Sep 1, 2024

Almost good, please squash your commits (should be a single commit). See the PR workflow docs.

@basicer
Copy link
Contributor Author

basicer commented Sep 1, 2024

Almost good, please squash your commits (should be a single commit). See the PR workflow docs.

Can do. For some reason I thought he rebase on merge option in github was turned on here.

EditorResourcePreviewGenerator::_generate says that
"Returning an empty texture is an OK way to fail and
let another generator take care."

This patch enables that behavior.
@basicer basicer force-pushed the preview-keep-looking branch from 84055a7 to 28e7069 Compare September 1, 2024 22:17
@akien-mga akien-mga merged commit d06579f into godotengine:master Sep 2, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

@basicer basicer deleted the preview-keep-looking branch September 2, 2024 22:19
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.

5 participants