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

iOS metal seems to require compile-time constant sampler for sample_compare #533

Closed
cyh36 opened this issue Apr 13, 2018 · 8 comments · Fixed by #539
Closed

iOS metal seems to require compile-time constant sampler for sample_compare #533

cyh36 opened this issue Apr 13, 2018 · 8 comments · Fixed by #539
Labels
bug Feature which should work in SPIRV-Cross does not for some reason.

Comments

@cyh36
Copy link

cyh36 commented Apr 13, 2018

Hi, I've raised an issue #529 several days ago. You said "no constexpr samplers are needed" because you tested on OSX, it seems that only iOS metal requires this. I upgraded my iOS system and Xcode to the latest version, but the issue still remains. Is there any way to deal with this problem? Otherwise, maybe I have to use sample instead of sample_compare on iOS.

@HansKristian-Work
Copy link
Contributor

If iOS truly requires this, MoltenVK would need to add emulation support here because SPIRV-Cross would need another interface which lets you set constant sampler state ahead of time. I don't think there is much I can do right now. Please file a bug on MoltenVK if you're using that.

@HansKristian-Work HansKristian-Work added the bug Feature which should work in SPIRV-Cross does not for some reason. label Apr 13, 2018
@alek314
Copy link

alek314 commented Apr 13, 2018

Can you add a new interface while remain backward compatibility? We can not do PCF shadow mapping without this feature in our in house engine, which has nothing to do with MoltenVK.
Thank you.

@HansKristian-Work
Copy link
Contributor

If you're using the API only, I could add something like:

void CompilerMSL::remap_constexpr_sampler(unsigned id, const MSLConstexprSamplerData &)

However, I don't know how this works in MSL, so please paste some example shaders how you'd like the resulting MSL code to look.

@cyh36
Copy link
Author

cyh36 commented Apr 13, 2018

The sampler remapped could be declared at the beginning of the entry point function, and passed to those functions that use this sampler. Just like:

#pragma clang diagnostic ignored "-Wmissing-prototypes"

#include <metal_stdlib>
#include <simd/simd.h>

using namespace metal;

struct main0_in
{
    float3 vUV [[user(locn0)]];
};

struct main0_out
{
    float FragColor [[color(0)]];
};

float Samp(thread const float3& uv, thread depth2d<float> uTex, thread sampler uSamp)
{
    return uTex.sample_compare(uSamp, uv.xy, uv.z);
}

fragment main0_out main0(main0_in in [[stage_in]], depth2d<float> uTex [[texture(0)]], depth2d<float> uTex2 [[texture(1)]], sampler uSamp [[sampler(0)]], sampler uSamp2 [[sampler(1)]])
{
    constexpr sampler uSamp_(address::clamp_to_edge, mip_filter::none, min_filter::linear, mag_filter::linear, compare_func::less);
    constexpr sampler uSamp2_(address::clamp_to_edge, mip_filter::none, min_filter::nearest, mag_filter::nearest, compare_func::less_equal);

    main0_out out = {};
    out.FragColor = uTex.sample_compare(uSamp_, in.vUV.xy, in.vUV.z);
    out.FragColor += Samp(in.vUV, uTex2, uSamp2_);
    return out;
}

@HansKristian-Work
Copy link
Contributor

Ok, that seems reasonable enough.

@HansKristian-Work
Copy link
Contributor

HansKristian-Work commented Apr 17, 2018

Got something WIP that should solve your problem. Please check out the PR and give feedback if this fixes your issue.

@alek314
Copy link

alek314 commented Apr 18, 2018

We will test it ASAP

@cyh36
Copy link
Author

cyh36 commented Apr 18, 2018

This seems to resolve the issue we had. Thanks very much! @HansKristian-ARM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Feature which should work in SPIRV-Cross does not for some reason.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants