[Impeller] Add MockFence to mock_vulkan.
#45862
Conversation
…larke/engine into gaaclarke-mock-vulkan-context-builder
|
I think the way you would do this to make it reviewable is to set the branch to merge against to @gaaclarke 's branch, then change it back to main after that lands. |
I tried that initially but couldn't see Aaron's branch in the list? |
|
Yeah I'm not sure if there is a way to set a forks branch in there though. You'd need to have a branch on flutter/engine |
You have to add my fork of the repo in your remotes. I have for example jonah's repo named so I can grab PR's from him before they have merged. At any rate that PR has landed now finally. |
Thanks! Merged and ready for review. |
matanlurey
changed the title
MockFence to mock_vulkan.MockFence to mock_vulkan.
| @@ -410,7 +413,8 @@ VkResult vkCreateFence(VkDevice device, | |||
| const VkFenceCreateInfo* pCreateInfo, | |||
| const VkAllocationCallbacks* pAllocator, | |||
| VkFence* pFence) { | |||
| *pFence = reinterpret_cast<VkFence>(0xfe0ce); | |||
| MockDevice* mock_device = reinterpret_cast<MockDevice*>(device); | |||
| @@ -410,7 +413,8 @@ VkResult vkCreateFence(VkDevice device, | |||
| const VkFenceCreateInfo* pCreateInfo, | |||
| const VkAllocationCallbacks* pAllocator, | |||
| VkFence* pFence) { | |||
| *pFence = reinterpret_cast<VkFence>(0xfe0ce); | |||
| MockDevice* mock_device = reinterpret_cast<MockDevice*>(device); | |||
| *pFence = reinterpret_cast<VkFence>(new MockFence()); | |||
There was a problem hiding this comment.
You need to add an override to vkDestroyFence too so you aren't leaking these.
| } | ||
|
|
||
| private: | ||
| vk::Result result_ = vk::Result::eSuccess; |
There was a problem hiding this comment.
You should make this atomic since potentially people are reading and writing from this at the same time.
| MockFence() = default; | ||
|
|
||
| // Returns the result that was set in the constructor or |SetStatus|. | ||
| VkResult GetStatus() { return static_cast<VkResult>(result_); } |
There was a problem hiding this comment.
nit: I think vk::Status has cast operators to make this conversion implicit
|
I wonder if I should just abandon this PR and merge it into #45870 since it has 2/3 of the suggestions you mentioned. @gaaclarke wdut? |
Whatever is easier for you. That PR doesn't look unwieldy yet. |
|
Sg, let's do that. Closing this one, thanks Aaron! Replaced by #45870. |
I admit I'm not sure how to do "stacked" PRs properly, so this will look goofy until @gaaclarke merges #45834.
I considered a few different ways to do this, but figured this gives maximum flexibility without needing a lot of API space.
Wdut?
/cc @jonahwilliams