-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Conversation
Thanks @Reon90. This seems a little short on details. The README should probably answer questions such as: What is it trying to show? What constitutes successful rendering vs not? How can the viewer see if the effect of each light source ended up in the correct location or orientation? |
I tried displaying Lights.gltf in several libraries. Three.js + Lights.gltf result: Babylon.js + Lights.gltf result: I do not know which is the correct result. |
@cx20 but I made screen shot in blender 2.80, can't we trust it? |
@Reon90 I think that having Blender's source model will help understanding. |
This model seems to contain four lights, but it seems that the red point light is not displayed in each viewer.
Perhaps the distance of the light may be far. |
@cx20 I made point light closer. Babylon.js: |
@Reon90 I could not find a model in Is the file correct? (Because I am not familiar with Blender, it may be misunderstanding.) |
@cx20 I think this model can open only in blender 2.80, but your version is 2.79. |
@Reon90 Thanks. I was able to open that model with Blender 2.80 Beta. |
Unfortunately I don't think we can use Blender for the canonical screenshot here. There are open questions about the physical basis of their analytical lights' intensity (KhronosGroup/glTF-Blender-IO#24). I'll fix the issue in the three.js rendering shortly. |
@donmccurdy In the updated model the position of the |
@06wj I tried the display of Lights.gltf in Hilo3d online viewer. However, the light seems to be too bright. Please confirm. |
@cx20 When the range is not defined, Hilo's light is not attenuated, so the light will be very bright. I'll fix it. |
@cx20 Fixed it. |
What about this pull request? Is there any concerns now? |
I have mild concerns that it's not easy for a casual observer to tell from a glance what the correct rendering of this is supposed to be. But it seems like several bugs have already been found & fixed here, so it's probably worth merging. |
I tried this model with gltf-test. |
This PR has been around for a while, any hope of getting it merged? Would be nice to have a test for punctual lights. |
Along the lines of @emackey's comment #210 (comment), I'm not sure this is a clear enough test of punctual light support. I'd prefer if we could create something that is more self-explanatory and unambiguous. An example from KhronosGroup/glTF#1223 (which probably needs to be recreated at this point) would be: |
Hi, I think that we need common visual criteria. Why don't you choose a reliable physically-based offline renderer for getting a "ground-truth quality" rendered image? I'm concerned about Blender's use of Watts (energy consumption? outgoing light energy?) for light strength. |
@Reon90 Please let me know if you remember. The point light for this Blender model is If the intensity values in the glTF file are more correct, we would be better off using the glTF Viewer results for the screenshots as well. |
@cx20 I believe that it was originally 5 because according to spec the units are not a Watt:
|
@Reon90 Thank you for your reply. I tried opening |
Yes, I think we were not sure of the units in Blender when KHR_lights_punctual support was written, but it's now clearly in watts. The bug to fix this (in Blender) is KhronosGroup/glTF-Blender-IO#564. |
@donmccurdy Thank you for the information. I understand why you said you can't adopt the Blender screenshot. |
I tried again to view this model in some glTF Viewer.
All the viewers seem to be generally correct, but the Khronos glTF Sample Viewer seems to have too much light. Edit 2020.08.21: Added Enterprise PBR Sample Renderer |
It would be great to merge this (unsure what's preventing this, other than a conflict that needs resolution?), as right now there are no other examples of this extension in the repository. |
@zeux My problem with this model is there's no easy way to tell if a viewer got it right or not. Lately we've been working to raise the bar for sample models, and I think this one is in need of a refresh, if not a redesign. It should clearly label the different light sources, and have some means of indicating the intended effects. It should also test to make sure that any angles or rotations that can be specified on lights have been implemented with the correct rotational direction and order of transformations. |
This can be a stretch goal (I'd certainly accept a sample without it) but aspirationally it would also be great to have a sample that confirms whether |
@emackey Mhm, I guess this makes sense although I wonder if presence of a test that exercises this functionality is better than absence of a test even if this is imperfect :) Unfortunately glTF-Asset-Generator doesn't contain a sample for this extension either which means that tools that want to make sure this extension is supported need to get the scene from somewhere else. |
I also think it would be better to have some kind of model to test this extension. A cooler model would be nice, but if it takes time to prepare, I think this model would be useful for minimal development. |
I agree with @cx20, it's important to at least have a simple smoke test for punctual lights. |
@Reon90 : Thre are conflicts in the branch that is used by this PR. Please resolve those conflicts and address any open concerns so that this PR can be merged. |
@DRx3D This PR has been open for >4 years. It is very likely that the person who opened is has moved on. Just in case: The conflict is only in the README (which is/was always an issue: Who is responsible for updating the tables? This will be sorted out in the new infrastructure). So the conflicting file could be ignored or resolved by just brushing the original README over the one that is changed here. |
@javagl : If you feel the content is right for the repo, I can bring it into the new repo so there is no need to merge it here, then fix the README and other associated files. |
My last comment was only about the fact that the merge conflict would become obsolete if this was just moved to the Beyond that, I cannot say whether this model is "suitable" as a sample model. I think there should be at least(!) one sample model for each extension. But in a recent comment @emackey raised some concerns about the model itself, so it might not be the best one to use here... |
I think we should probably not merge this PR. Instead we should provide a model that either clearly illustrates correct vs. incorrect implementation, or showcases the artistic value of the extension. Support for KHR_lights_punctual is much better in Blender today than it was when this PR was opened, so that shouldn't be as much work as it might have been previously. Getting consistent punctual lighting between authoring and rendering tools is not trivial, for reasons related to image formation concepts like exposure and tone mapping — we should not add a sample at all without addressing those issues, at least in the description. |
On the artistic side of https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/LightsPunctualLamp |
Spec