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

[SUGGESTION] Control compatibility level for method implementations. #81

Open
thargy opened this issue Jun 6, 2018 · 3 comments
Open

Comments

@thargy
Copy link
Contributor

thargy commented Jun 6, 2018

As I've been going through #65, (see GH-77 and GH-80), and making methods consistent, it has occurred to me that, there is a concept of how compatible you want implementations to be across backends.

For example, the Pow method on Vulkan ignores the sign of the first parameter, whereas all other backends will return a NaN for -ve values. To make compatible, I had to wrap the input parameter with an abs().

In some scenarios, this optimisation is a cost that developers may not wish to pay - particularly if the input is never intended to be negative. There are numerous other examples.

There are several ways to resolve this:

  • Along with [SUGGGESTION] ShaderBuiltins should be split #78 (which I still think we shoul do to distinguish GPU-exclusive methods from GPU/CPU methods), add a new FastBuiltins class which implements the same methods as Builtins but where implementations are only consistent within certain input ranges (which can be documented in the intelli-sense for those functions). This allows for quick switching between 'accurate' and 'fast' compatibility in a single file, but you can still mix and match by using explicit static invocation, or by explicit using statements.
  • Provide method overloads which are less fussy, e.g. PowFast() as well as Pow(), this prevents name collisions when using using static, and is more visible, it requires manually changing invocations when required, which is no bad thing as care should be taken when using a fast implementation anyway.
  • Use an attribute on the class/method to indicate compatability level.
  • Add transpiler options for compatability, which can be passed in on build.
@mellinoe
Copy link
Owner

mellinoe commented Jun 6, 2018

I prefer the second option (PowFast vs Pow), personally. @tgjones Any thoughts?

@tgjones
Copy link
Contributor

tgjones commented Jun 7, 2018

I like the second option too.

There's some potential for confusion, because HLSL at least already has a similar concept (ddx_coarse vs ddx vs ddx_fine), but good IntelliSense comments can help.

@thargy
Copy link
Contributor Author

thargy commented Jun 7, 2018

I also prefer option 2, however, it would be nice for the auto-generated tests to recognise the limits of the functions, which I could do by adding attributes? The benefit of describing behaviour via attributes is that this will eventually allow the analysis path to perform more detailed value analaysis and issues warnings/failures when it sees the methods inappropriately used.

Eventually, I'd like to see the the analyser used to provide realtime intellisense whilst developing shader code, which should be relatively easy considering it uses Roslyn.

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

No branches or pull requests

3 participants