Closed rvandoosselaer closed 4 years ago
Hi Stephen, I was also able to debug it to this point. It gets indeed set, but there is somewhere a renderpass before where it is not available. I can only reproduce this issue on OSX.
The model is using the default PBRLighting definition, and I double checked, even manually set the technique to this value.
When I do a check in the shader for the variable and set it to a default value of 0 when it is not defined (#ifndef) it works fine. I’ll make a PR for it tomorrow.
Edit: I suspect it has something todo with the (hardware) armature/skinning. When using the same model without an armature I don’t have the issue.
I created a PR to fix the issue and added a small test case as requested. When running the test case after the #ifndef the shader compilation error is gone.
If needed I can remove the test case as I don't think it's really 'helpful' as it only loads and displays a model.
The thing that concerns me is how is the value set on other platforms and why is it not set on Macs. 99% sure that other platforms would also error out if the value wasn't set... which makes me think that the error isn't in the shader but in the apparatus that calls it.
The value do get set, but not ‘soon enough’. There is a renderpass where it is not available yet and the error is thrown. I tested this by setting the fragColor in the shader when NB_PROBES >= 1 in the provided test case, and this was executed. Maybe it’s because of the strict OpenGL core profile that Mac uses? The PR will not break any existing behavior at least.
But your concern is certainly correct. How do you propose we should proceed with this?
What I worry about is that this fix is only hiding a different problem. If NB_PROBES was missing on Windows, I think we'd still get that error... so it must be set somehow. That would mean that whatever code is supposed to set it isn't working on a Mac. And so I wonder what else is missing.
I think I'm getting closer:
SinglePassAndImageBasedLightingLogic:77
@Override
public Shader makeCurrent(AssetManager assetManager, RenderManager renderManager,
EnumSet<Caps> rendererCaps, LightList lights, DefineList defines) {
defines.set(nbLightsDefineId, renderManager.getSinglePassLightBatchSize() * 3);
defines.set(singlePassLightingDefineId, true);
//TODO here we have a problem, this is called once before render, so the define will be set for all passes (in case we have more than NB_LIGHTS lights)
//Though the second pass should not render IBL as it is taken care of on first pass like ambient light in phong lighting.
//We cannot change the define between passes and the old technique, and for some reason the code fails on mac (renders nothing).
if(lights != null) {
lightProbes.clear();
extractIndirectLights(lights, false);
defines.set(nbProbesDefineId, lightProbes.size());
defines.set(useAmbientLightDefineId, useAmbientLight);
}
return super.makeCurrent(assetManager, renderManager, rendererCaps, lights, defines);
}
The define is only set when the Lightlist is not empty.
When I step through the application, the fist time this method is called, the lightlist is empty, that's why the define is not set, and it isn't added to the shader.
I wonder why light list is set on windows and linux.
Can you try to use the GL Debug Mode (AppSettings#setGraphicsDebug(true)
), so we can see if some GL operation fails before that?
Note that on 3.2 Core Profile PBR works in our CI, so it depends on the setup of your scene. Maybe you can help contributing to https://github.com/MeFisto94/jme3-testing which is then visible at https://github.com/MeFisto94/test-spotbugs-integration on the testing branch (which we will integrate into the engine at some point)
@MeFisto94 : I added the #setGraphicsDebug() boolean , but I don't get an error prior to the shader compilation error. I would very much like to help and contribute to your repo. I'll have a look at it in the coming days.
@pspeed42 : I narrowed it down: the method is called with the lights set to null
in the the #preload method of the Material.
The preload scene is called when checking for hardware skinning in the SkinningControl class: https://github.com/jMonkeyEngine/jmonkeyengine/blob/4c8e400a83352b12a7ac5d5316c5bbdd06caffa8/jme3-core/src/main/java/com/jme3/anim/SkinningControl.java#L165-L181
This is called in the #controlRender method of the control: https://github.com/jMonkeyEngine/jmonkeyengine/blob/4c8e400a83352b12a7ac5d5316c5bbdd06caffa8/jme3-core/src/main/java/com/jme3/anim/SkinningControl.java#L283
This seems like a general issue, I don't get why it hasn't come up sooner?
What would be the best fix given this information? Keeping the check in the shader, or setting the define in the SinglePassAndImageBasedLightingLogic
class, regardless of whether the lights param is set.
If NB_PROBES wasn't defined on Windows or Linux then it would fail to compile there, too. So, fact: on Linux and Windows, NB_PROBES is set or it's running a different shader.
So on a Mac, either the list is not being set when it is on other platforms or a different fragment shader is being used than on Windows or Linux.
Hiding the error with a #ifndef isn't really going to fix the actual problem. Probably someone on Linux or Windows needs to figure out what is being run in the cases you identify... because something different is being run on Macs.
I see if I can get my hands on a Unix machine to test. Looking at the code I find it hard to believe this error is only related to the Mac OS. As the list is passed as null, if it is Mac, Unix, or Windows.
Thanks for your insights and help investigating this issue.
Ok, I see what the issue is.
It's true that even on Windows/Linux, referencing NB_PROBES will fail... if it's used as an actual reference like: float test = NB_PROBES;
However, at least on Windows (and I assume Linux), the preprocessor directives are more lenient.
...will succeed on my Windows box even if NB_PROBES is undefined. The #ifdef is assumed in this case... unless you are Apple.
So I'm ok with the #ifndef approach to make overly sensitive Macs happy... and at least now I know why it only fails there.
Fixed in v3.3 branch at 9a84e72 .
Get a
NB_PROBES
is not defined error in the shader.Environment:
Stacktrace: