Add support for global lights #2548
Conversation
| * @param global if true, the light affects the entire tree from the root node, | ||
| * otherwise it only affects the children of the node in which it is attached. | ||
| */ | ||
| public DirectionalLight(boolean global) { |
There was a problem hiding this comment.
i didn't make a setter for the global param, to make it defacto immutable.
This makes implementation much easier and performant, since we never need to check if the flag changed.
| * @param filter an optional filter to apply to the lights | ||
| * (null means no filtering) | ||
| */ | ||
| public void update(LightList local, LightList parent, Predicate<Light> filter) { |
There was a problem hiding this comment.
added a filter here so that we can exclude global lights from being added the children's internal world lights list.
That's because the rootNode propagates down all the global lights, so when a global light is owned by a children and it is propagated to the same children later, we would end up with the children having two references to the same light, one from its internal local light list and one from the rootNode global light list.
To fix that we would need to deduplicate the lights in the list, that is not great for performances, so instead i've decided to use this filter based approach
| } | ||
| sb.append("]"); | ||
| return sb.toString(); | ||
| } |
There was a problem hiding this comment.
this is unrelated, but helped with the debug, so i've just left it here
| refreshFlags &= ~RF_GLOBAL_LIGHTS; | ||
| // if root node, we collect the global lights | ||
| if (getParent() == null){ | ||
| depthFirstTraversal(sx->{ |
There was a problem hiding this comment.
this is the traversal path, it is heavy, but only needs to happen if
- you have global lights in the scene
- light lists need a refresh
There was a problem hiding this comment.
And from my reading only happens if a global light is literally removed or added (not moved, as I was initially worried might be the case)
There was a problem hiding this comment.
yes, only if added or removed
| while (p != null) { | ||
| if ((p.refreshFlags & RF_CHILD_LIGHTLIST) != 0) { | ||
| // The parent already has this flag, | ||
| // so must all ancestors. | ||
| return; | ||
| } | ||
| p.refreshFlags |= RF_CHILD_LIGHTLIST; | ||
| if (hasGlobalLights) { | ||
| p.refreshFlags |= RF_GLOBAL_LIGHTS; | ||
| } |
There was a problem hiding this comment.
this propagates the flag upstream
|
Very cool! I have often wanted lights to behave exactly like this. This avoids a lot of messing about and booking keeping with lights attached to the root node but positioned elsewhere |
| LightList childLights = sx.getLocalLightList(); | ||
| for (Light l : childLights) { | ||
| if (l.isGlobal()) { | ||
| worldLights.add(l); |
There was a problem hiding this comment.
I'm trying to think about ways to avoid this full traversal. Adding new lights is probably easy; only track down paths with RF_GLOBAL_LIGHTS looking for new lights. The getting rid of old lights is harder, I can't see anything in Light that records where a light came from (which would make removing old ones much easier)
There was a problem hiding this comment.
Good idea!
Check the last commit. The refresh flag was set after the light was removed, so it wasn't even refreshing the global lights properly, i've changed the order now.
This means also that we have a path with RF_GLOBAL_LIGHTS for removed lights, this should make what you've proposed possible, right?
The rootNode will just see a RF_GLOBAL_LIGHTS and rebuild the list and the removed global light won't be there anymore because it isn't attached anywhere anymore.
|
Thanks for this! This is definitely something I wish I had.
Come to think of it, I think this may actually be a performance problem. JME currently uploads a new light buffer for each geometry, so if there are 5 global lights and 100 geometries, roughly 8kb of light data is being sent to the gpu every frame. It would probably be much faster to upload all lights once using a UBO or SSBO (maybe using a byte/short buffer for selecting local lights in-shader, or forget local lights altogether). Plus, we're also recompiling all shaders whenever a light is added or removed, so SSBOs would fix that, too. |
|
It depends. But if you have lights affecting small parts of the scene you will end up processing a lot more lights than what you need in every fragment. In lowend gpus or old opengl with no UBO support, the current jme approach is much better. So I'd say it depends on how you build the scene and what your target gl version and platform is. The current approach allows for more optimization, but is probably outdated with modern expectations and necessities.
I think this is the case only if you change the light batch size in every frame in single pass lighting, and never the case in multipass lighting. |
|
@richardTingle check the last commit, i think i've implemented your suggestion. |
|
@riccardobl Nice! I'm going to write some screenshot tests for global lights and that will double as some testing |
|
I also agree this would be a great addition. I've previously had to use a hacky workaround for global lights in my custom editor, where I flag lights as "global" with userData and then move them to the rootNode if they have that userData. But then they also need moved back and forth anytime the scene is saved/loaded to make sure the light is actually attached to the scene thats being saved (and not lost in the rootNode). So I'm looking forward to this PR for sure! It will definitely be a useful feature. |
|
@riccardobl I'm struggling to get this working. I've created the beginnings of a test at https://github.com/richardTingle/jmonkeyengine/blob/globalights-tests/jme3-screenshot-tests/src/test/java/org/jmonkeyengine/screenshottests/light/GlobalLights.java I create two nodes with a red local light and a green global light. Two white cubes are added, one a sibling of the lights, the other on a different branch. I expect the sibling to be illuminated by both and the non sibling to only be illuminated by the global light. But the global light doesn't seem to affect either cube. 
I've experimented and I think that's because it is attached from the beginning. I get different behaviour if I add it later 
But it still doesn't illuminate the other cube |
|
Thanks for the test case, i'll look into it. |
|
I think it is fixed now, basically the problem was that the previous behavior had the the refresh flag being propagated up the branch, but it didn't take into consideration that if the scene has multiple branches the flag must be propagated down to all the other branches too |
JME lights only affect children of the nodes to which they are attached.
This makes sense from a performance/optimization standpoint, but it does not reflect the behavior of blender and other renderers.
This PR adds support for global lights ie. lights that affect all the spatials up to the rootNode.
Lights can be marked as global by setting to true the "global" boolean flag on the lights' constructors . The flag is false by by default, so existing scene won't be affected.
This implementation exploits the jme scenegraph update logic to make global lights possible without any change to the renderer, this is an overview of how it works:
RF_GLOBAL_LIGHTSis added.