Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] [vulkan] Implement command pool and sampler library#35282

Merged
iskakaushik merged 3 commits intoflutter:mainfrom
iskakaushik:command-pool-vk
Aug 10, 2022
Merged

[Impeller] [vulkan] Implement command pool and sampler library#35282
iskakaushik merged 3 commits intoflutter:mainfrom
iskakaushik:command-pool-vk

Conversation

@iskakaushik
Copy link
Contributor

No description provided.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

static std::unique_ptr<CommandPoolVK> Create(vk::Device device,
uint32_t queue_index);

explicit CommandPoolVK(vk::UniqueCommandPool command_pool)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's OOL these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

return vk::Filter::eNearest;
case MinMagFilter::kLinear:
return vk::Filter::eLinear;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The pattern is to put an FML_UNREACHABLE at the end instead of a default of missing case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here and elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return vk::SamplerMipmapMode::eNearest;
case MipFilter::kLinear:
return vk::SamplerMipmapMode::eLinear;
default:
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of a default, explicitly list all cases and put an unreachable at the end. Since we are constantly evolving these APIs, its easier to find whats missing when we do add new enum values (hopefully fewer times now).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return nullptr;
}

auto mip_map = ToVKSamplerMipmapMode(desc.mip_filter);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be const too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@chinmaygarde chinmaygarde changed the title [impeller] [vulkan] Implement command pool and sampler library [Impeller] [vulkan] Implement command pool and sampler library Aug 10, 2022
@iskakaushik iskakaushik added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 10, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Aug 10, 2022

  • This commit has no checks. Please check that ci.yaml validation has started and there are multiple checks. If not, try uploading an empty commit.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 10, 2022
@iskakaushik iskakaushik merged commit 9a14d0b into flutter:main Aug 10, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 10, 2022
emilyabest pushed a commit to emilyabest/engine that referenced this pull request Aug 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants