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

Add resource details display to fgviewer web view #8469

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

show50726
Copy link
Contributor

Show some basic properties for selected resource:
image

Also

  • Implement utils::to_string for TextureFormat to show format as readable string
  • Refactor app.js

@show50726 show50726 added the internal Issue/PR does not affect clients label Feb 23, 2025
Comment on lines 549 to 550
auto descriptor = static_cast<Resource<FrameGraphTexture> const*>(
getResource(resourceHandle))->descriptor;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this static_cast<> is not always valid right? It works right now because FrameGraphTexture is the only kind of resources we have. Maybe this deserves at least a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. I add an if statement to check if the cast succeeded.

@@ -591,4 +591,47 @@ CString to_string<filament::backend::TargetBufferFlags>(filament::backend::Targe
return { string, 6 };
}


template<>
CString to_string<filament::backend::TextureFormat>(filament::backend::TextureFormat value) noexcept {
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you return a std::string_view<> instead? All these strings are static strings, so no need to do a heap allocation for them.

also I'm a little confused because this is a cpp file, so where is to_string declared?

Copy link
Contributor Author

@show50726 show50726 Feb 26, 2025

Choose a reason for hiding this comment

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

There's a to_string template for utils::CString, so I implemented the one for TextureFormat.

I can make the function return std::string_view, but since we store the resource properties in the class as utils::CString, even if we return a std::string_view here, we will still need to allocate a new CString then store it into the Resource::Property. WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Issue/PR does not affect clients
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants