-
Notifications
You must be signed in to change notification settings - Fork 41
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
Support KHR Lights Punctual Extension #7
Comments
@Skylion007 to finalize the merge of the other PRs and also for this one, could you
That would help a lot. Thanks! |
Here is the spec: https://github.com/KhronosGroup/glTF/tree/master/extensions/2.0/Khronos/KHR_lights_punctual I have yet to find a implementation that doesn't support this extension as it's one of the most common if not the most common extension (except for software that doesn't support lights at all).. ASSIMP, TinyGLTF, Magnum, Blender, ThreeJS, FBX2GLTF, etc.. all support it. The screenshots seem a bit too trivial TBH as with the extension, these renderers can load punctual lights (Spotlights, etc). and without it, they can't. |
@hybridherbst Here is a sample lit GLB: https://github.com/KhronosGroup/glTF-Sample-Models/pull/210/files |
Screenshots can also be found in the above thread. |
@Skylion007 have you observed functional differences between the lights feature from plepers and from CognitiveVR? The latter is unfortunately hard to merge as it's a bazillion changes throughout the repo in a single "add lights" commit. FlorentinMonteil's fork seems to be on par with plepers; I looked at other changes that plepers did and think most of those should be covered in our fork already (minus the ones that are purely hacks, e.g. adding a rotated subnode). Let me know if you find specific stuff that's missing though! |
@plepers Do I understand correctly that |
@hybridherbst BTW, one of the screenshots you posted has Babylon.JS incorrectly configured (lighting has to be enabled): KhronosGroup/glTF-Sample-Models#210 (comment) I think the CognitiveVR one I linked might implement reading them too? But it's messy (has a lot of comments / fixmes in the code): CognitiveVR@76bb1cb |
@hybridherbst Here is the original issue to support the extension btw: KhronosGroup#324 |
@Skylion007 I wouldn't say "wrongly configured", the sample file just doesn't work good with IBL on (which nearly every usecase would have). That @CognitiveVR commit only touches the dll side, there are no changes in the actual unity exporter/importer code unfortunately that I'm seeing. While I'm happy to merge (and clean up, which I started) lights support I don't think I'll find time to implement the Unity side in the foreseeable future. If you can convince someone to share that (I kind of doubt they added extension support without using it) I'm happy to merge it in. |
@calderarchinuk do you have the changes you made the Unity side available too on that commit? |
The Unity side changes are in our Unity SDK available here. The implementation on both Unity and the dll are a bit messy (as I'm sure you've seen) but it seems to be working fine for our use case. Let me know if you have any issues getting the light exporting as nodes. |
I believe importing is now correct as well. The KHR_lights_punctual implementation in UnityGltf is a mess (seems there's two complete extensions, for each of them one half is used for import and export respectively), but functional. |
@hybridherbst Thanks for merging in most of the branches I mentioned previously. Now that those easy bugfixes are merged in, might be time to go look for additional features / specifications that were added by PRs.
In particular, I have found the KHR Lights Punctual Extension to be a very useful feature to support and several people have merged in @plepers implementation.
Here is the link to create the PR: development-3...plepers:feature/KHR_lights_punctual_Serialisation
There is also this implementation of it:
CognitiveVR@76bb1cb
With lighting data supported, that should makes these exported GLTFs support most important Unity properties. Exporting lighting data would be one of the last features needed to bring up UnityGLTF to parity with FBXExporter.
There may be some other bugfixes and features from FlorentinMontiel's fork that could be useful to merge in as well.
FlorentinMonteil@c5349dd
I think that should be pretty much complete GLTF Support at least for export (I am not sure all those extensions are working for importing too).
Thoughts?
The text was updated successfully, but these errors were encountered: