Performance: LightFilter gets applied even if not needed

[zzuegg]

Hi, during my investigation on my deferred performance i found that the LightFilter gets applied on each RenderManager.renderGeometry()

I looked up on how Jme internally works around this issue when no lights are needed and it seems it does not.

It surely impacts the performance of the AbstractShadowRenderer. At least on objects outside of the view frustum but inside of the shadow frustum the light filtering is not needed.

A quick fix would be to implement:
tmp = RenderManager.getLightFilter(); RenderManager.setLightFilter(null); //Do your stuff RenderManager.setLightFilter(tmp);

what do you think? applying the easy fix or implement a fix at global scale?

[Ali-RS]

Probably you should ask on the forum as there will be a few more eyes to see it there.

But without knowing much about this, from what I understand when the light filter is removed then RenderManager will use all the lights in the geom.getWorldLightList() for rendering geometry. Is this what you want in your case? Wouldn't it be bad for performance?

I do not have enough knowledge in this area, but if it should be up to the caller to decide if it needs light filtering or not then what about introducing a new method?

RenderManager.renderGeometry(Geometry geom, boolean requiresLightFiltering)

otherwise, your quick fix sounds ok to me.

implement a fix at global scale?

Do you have another approach in mind?

[zzuegg]

But without knowing much about this, from what I understand when the light filter is removed then RenderManager will use all the lights in the geom.getWorldLightList() for rendering geometry. Is this what you want in your case? Wouldn't it be bad for performance?

The performance penalty comes from doing frustum checks for each light. I will have to dig deeper into the LightMode techniques to see if there is more to gain.
Not only my case would benefit from this fix. Any rendering technique that renders geometries from the scenegraph with lights attached suffers from this penalty. At very least the shadow renderers suffer from this.

but if it should be up to the caller to decide if it needs light filtering or not then what about introducing a new method?

In an optimal solution it would be up to the material.technique to decide if it needs light at all. But the technique gets selected after the lights are filtered so there is no way to get that info without major refactoring.

If this option is out of reach the next best solution would be that the caller gets an option to apply the the filters. a boolean would be nicer, but by setting LightFilter to null we kind of have that option.

Another, maybe better solution would be to create an NullLightFilter that is settable when not wanting filtration.

[Ali-RS]

Another, maybe better solution would be to create an NullLightFilter that is settable when not wanting filtration.

Will NullLightFilter return an empty LightList or world LightList?

[zzuegg]

Will NullLightFilter return an empty LightList or world LightList?

It will not add any lights to the filteredLightList but return immediately

[Ali-RS]

I see, and the filteredLightList is already empty at that point.
(note that, this is different from RenderManager.setLightFilter(null) as in setLightFilter(null) it will still pass the geometry world lights to the material but using NullLightFilter it will pass an empty light list)

jmonkeyengine/jme3-core/src/main/java/com/jme3/renderer/RenderManager.java

Lines 632 to 639 in 488d9fa

	// Perform light filtering if we have a light filter.
	LightList lightList = geom.getWorldLightList();
	if (lightFilter != null) {
	filteredLightList.clear();
	lightFilter.filterLights(geom, filteredLightList);
	lightList = filteredLightList;
	}

Can we expect a PR from you?

[zzuegg]

Yeah sure. I will make the changes once all agree on the design solutions.

Yeah, a NullFilter would be the better option.

I think we can assume that calls to rendermanager are single threaded?
If yes the NullFilter could offer some static utility functions. i am thinking of something like:
NullFilter.use(renderManager); NullFilter.revert(renderManager)

[Ali-RS]

NullFilter.use(renderManager); NullFilter.revert(renderManager)

Well, that seems backward to me as it is the RenderManager that uses the filter not the vice-versa.

What about RenderManager.renderGeometry(Geometry geom, LightFilter filter) so it will use the specified filter instead of the default one.

[zzuegg]

What about RenderManager.renderGeometry(Geometry geom, LightFilter filter) so it will use the specified filter instead of the default one.

That would imply adding a bunch of methods.

Since we have the possibility to add the required solution without any changes to the RenderManger i would go that route.
What about:
NullFilter.applyTo(renderManager); NullFilter.revert();

that would be just utilityfunctions to make the usage clean and not to store the currently used LightFilter every time we need to apply the NullFilter

[Ali-RS]

Yeah, that makes sense.

@pspeed42 may we hear about your opinion as well?

[pspeed42]

I don't really understand the problem enough to comment. I do wonder what NullFilter.revert() is doing, though.

[Ali-RS]

I don't really understand the problem enough

I think it is about increasing the performance of RenderManager.renderGeometry by adding an option that lets us disable the light filtering process for the cases where the light list is not required at all. That is what I understood from @zzuegg explanation.

And one of the solutions was to add a NullLightFilter that does nothing and just returns the light list "empty" and is meant to be set right before calling RenderManager.renderGeometry and unset (set back to the default one) right after we are done rendering:

NullLightFilter.applyTo(renderManager);
renderManager.renderGeometry(geom);
NullLightFilter.revert();

I do wonder what NullFilter.revert() is doing, though.

I guess it removes itself from RenderManager and sets back the previous light filter (grabbed in NullFilter.applyTo()) into the RenderManager.

[Ali-RS]

It surely impacts the performance of the AbstractShadowRenderer.

By the way, I looked at AbstractShadowRenderer, SpotLightShadowRenderer, PointLightShadowRenderer, and DirectionalLightShadowRenderer but I couldn't find where they are using RenderManager.renderGeometry except in the below method which seems to be for debugging use only

jmonkeyengine/jme3-core/src/main/java/com/jme3/shadow/AbstractShadowRenderer.java

Lines 456 to 473 in ac8c101

	/**
	* For debugging purposes, display depth shadow maps.
	*
	* @param r ignored
	*/
	protected void displayShadowMap(Renderer r) {
	Camera cam = viewPort.getCamera();
	renderManager.setCamera(cam, true);
	int h = cam.getHeight();
	for (int i = 0; i < dispPic.length; i++) {
	dispPic[i].setPosition((128 * i) + (150 + 64 * (i + 1)), h / 20f);
	dispPic[i].setWidth(128);
	dispPic[i].setHeight(128);
	dispPic[i].updateGeometricState();
	renderManager.renderGeometry(dispPic[i]);
	}
	renderManager.setCamera(cam, false);
	}

Can you please point out which code you refer to?

[zzuegg]

Can you please point out which code you refer to?

viewPort.getQueue().renderShadowQueue(shadowMapOccluders, renderManager, shadowCam, true);

It ultimately uses renderGeometry()

I guess it removes itself from RenderManager and sets back the previous light filter (grabbed in NullFilter.applyTo()) into the RenderManager.

Correct

I don't really understand the problem enough to comment. I do wonder what NullFilter.revert() is doing, though.

The base problem is that every time a geometry gets rendered all lights from Geometry.getWorldLights() are filtered against the camera frustum. Like in the shadow map rendering pass this is unnecessary since no lights are used during actual rendering.

[zzuegg]

I tested it locally. NullFilter would work as we expected.

https://gist.github.com/zzuegg/246f7cb47daf4f8d8c5a3a64a83544f8

This would be the implementation. Without comments up to now but

[Ali-RS]

Having NullLightFilter statically bookkeeping old light filter and RenderManager still seems weird to me though, I would rather not use that static bookkeeping stuff and methods inside the NullLightFilter and instead do something like:

Define null filter as a class field somewhere at the top in the using class

private static final LightFilter NULL_FILTER = new NullLightFilter();

then use it like this:

LightFilter previous = renderManager.getLightFilter();
remderManager.setLightFilter(NULL_FILTER);
// do your stuff
remderManager.setLightFilter(previous);

It requires adding just one more line of code (well actually 2 if we consider the class field definition;)

P.S:
one might say why not use remderManager.setLightFilter(null); then, well this way render manager would still pass the geometry world light list to the material but using NullLightFilter we explicitly say that we do not want any light and it will just pass an empty list.

This is just my opinion anyway.

Hope to hear other's opinion as well :)

[pspeed42]

This is why I was curious what revert() was doing... because I couldn't see how it would work without sharing state with the entire JVM. ...a big no-no.

[zzuegg]

Allright.
I agree on the global static issue but i would still prefer applyTo(RenderManager) and revert(). But it is your call. I implement it the way you like
(Thats why i am a really bad api designer)
Ok,Ok after rethinking it. I implement it the way Ali proposed. Who knows. JME might get multithreaded at some point in the future

[pspeed42]

If it's user-code that is calling apply/revert then it could just be on an instance instead of a static method... then you can have as many states as you want.

We've got some really nasty shared state issues in the anim package that are very tricky to unwind at this point, so I'm very sensitive to this subject.

[Ali-RS]

@zzuegg, any updates?

[zzuegg]

I have implemented the changes but had not yet time to test them fully for side effects.