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

Add atan signature with two input #116

Merged
merged 2 commits into from
Aug 7, 2019
Merged

Add atan signature with two input #116

merged 2 commits into from
Aug 7, 2019

Conversation

nkint
Copy link
Contributor

@nkint nkint commented Aug 7, 2019

To be a little bit more compliant with GLSL spec atan function has two signatures.
See here: https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/atan.xhtml

To be a little bit more compliant with GLSL spec `atan` function has two signatures.
See here: https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/atan.xhtml
@postspectacular
Copy link
Member

Thank you, @nkint - I was aware of the two arities, but settled for the 1-arg version so far, since the other sig would also need special treatment in the JS code gen and I was hoping to work on this as part of #96. So I'd propose we do support the 2 arities of the atan builtin, but emit only the single arg version (by first dividing a over b, if needed), at least for now:

export function atan<T extends Prim>(a: Term<T>): FnCall<T>;
export function atan<A extends Prim, B extends A>(a: Term<A>, b: Term<B>): FnCall<A>;
export function atan(a:Term<any>, b?: Term<any>) {
    return b
        ? builtinCall("atan", a.type, div(a, b))
        : builtinCall("atan", a.type, a);
}

@postspectacular
Copy link
Member

postspectacular commented Aug 7, 2019

Actually, since the 2 atan versions have different output ranges [-π/2, +π/2] (single arg) vs [-π, +π] (2 args), it should be like this:

export function atan<T extends Prim>(a: Term<T>): FnCall<T>;
// prettier-ignore
export function atan<A extends Prim, B extends A>(a: Term<A>, b: Term<B>): FnCall<A>;
export function atan(a:Term<any>, b?: Term<any>) {
    return b
        ? mul(builtinCall("atan", a.type, div(a, b)), 2)
        : builtinCall("atan", a.type, a);
}

(also adding prettier-ignore to keep fn overloads more easy on the eye)

@postspectacular
Copy link
Member

postspectacular commented Aug 7, 2019

This will make it temporarily minutely slower, but at least is correct... and once the function dispatch updates (#96) are done, we can update this here once more

@postspectacular
Copy link
Member

Soo... scrap my 2 last comments, the differences are (sadly) quite different. See this little comparison I just made: http://glslsandbox.com/e#56557.0

Screen Shot 2019-08-07 at 12 17 04 PM

So, let's just keep your version and I will add a note to the readmes that atan w/ 2 args is not supported by the JS code gen yet...

@postspectacular postspectacular merged commit be94f59 into thi-ng:master Aug 7, 2019
@postspectacular
Copy link
Member

Also just once more for reference (note to future self): https://en.wikipedia.org/wiki/Atan2#Definition_and_computation

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.

None yet

2 participants