[Impeller] fix playground validation errors in imgui overlay.#167491
Conversation
|
|
||
| namespace impeller { | ||
|
|
||
| // These methods should only be used by render_pass_vk.h |
There was a problem hiding this comment.
"Methods" is the appropriate term, "fields" is probably what you want.
| if (sample_count != frame_data.sample_count) { | ||
| frame_data.framebuffer = nullptr; | ||
| frame_data.render_pass = nullptr; | ||
| } | ||
|
|
There was a problem hiding this comment.
Instead of clearing out the cached data if it doesn't have the right sample counts, should we instead be caching based on the sample count? This will cause thrashing in the cache in playgrounds where something is cached then thrown away directly, no? This isn't a big deal for playgrounds, my concern is that we'd accidentally introduce this behavior in production and it would take effort to figure out.
There was a problem hiding this comment.
I think its unlikely to be a problem in a real application as the render_target_cache actually separates MSAA and non-MSAA allocations.
There was a problem hiding this comment.
By effectively not caching in playgrounds but caching in production we are opening ourselves up to bugs that can only be reproduced in production, no?
There was a problem hiding this comment.
this is only going to happen with the imgui overlay active though.
There was a problem hiding this comment.
In playgrounds we do the imgui pass regardless if there is some imgui overlay active though. The test I was using was CanDrawCapsAndJoins which doesn't have any visual change from imgui.
The golden tests don't have the imgui pass though.
This feels fragile to me. What argument is there against adding TextureSourceVK::GetCachedFrameData(size_t sample_count)?
Alternatively, I think instead of nulling out the cached value when it doesn't match our needs we can just put it back. That way we wouldn't be thrashing. We'd still be caching when it's possible and creating a new one when it isn't. That makes the worst case scenario less worse.
There was a problem hiding this comment.
We don't use it for the goldens though, and AFAIK that is what is actually running on CI?
…r#167491) Fixes flutter#167316 Recycling the render passes didn't account for a render pass changing from MSAA to non-MSAA.
Fixes #167316
Recycling the render passes didn't account for a render pass changing from MSAA to non-MSAA.