Add depth unprojection support to batch renderer#2129
Merged
Conversation
facebook-github-bot
added
the
CLA Signed
jturner65
reviewed
Jun 13, 2023
11 tasks
5 tasks
mosra
reviewed
Jun 15, 2023
Contributor
mosra
left a comment
mosra
left a comment
There was a problem hiding this comment.
22 comments, sorry, I'll stop now 😅
| /* Combined view and projection matrices. Updated from updateCamera() */ | ||
| Cr::Containers::Array<ProjectionPadded> cameraMatrices; | ||
| /* Unprojection for cameras. Updated from updateCamera() */ | ||
| Cr::Containers::Array<Mn::Vector2> cameraUnprojections; |
Contributor
There was a problem hiding this comment.
Couldn't this be put into the Scene struct among all other per-scene data? Feels unnecessary to allocate a dedicated array for these.
Contributor
Author
There was a problem hiding this comment.
I'm inclined to leave this as-is until we have multi-sensor support because it'll be more straightforward to refactor per-sensor data.
Contributor
There was a problem hiding this comment.
it'll be more straightforward to refactor
I don't know :) There most probably will be another array, yes, but that's about it, its size and contents will be significantly different. In the context of this PR it's unnecessary noise.
mosra
reviewed
Jun 20, 2023
mosra
reviewed
Jun 20, 2023
mosra
reviewed
Jun 20, 2023
Contributor
mosra
left a comment
mosra
left a comment
There was a problem hiding this comment.
Final set of code golf suggestions because this was bothering me :P
mosra
reviewed
Jun 23, 2023
mosra
force-pushed
the
batch-renderer-depth-unprojection
branch
from
1ae1473 to
b100aad
Compare
Contributor
|
Did the two remaining requested changes myself, see the commits below for details. |
mosra
approved these changes
Jun 25, 2023
No need to maintain a whole new array for it.
mosra
force-pushed
the
batch-renderer-depth-unprojection
branch
2 times, most recently
from
58f56ed to
ec15aac
Compare
My suggestion was meant to *replace* the original and adapt the code as necessary, not add a new overload without actually testing any of it. Due to the copypaste the new overload also had no FMV attribute applied, meaning it suddenly got significantly slower for no reason.
Takes too long in Debug.
mosra
force-pushed
the
batch-renderer-depth-unprojection
branch
from
ec15aac to
46b8286
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation and Context
This changeset adds support for unprojecting depth within the batch renderer.
In this context, unprojected depth means that each pixel of the depth image represents the distance in meters from the camera, rather than the raw depth buffer.
To accomplish this,
DepthUnprojectionis now shared by the classic and the batch renderer. To preserve batch renderer isolation, it was moved to the batch renderer along with the shaders.Summary:
gfxandgfx_batch.gfxshaders can referencegfx_batchshaders, but not the other way around.DepthUnprojectionis moved togfx_batchHow Has This Been Tested
Tested locally and on CI.
The following tests are added:
GfxBatchRendererTestBatchReplayRendererTestTypes of changes
Checklist