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

direction property missing in KHR Material Common/KHR lights spec. tables #1063

Closed
andreasplesch opened this issue Aug 6, 2017 · 9 comments

Comments

@andreasplesch
Copy link
Contributor

andreasplesch commented Aug 6, 2017

##This is a late note for the glTF 1.0 KHR Material Common extension spec. draft.

https://github.com/KhronosGroup/glTF/tree/master/extensions/Khronos/KHR_materials_common#directional
mentions a direction property as being described in table 2.

However, table 2 (https://github.com/KhronosGroup/glTF/tree/master/extensions/Khronos/KHR_materials_common#common-light-shared-properties) does not include such a property.

The direction is presumably the constant (unit) vector along which the light wave front propagates (imagening an infinitely distant light source).

Rather than a 3d vector, the direction could also be described as a rotation/quaternion of an initial (default) direction (often 0,0,-1). This may mean that the direction is superfluous as it could be defined as 0,0,-1 and then the light be rotated by an associated transform if necessary. (Perhaps this is the reason why direction was not specified in table 2??).

#947 has discussion on the glTF 2.0 version of the extension which presumably will include a similar table.

If there is interest, I could prepare a quick PR for table 2 as a starting point until 2.0 is fully established.

@stevepg
Copy link

stevepg commented Aug 7, 2017

personally, I don't think the material_common definition should include anything about lights.

@andreasplesch
Copy link
Contributor Author

andreasplesch commented Aug 7, 2017

I agree. I think I saw somewhere a separate KHR-lights extension for 2.0 (?). There may still be a need for an addendum for v.1.0 users since the draft here became the defacto standard. But there may be no appetite to put any effort into improving 1.0. In this case, this issue may become a reference and I propose direction to be specified as a unit 3d vector which can be used as the 'Light vector L' in the shading equations.

@andreasplesch
Copy link
Contributor Author

andreasplesch commented Aug 7, 2017

@donmccurdy
Copy link
Contributor

donmccurdy commented Aug 7, 2017

Yes, #945 is the best reference for KHR_lights. Direction is simply the orientation of the node to which the light is attached, so it doesn't require an extra property. Although probably the spec should define that a directional light's base direction is "down" (or whatever it actually is) -Z, and any parent node orientation transforms that.

@andreasplesch andreasplesch changed the title direction property missing in KHR Material Common spec. table 2 direction property missing in KHR Material Common/KHR lights spec. tables Aug 7, 2017
@andreasplesch
Copy link
Contributor Author

Ok, very helpful. That is a definite statement and needs to be explicit since currently the spec. does define an extra property, and I would have expected -Z as the base direction.

@donmccurdy
Copy link
Contributor

Oh, hah, well I could also be wrong... My implementation uses -Z as the base direction after all, and does not check for this mysterious direction property. In any case we should certainly clarify this. Realistically I think that clarification needs to happen in the KHR_lights extension, rather than amending the glTF1.0 spec at this point. Suggestions on any other issues with #945 are welcome, also. 🙂

@McNopper
Copy link
Contributor

McNopper commented Aug 7, 2017

It is -z, as stated here (see GL_POSITION section):

https://www.khronos.org/registry/OpenGL-Refpages/gl2.1/xhtml/glLight.xml

@pjcozzi
Copy link
Member

pjcozzi commented Dec 24, 2017

Is this now duplicate with #945?

@andreasplesch
Copy link
Contributor Author

Yes, let me close this. I think this is resolved as -z plus transform.

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

No branches or pull requests

5 participants