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

Support KHR Lights Punctual Extension #7

Closed
Skylion007 opened this issue Oct 3, 2020 · 14 comments
Closed

Support KHR Lights Punctual Extension #7

Skylion007 opened this issue Oct 3, 2020 · 14 comments

Comments

@Skylion007
Copy link

Skylion007 commented Oct 3, 2020

@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?

@hybridherbst
Copy link

@Skylion007 to finalize the merge of the other PRs and also for this one, could you

  • point me to the spec, and ideally a list of current implementers (where does/does this not work)
  • some sample files that show what these extensions add (e.g. export result changes with PR) + where to test the resulting glb files
  • some screenshots

That would help a lot. Thanks!

@Skylion007
Copy link
Author

Skylion007 commented Oct 3, 2020

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.

@Skylion007
Copy link
Author

@Skylion007
Copy link
Author

Screenshots can also be found in the above thread.

@hybridherbst
Copy link

Thank you. For my future self, here's the direct links to the GLB and the Readme .

Also for future me testing this, the Khronos reference image is a bit misleading since it doesn't use any IBL and has unclear exposure...

Khronos Reference Image:
image

Babylon (traces of blue, no red):
image

ModelViewer:
image

@hybridherbst
Copy link

@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!

@hybridherbst
Copy link

@plepers Do I understand correctly that plepers:feature/KHR_lights_punctual_Serialisation so far only implements the serialization part, but not the actual connection to Unity lights? Do or @Skylion007 know if there's a fork:branch that has that already?

@Skylion007
Copy link
Author

Skylion007 commented Oct 4, 2020

@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

@Skylion007
Copy link
Author

@hybridherbst Here is the original issue to support the extension btw: KhronosGroup#324

@hybridherbst
Copy link

@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.

@Skylion007
Copy link
Author

@calderarchinuk do you have the changes you made the Unity side available too on that commit?

@calderarchinuk
Copy link

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.

@hybridherbst
Copy link

From my (very limited) tests it looks like that worked, thanks for pointing me to the right file! I extracted the relevant bits and pieces related to lighting (and some other random stuff that looked useful).

Left: Babylon Sandbox with shadows enabled, Right: Unity
image

Importing doesn't look right yet though, but exporting seems to be fine.

@hybridherbst
Copy link

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.

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