Skip to content

Conversation

litherum
Copy link
Contributor

Flesh out the definition of WebGPUSamplerDescriptor

@litherum litherum requested review from Kangz, kainino0x and kvark August 25, 2018 00:38
Copy link
Contributor

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

LGTM although I don't remember off the top of my head whether every option is supported in every API. Assuming you or someone investigated this.

One comment

};

// Sampler
enum WebGPUFilterMode {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unintentional removal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's on line 121 below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, in that case, I assume you meant to remove the 3 lines below this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

....yep.

@litherum
Copy link
Contributor Author

Yeah, I did the investigation. The only thing that was non-obvious was the bias parameter - all the shading languages allow specifying the bias in the sample() function, but only HLSL and Vulkan allow setting the bias in the sampler. Emulating it using a constant buffer would be pretty easy in Metal, and we're likely going to have to create these invisible constant buffers anyway, so I figure we can just do that.

float lodMinClamp = 0,
float lodMaxClamp = FLT_MAX,
unsigned long maxAnisotropy = 1,
boolean normalizedCoordinates = true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't see something like this in D3D12_SAMPLER_DESC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, you’re right, I missed that. It would have to be emulated, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps, we should instead remove this for now?

Copy link
Contributor

@Kangz Kangz left a comment

Choose a reason for hiding this comment

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

LGTM too, we'll need to investigate the lodClamp.

@litherum
Copy link
Contributor Author

We should discuss emulation in the group.

@kvark
Copy link
Contributor

kvark commented Aug 27, 2018

The only thing that was non-obvious was the bias parameter - all the shading languages allow specifying the bias in the sample() function, but only HLSL and Vulkan allow setting the bias in the sampler.

Interestingly, we've been calling setLodBias on MTLSamplerDescriptor unconditionally and it didn't trigger an "unrecognized selector sent to an instance..." failure.

// Samplers
enum WebGPUAddressMode {
"clampToEdge",
"mirrorClampToEdge",
Copy link
Contributor

Choose a reason for hiding this comment

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

note: this requires "VK_KHR_sampler_mirror_clamp_to_edge". It's not supported on Mali/Adreno Android GPUs, and a few desktop ones (e.g. Kabylake GT2 on Linux). Perhaps we can get started without it?

boolean normalizedCoordinates = true,
WebGPUCompareFunction compareFunction = "never",
WebGPUBorderColor borderColor = "transparentBlack"
float bias = 0;
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 call it lodBias for better clarity

@litherum litherum merged commit e84ad46 into gpuweb:master Aug 27, 2018
@litherum litherum deleted the samplerdefinition branch August 27, 2018 20:37
@kainino0x
Copy link
Contributor

kainino0x commented Sep 11, 2018

Looks like the WebGPUCompareFunction was actually a duplication, I'm going to remove one of them in a cleanup commit to make this pass IDL validation again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants