-
Notifications
You must be signed in to change notification settings - Fork 67
Rework environment map importance sampling to vulkan and hlsl #946
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
base: master
Are you sure you want to change the base?
Conversation
| namespace workgroup | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong namespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nbl::hlsl::concepts::accessors
| namespace workgroup | ||
| { | ||
| namespace envmap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're not in workgroup anything, also envmap should be image_importance_sampling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually nbl::hlsl::sampling and nbl/builtin/hlsl/sampling folder
| #define level NBL_CONCEPT_PARAM_T NBL_CONCEPT_PARAM_2 | ||
| #define offset NBL_CONCEPT_PARAM_T NBL_CONCEPT_PARAM_3 | ||
| NBL_CONCEPT_END( | ||
| ((NBL_CONCEPT_REQ_EXPR_RET_TYPE)((a.template get(uv,level,offset)) , ::nbl::hlsl::is_same_v, float32_t4>)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the same as the MipmappedLoadableImage accessor
make the get return a float32_t instead of float32_t4 then a new concept is justified.
| { | ||
|
|
||
| template <typename LuminanceAccessor, typename OutputAccessor NBL_FUNC_REQUIRES (envmap::LuminanceReadAccessor<LuminanceAccessor> && envmap::WarpmapWriteAccessor<OutputAccessor>) | ||
| // TODO(kevinyu): Should lumapMapSize and warpMapSize provided by Accessor? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not necessary, this is fine, make the const though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take the xi from the outside and never ask about the warpMapSize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also no write accessor necessary
| const uint32_t threadID = uint32_t(SubgroupContiguousIndex()); | ||
| const uint32_t lastWarpMapPixel = warpMapSize - uint32_t2(1, 1); | ||
|
|
||
| if (all(threadID < warpMapSize)) | ||
| { | ||
| float32_t2 xi = float32_t2(threadID) / float32_t2(lastWarpMapPixel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, threadID is 1D, AND its in 0,512
There's nothing "workgroup" about this.
Take the xi from the outside and never ask about the warpMapSize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also when you move the code outside, leave comments about why the xi calculation is like that (corner sampled images)
| namespace envmap | ||
| { | ||
|
|
||
| struct WarpmapGeneration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to Image , this could be used to importance sample without a warpmap given a xi like all the other sampling structs in nbl/builtin/hlsl/sampling
warpmap just caches the results in a LUT for a grid of xi inputs
| // TODO(kevinyu): Implement findMSB | ||
| const uint32_t2 mip2x1 = findMSB(lumaMapSize.x) - 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe ask for the uint32_t lumaMapWidthLog2 and bool aspectRatio2_1 instead of lumaMapSize
| // do one split in the X axis first cause penultimate full mip would have been 2x1 | ||
| p.x = impl::choseSecond(luminanceAccessor.get(uint32_t2(0, 0), mip2x1, uint32_t2(0, 0)), luminanceAccessor.get(uint32_t2(0, 0), mip2x1, uint32_t2(1, 0), xi.x) ? 1 : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to support octahedral maps (which are 1:1 apsect raito) only do this split if (aspectRatio2_1), and initialize p.x to 0 at the time p.y gets initialized
| } | ||
| } | ||
|
|
||
| const float32_t2 directionUV = (float32_t2(p.x, p.y) + xi) / float32_t2(lumaMapSize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments, comments, lets not let the discord discussions go to waste
| } | ||
|
|
||
| const float32_t2 directionUV = (float32_t2(p.x, p.y) + xi) / float32_t2(lumaMapSize); | ||
| outputAccessor.set(threadID, directionUV); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't use an output accessor here because we just want a sampling function, so just return directionUV
| const float32_t4 values = float32_t4( | ||
| luminanceAccessor.get(p, i, uint32_t2(0, 1)), | ||
| luminanceAccessor.get(p, i, uint32_t2(1, 1)), | ||
| luminanceAccessor.get(p, i, uint32_t2(1, 0)), | ||
| luminanceAccessor.get(p, i, uint32_t2(0, 0)) | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the get signature should be different, should be as if a textureGatherLod existed (which allows accessor to use textureGather + descriptor indexing for the mip-levels internally)
Description
Rework environment map importance sampling to vulkan and hlsl
Testing
Rework example 0 to use vulkan and hlsl
TODO list: