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 helpers for backend tests (cleanup+render loops). #8505

Merged

Conversation

mhoff12358
Copy link
Contributor

Both for cleaning up graphics resources at the end of a test and for starting/stopping render loops.

@mhoff12358
Copy link
Contributor Author

mhoff12358 commented Mar 7, 2025

This is missing a FIXES, waiting on getting the correct bug ID.
Edit: resolved.


// These methods can't be overloaded because of a compiler error caused by the type safe casting
// constructor needing to be resolved for each option.
void AddSwapChain(filament::backend::SwapChainHandle swapChain);
Copy link
Member

Choose a reason for hiding this comment

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

I had a couple optional ideas here. Maybe name all of these methods simply add, and overload based on the argument type. Would save some verbosity since the type of the handle will be easy to infer based on context. For example:

Handle<HwTexture> const srcTexture = api.createTexture(...);
cleanup.add(srcTexture);

You could also return the handle from the add method so we can do things like this:

Handle<HwTexture> const srcTexture = cleanup.add(api.createTexture(...));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially wrote this as a templated add method but IIRC there was some weird templating issue. Due to all the random github delays it's been like 3 weeks since I wrote this so I don't remember what the issue was though, I'll change back to that and either it'll work or I'll remember what the problem was.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good! We can also address these improvements in follow-up PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Haha, I had left a comment with the issue. It was that method overloading doesn't work because there's an implicit constructor from Handle<A> to Handle<B> if A is a base of B and that throws off the overloading.
    It seems template specialization doesn't have that issue though.
  2. Both were pretty easy changes to throw in.

@mhoff12358 mhoff12358 force-pushed the mhoff/cleanup_render_loop_helpers branch 2 times, most recently from e2ee467 to 7f59bf6 Compare March 11, 2025 18:31
Both for cleaning up graphics resources at the end of a test and for starting/stopping render loops.

BUGS=[402472882]
@mhoff12358 mhoff12358 force-pushed the mhoff/cleanup_render_loop_helpers branch from 7f59bf6 to f9a4b4f Compare March 11, 2025 18:54
@mhoff12358 mhoff12358 added the internal Issue/PR does not affect clients label Mar 11, 2025
@mhoff12358 mhoff12358 requested a review from bejado March 11, 2025 19:23
Copy link
Member

@bejado bejado left a comment

Choose a reason for hiding this comment

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

LGTM! These test cases are already looking cleaner

@mhoff12358 mhoff12358 merged commit 8ea4cb5 into google:main Mar 11, 2025
12 of 13 checks passed
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