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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add metadata to resource previews #64628

Merged
merged 1 commit into from May 10, 2023
Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Aug 19, 2022

Prerequisite for #63263

This PR adds preview metadata to EditorResourcePreview. A metadata can be image dimensions, audio stream length, model triangle count etc. In #63263 I want to display this information in a tooltip, but the problem is that fetching this information requires loading the resource, which can be costly sometimes. This PR allows to easily retrieve and cache the metadata, because generators already load the resource to make the preview.

What I do is pass a HashMap to the generate() method of EditorResourcePreviewGenerator and the generator can add any metadata to that hashmap. It can be later retrieved by using resource path + metadata name. For now I only added "dimensions" metadata for textures.

Marking as draft because:

  • this metadata is not saved yet
  • the PR should force preview cache invalidation, because old cache obviously doesn't have the metadata
  • I think I should pass Dictionary instead of HashMap, because I convert it anyway 馃
    Any advice on these welcome, but I'll try to figure it out myself.

Comment on lines -116 to +110
if (p_str.begins_with("ID:")) {
hash = uint32_t(p_str.get_slicec(':', 2).to_int());
path = "ID:" + p_str.get_slicec(':', 1);
} else {
modified_time = FileAccess::get_modified_time(path);
if (!p_path.begins_with("ID:")) {
modified_time = FileAccess::get_modified_time(p_path);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is just a bonus refactoring.

@KoBeWi KoBeWi marked this pull request as ready for review August 21, 2022 14:29
@KoBeWi KoBeWi requested a review from reduz August 21, 2022 14:30
@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 21, 2022

Ok this is probably ready to review. The metadata is correctly saved and loaded. Not sure how to force cache invalidation though; old previews don't have metadata stored.

@KoBeWi KoBeWi force-pushed the a_new_meta branch 4 times, most recently from 356e480 to bcf9851 Compare August 21, 2022 18:09
@KoBeWi KoBeWi requested a review from a team as a code owner August 21, 2022 18:09
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Seems good to me. Lacking @reduz's explicit go but it doesn't seem too critical, he can validate later if needed.

Edit: Discussed it with reduz:

I am just not happy with the approach in general, but thinking how to do this properly is a good amount of work
we can probably still merge it anyway
If its important, it can be merged as-is, its fine

Needs a rebase. Might also be good to clarify what [param metadata] is used for in the docs.

@KoBeWi
Copy link
Member Author

KoBeWi commented May 9, 2023

Rebased. I clarified the metadata, but right now it can't be used from a plugin. Maybe I'll rework #63263 to add EditorResourceFileTooltipPlugin as suggested by reduz.

@akien-mga
Copy link
Member

Needs another rebase to fix CI.

@akien-mga
Copy link
Member

I promise the CI failure isn't my fault this time :P

@akien-mga akien-mga merged commit 104de1a into godotengine:master May 10, 2023
13 checks passed
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the a_new_meta branch May 10, 2023 12:22
@@ -35,6 +35,7 @@
#include "core/io/resource_loader.h"
#include "core/io/resource_saver.h"
#include "core/object/message_queue.h"
#include "core/variant/variant_utility.cpp"
Copy link
Member

Choose a reason for hiding this comment

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

Including the .cpp here causes a conflict with #70080 which also included the .cpp for the tests. I opened #78108 to make a header for this file so we can stop including the .cpp file.

@Zylann
Copy link
Contributor

Zylann commented Jul 6, 2023

The new required argument breaks compatibility, my plugin that was working from Godot 4.0 is no longer working in 4.1:

Parse Error: The function signature doesn't match the parent. Parent signature is "_generate(Resource, Vector2i, Dictionary) -> Texture2D".

And if I fix this, it won't work in 4.0 anymore.

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.

None yet

4 participants