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

Convention for directional lights in OpenGL-Binding for GLSL #4275

Closed
KnutHartmann opened this Issue Feb 7, 2016 · 6 comments

Comments

Projects
None yet
3 participants
@KnutHartmann

KnutHartmann commented Feb 7, 2016

The standard GLSL shader uses the homogeneous component of the uniform variable lightPosition to differentiate between point and directional light.

uniform vec4 lightPosition

core/src/processing/opengl/shaders/LightVert.gls
line 101: bool isDir = zero_float < lightPosition[i].w;

Many textbooks in computer graphics suggest to differentiate between points and vectors by setting the homogeneous component to 1 for points and to 0 for directions.

This is conform with the normalization = division by the homogeneous component.

All the examples in other GLSL shader NOT using processing looks likes this:

if ( lightPosition.w == 0.0 ) {
// it is a directional light
// get the direction by converting to vec3 (ignore W) and negate it
vec3 lightDirection = -lightPosition.xyz
} else {
// point light
}

http://www.tomdalling.com/blog/modern-opengl/08-even-more-lighting-directional-lights-spotlights-multiple-lights/

My processing GLSL shader code looks likes this:

if ( lightPosition.w != 0.0 ) {
// it is a directional light
direction = -lightNormal;
} else {
// point light
direction = lightPosition.xyz - position.xyz;
}

Could you take over the convention w == 1 for point lights to minimize the confusion of cross-plattform programmers?

@codeanticode

This comment has been minimized.

Show comment
Hide comment
@codeanticode

codeanticode Feb 7, 2016

Member

@KnutHartmann I think this would be a reasonable improvement so Processing follows the accepted convention on the use of the w coordinate to differentiate directional from point lights. I don't see any unintended consequences from swapping the current setting in the light shaders, as these values are not used for anything else. Thanks for bringing this up!

@JakubValtar any comments?

Member

codeanticode commented Feb 7, 2016

@KnutHartmann I think this would be a reasonable improvement so Processing follows the accepted convention on the use of the w coordinate to differentiate directional from point lights. I don't see any unintended consequences from swapping the current setting in the light shaders, as these values are not used for anything else. Thanks for bringing this up!

@JakubValtar any comments?

@codeanticode

This comment has been minimized.

Show comment
Hide comment
@codeanticode

codeanticode Feb 29, 2016

Member

I went ahead and made the switch with commit 59dd00f, as it does not break any API and the shader code follows accepted CG conventions.

Member

codeanticode commented Feb 29, 2016

I went ahead and made the switch with commit 59dd00f, as it does not break any API and the shader code follows accepted CG conventions.

@hype

This comment has been minimized.

Show comment
Hide comment
@hype

hype Aug 22, 2016

grrrr - this change means all my projects running in 3.0 work fine... but now break in all other 3.1+ versions - meaning I have to now update ALL my GLSL - damn you... and your convention standards... lol

hype commented Aug 22, 2016

grrrr - this change means all my projects running in 3.0 work fine... but now break in all other 3.1+ versions - meaning I have to now update ALL my GLSL - damn you... and your convention standards... lol

@hype

This comment has been minimized.

Show comment
Hide comment
@hype

hype Aug 22, 2016

search and replace... 637 .GLSL files... ha ha ha.

hype commented Aug 22, 2016

search and replace... 637 .GLSL files... ha ha ha.

@codeanticode

This comment has been minimized.

Show comment
Hide comment
@codeanticode

codeanticode Sep 2, 2016

Member

sorry... there should any further changes in the shader API... unless we move to Vulkan :-)

Member

codeanticode commented Sep 2, 2016

sorry... there should any further changes in the shader API... unless we move to Vulkan :-)

@hype

This comment has been minimized.

Show comment
Hide comment
@hype

hype Sep 2, 2016

:) .... and iiiiiiiiiiiii eeee iiiiiiiiiiii will always love you @codeanticode

hype commented Sep 2, 2016

:) .... and iiiiiiiiiiiii eeee iiiiiiiiiiii will always love you @codeanticode

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