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 util functions to fill foveation pattern bitmaps #485

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

jorgeag-google
Copy link
Collaborator

@jorgeag-google jorgeag-google commented Jun 25, 2024

The first of a series of CL that integrates the commits from:

https://github.com/bjoeris/bigwheels/tree/experimental-foveation

into BW main's branch.

The series will have 4 CL, this is the first ona that integrates the following commit

  • Add util functions to fill foveation pattern bitmaps
@apazylbe
Copy link
Collaborator

Thanks Jorge! Any chance we can split this into separate PRs for each commit/meaningful change instead? This much code is a bit daunting to review.

@jorgeag-google
Copy link
Collaborator Author

I have split the PR into the original commits.
The last one also included my won addition to integrate.
I am looking into the shader compilation problem on Linux

@apazylbe
Copy link
Collaborator

Since you already have this in separate commits, would it be possible to split into PRs?
It would be much easier/faster to review, and address review feedback, feedback is supposed to be addressed as separate commits.

@jorgeag-google
Copy link
Collaborator Author

I follows Aliya's advice, I update this PR to contain only one of the CL in the original merge.
I create the nexts ones when this gets merged.

@bjoeris
Copy link
Collaborator

bjoeris commented Jul 1, 2024

Thanks Jorge for taking care of merging these.

I don't remember exactly what benchmarking you're planning to do, so I just want to make sure these functions are actually needed. I added them specifically to test FDM and VRS independent of OpenXR foveation. These let the application use it's own density patterns.

In normal OpenXR foveation, the foveation pattern comes from OpenXR, via the application via XrSwapchainImageFoveationVulkanFB in the next chain of xrEnumerateSwapchainImages. For this use case, we would want to build a ShadingRatePattern around the density image from OpenXR.

@jorgeag-google
Copy link
Collaborator Author

jorgeag-google commented Jul 1, 2024

Hi Benson,
Yes, the idea is to port all your changes that lead to the foveation_benchmark app.
To my undestanding these is a CL that is part of four required ones (correct me if I am worng here)
I want to integrate all four of these:

  1. Add util functions to fill foveation pattern bitmaps
  2. Add BlitImage (Vulkan Only)
  3. Add foveation benchmark
  4. Add multisample support for ShadingRatePattern.
    Since they are al required to get the app working.

To answer the other part of your your comment, I want fixed foveated rendering as a first step. So, if I am able to set a fix pattern of two concentric elipses manually, that will be perfect for my use case.
Maybe later I would want to query the pattern from the app using OpenXR, but in the meantime fixed patter will work fine.

Copy link
Collaborator

@bjoeris bjoeris left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@apazylbe apazylbe left a comment

Choose a reason for hiding this comment

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

thank you! lgtm, just a few comments. Also could you update the PR title and description?

include/ppx/grfx/grfx_shading_rate_util.h Outdated Show resolved Hide resolved
include/ppx/grfx/grfx_shading_rate_util.h Outdated Show resolved Hide resolved
src/ppx/grfx/grfx_shading_rate_util.cpp Outdated Show resolved Hide resolved
@apazylbe apazylbe requested a review from Keenuts July 2, 2024 14:57
@jorgeag-google jorgeag-google changed the title Integrate experimental-foveation branch Jul 2, 2024
Copy link
Collaborator

@apazylbe apazylbe left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!
By the way, if possible, next time it would be great to address review feedback in separate commits https://github.com/google/bigwheels/blob/main/CONTRIBUTING.md#for-developers-contributing-a-patch

include/ppx/grfx/grfx_shading_rate_util.h Show resolved Hide resolved
@jorgeag-google jorgeag-google merged commit 9ab56d9 into google:main Jul 2, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants