Page MenuHome

BGE: Allow access to light shadow settings with python
ClosedPublic

Authored by Ulysse Martin (youle) on Dec 25 2015, 12:51 PM.

Diff Detail

Repository
rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
source/gameengine/Ketsji/KX_Light.h
76

static PyObject *pyattr_get_clipstart(void *self_v, const KX_PYATTRIBUTE_DEF *attrdef);

77

Same as above

Ulysse Martin (youle) updated this revision to Diff 5639.EditedDec 25 2015, 10:41 PM
Ulysse Martin (youle) edited edge metadata.

Ok. Thanks very much for your review!

Ulysse Martin (youle) updated this revision to Diff 5656.EditedDec 28 2015, 1:15 AM
Ulysse Martin (youle) retitled this revision from BGE: Allow access to clipstart and clipend shadowmap values with python to BGE: Allow access to light shadow settings with python.
Ulysse Martin (youle) updated this object.

I added the possibility to access: shadowmap bindcode (I'm not really sure about my code and the place to put it), shadow buffer type, shadow bias, shadow bleed bias (I didn't remember if I mentionned that I've also added frustum size)

Thanks very much for your review, especially concerning the shadowmap bindcode :)

New test file: http://www.pasteall.org/blend/39836 (HG1 demo file from here: http://blenderartists.org/forum/showthread.php?359656-Dynamnic-Shadow-for-custom-shader-possible/page2 modified. The shader only works for variance shadows. I printed all the values of the new attributes in the console)

Please tell me if this works for you!

EDIT: I have to change something about the bindcode. It doesn't work correctly. The doc is false. I think the code for bindcode has to be put lines 836 && 844 in gpu_material.c (lamp->la->shadow_bind_code = GPU_texture_opengl_bindcode(lamp->tex);) . However, I can't make the shader work with simple buffer type in this file: http://www.pasteall.org/blend/39836 (I mixed the 2 shaders... Maybe I made a mistake. http://www.pasteall.org/62975/diff .

The simple sahadow don't works because buffertype returns a wrong value.
I also changed some attribute names and changed it to camelCase.
Modified padding in DNA_lamp_types.h.

Maybe we should alos add the shadow color and the enable to.

new patch http://www.pasteall.org/62979/diff
modified file http://www.pasteall.org/blend/39841

Ulysse Martin (youle) updated this revision to Diff 5667.EditedDec 28 2015, 2:17 PM

@Thomas Szepe (hg1): Thanks very much for debugging me :P . I added color and shadow option enabled.

The new test file: http://pasteall.org/blend/index.php?id=39844

Update: http://www.pasteall.org/62982/diff (replaced shadowActive with "hasShadow" (lamp.hasShadow))

Thomas Szepe (hg1) edited edge metadata.

Look OK for me.
But the naming of the new API should checked by a second reviewer.

This revision is now accepted and ready to land.Dec 30 2015, 12:51 PM

@Angus Hollands (agoose77): Hi, I added you as a reviewer because we're not sure (with HG1 and panzergame) about the names of the functions in the API. If you've got a moment, could you take a look at it and tell me what do you think of the patch and the names. I thank you very much!

Mitchell Stokes (moguri) requested changes to this revision.Dec 30 2015, 6:32 PM
Mitchell Stokes (moguri) edited edge metadata.

Some of these could be really useful as RW attributes (e.g., shadow color), but that can be handled in a separate patch.

I find the clipStart, clipEnd, bias, and bleedBias attribute names a little confusing since the name suggests an attribute of the light, not the light's shadow (although, the docs do clarify this). We may want to add shadow to the names (e.g., shadowClipStart). This does, however, make the names longer. The docs are almost useless since they just repeat the attribute name without giving an actual explanation (e.g., what is the shadowmap's clip start?). You might be able to grab more meaningful descriptions from the tooltips of the corresponding UI elements.

I'm not sure what I think about exposing the bindcode. It breaks encapsulation of the render code and is OpenGL dependent. However, this very useful to end users, so it is probably okay.

Adding Brecht to look over the bf_gpu changes.

This revision now requires changes to proceed.Dec 30 2015, 6:32 PM
source/blender/makesdna/DNA_lamp_types.h
107 ↗(On Diff #5667)

use_shadow can be a char. There is no need to waste 4 bytes on a boolean value.

source/blender/makesdna/DNA_lamp_types.h
107 ↗(On Diff #5667)

Or maybe a flag.

Brecht Van Lommel (brecht) requested changes to this revision.Dec 30 2015, 6:54 PM
Brecht Van Lommel (brecht) edited edge metadata.

We should not add shadow_bind_code and use_shadow to the Lamp struct, it should be retrieved through some GPU_lamp_get_*() utility function.

Generally it's not good design to duplicate such state, it's really hard to ensure it does not go out of sync. For example on file save/reload these would have invalid values. It's best to have a single source of truth whenever possible. If some temporary state needs to be stored it should be in GPULamp, but as far as I can tell there is no reason to do it here, it can be computed on the fly?

Also, I'm not sure that e.g. the shadow bindcode is ready when the Blender data conversion happens? I don't remember exactly, but maybe it's possible that GLSL shaders and associated GPULamp data is created lazily, while the game is already running?

For example, what happens when calling setGLSLMaterialSetting(), do the values get refreshed properly?

It should be possible to add member functions to RAS_ILightObject (and RAS_OpenGLLight) to get calculated values on the fly from GPULamp.

It looks like RAS_ILightObject already has a HasShadowBuffer() that we can use for KX_Light.shadowActive (maybe this should be renamed to KX_Light.hasShadowBuffer?).

Ulysse Martin (youle) updated this revision to Diff 5701.EditedDec 31 2015, 4:02 AM
Ulysse Martin (youle) edited edge metadata.

@Brecht Van Lommel (brecht) && @Mitchell Stokes (moguri) && @Porteries Tristan (panzergame): Thanks very much for your review! I hope I forgot nothing (but I'm very scatterbrained). I added shadow color in RW.

The new test file: http://www.pasteall.org/blend/39882

Brecht Van Lommel (brecht) requested changes to this revision.Dec 31 2015, 4:30 AM
Brecht Van Lommel (brecht) edited edge metadata.

I'll leave reviewing the game engine side changes to others.

source/blender/gpu/GPU_material.h
304

I said to name it GPU_lamp_get_* but GPU_lamp_shadow_bind_code seems like the consistent name here.

source/blender/gpu/intern/gpu_material.c
2220

lamp->tex could be NULL here, you need to check that.

This revision now requires changes to proceed.Dec 31 2015, 4:30 AM
Ulysse Martin (youle) updated this revision to Diff 5702.EditedDec 31 2015, 11:22 AM
Ulysse Martin (youle) edited edge metadata.

@Brecht Van Lommel (brecht): Thanks! @Porteries Tristan (panzergame): I replaced 0 with -1 but I'm waiting for others comments to update revision

EDIT: The new patch: http://www.pasteall.org/63075/diff
and the new test file: http://www.pasteall.org/blend/39902

I included light perspective matrix (read only). I had to modify the shader code in the example file (set bias to 0.001 when lamp.type == Sun and lamp.shadowMapType == simple)

@Thomas Szepe (hg1): I wait for Tristan review to update the revision. Shadow colors can be set this way (python without mathutils), but not r, g, b individually (up arrow key in the test file). If you don't mind, we could do another patch to include mathutils in KX_Light.cpp for light color and light shadows color. Or I'll do that with the help of Tristan if he's ok.
I'm very sorry about merging issues with the previous patch (I worked on upbge :S).
Happy new year! 01/01/2016

source/blender/gpu/intern/gpu_material.c
2220

code style :
return lamp->tex ? GPU_texture_opengl_bindcode(lamp->tex) : -1;

also use -1 for error value.

source/gameengine/Rasterizer/RAS_OpenGLRasterizer/RAS_OpenGLLight.cpp
184

-1 is the error value in opengl. 0 can be used iirc.

Your actual patch won't merge into the actual master. There are three merge conflicts within all three RAS_OpenGLLight files. I don't have NeedShadowUpdate() in my actual source.
One last thing maybe will be good. I think a getter for the perspective matrix of the lamp will be fine too. So we don't need to calcualte our own matrix in Python.

source/gameengine/Ketsji/KX_Light.cpp
284

If you don't make the color writable, it should return a tuple "(fff)" instead of the list.
Campbell prefer this because it is not mutable.

287–300

I haven't tested it but I think writing the color will not work.
Because actually like bias, bleedbias, the shader will bind it as GPU_uniform (=const) and not as GPU_dynamic_uniform (=uniform).

If you want to change it, to make the color writable, then a color callback should be used here.
Like here:
https://developer.blender.org/D157 adding a vector callback .
https://developer.blender.org/D1432 canges for a color callback .

289

KX_LightObject *self = static_cast<KX_LightObject *>(self_v);

Ulysse Martin (youle) updated this revision to Diff 5722.EditedJan 2 2016, 1:14 PM
Ulysse Martin (youle) edited edge metadata.
Ulysse Martin (youle) marked 2 inline comments as done.

Hi, I added lamp dynpersmat (lamp.viewMatrix... name suggested by @Porteries Tristan (panzergame)...)). It's the matrix used in test_shadowbuf and test_shadowbuf_vsm (variance) It seems to work with Suns and Spots (However I had to modify the shader in the test file to set bias to 0.001 in the case lamp.type == sun && lamp.shadowMapType == simple).

@Thomas Szepe (hg1): I updated my previous comment (I don't know if you've seen it): I'm very sorry about merging issues with the previous patch (I was working on upbge :S). Lamp.shadowColor can be set the way I did in my previous patch (lamp.shadowColor = [r, g, b] - up arrow key to test in the test file but each r, g, b component can't be set individually as I didn't implemented mathutils.Color in KX_Light.cpp. I'm wondering if you don't mind if we make another patch to implement matutils in KX_light.cpp both for lamp.color and lamp.shadowColor as it adds a big piece of code and in several files KX_PythonInit etc...

@Porteries Tristan (panzergame): I didn't find appropriate PyObject PyVectorFrom in the code to replace Py_BuildValue (pyattr_get_shadow_color) so I didn't change this code (I took example on lamp.color) for now.

The new test file: http://www.pasteall.org/blend/39912

I hope I made not too much errors this time and that you're not angry against me HG1 ( :P ) to not have implemented mathutils...

Happy new year :)

Thomas Szepe (hg1) requested changes to this revision.Jan 2 2016, 5:38 PM
Thomas Szepe (hg1) edited edge metadata.

I think we should discuss about the API naming, please discuss that with the others (mogury, campbell).
ViewMatrix is not a good naming. It implies that it is only the lamp view matrix. But it returns the bias*projection*view*model matrix (I am not 100% sure about this, I don't found where it is calculated).
So I think something like lightMatrix or a remark in the dokunation that it in reality the MVPB matrix would be returned would be fine.

Also shadowBindNumber now sound ugly to me, shadowMapNumber or shadowTextureNumber sound better to me.

And I think we can shorten hasShadowBuffer to hasShadow or to useShadow (same naming as in the UI).

Lamp.shadowColor can be set the way I did in my previous patch (lamp.shadowColor = [r, g, b] - up arrow key to test in the test file

I think you misunderstood me. Sure it works with the custom shader. Because setting the variable and reading the value back is working.
But It doesn't works for the normal shadow lamp (without custom shader).
Deactivate the cutomsahder and try to change the shadow color and you will se that the color will not change.
So as I written before there are two ways. You cahnge GPU_uniform to an GPU_dynamic_uniform in gpu_material.c to make the value writeable
or you make the color read only, but a tuple.
I think it is enough to make it read only for now. I never had a user request to make the shadow color writable.

I hope I made not too much errors this time and that you're not angry against me HG1 ( :P ) to not have implemented mathutils...

No problem we can do this in a later patch. Also it is not necessary if you make the color read only.

However I had to modify the shader in the test file to set bias to 0.001 in the case lamp.type == sun && lamp.shadowMapType == simple

From, the Blender source.

lamp->bias = 0.02f * la->bias;
*/* arbitrary correction for the fact we do no soft transition */
lamp->bias *= 0.25f;

doc/python_api/rst/bge_types/bge.types.KX_LightObject.rst
89

Not 100% sure but I think for variance shadowmaps. is correct.

This revision now requires changes to proceed.Jan 2 2016, 5:38 PM

@Thomas Szepe (hg1): Indeed, we could discuss about the names and the values to make read only or RW... (If I remember correctly, Moguri suggested to make the color writable (I understand now why we need a GPU_Dynamic_Uniform so I'll change that unless we won't make the shadowColor writable) and the name hasShadowBuffer, whereas Tristan suggested hasShadow or useShadow.

As it's difficult to get consensus from everyone, I leave the debate open here, on this revision. I add @Campbell Barton (campbellbarton) as a reviewer.

My proposition is:

shadowClipStart, shadowClipEnd, shadowFrustumSize, shadowBias, shadowBleedBias, shadowTextureNumber, shadowMapType, shadowColor (writable), hasShadow, mvpMatrix (with doc about bias too).

Please let me know if you're ok with this. Thanks.

EDIT: Another version of the patch http://www.pasteall.org/63139/diff
and of the test file: http://www.pasteall.org/blend/39940
(shadowColor read only and tuple instead of list, shadowBindId instead of shadowBindNumber, shadowMatrix instead of viewMatrix)

@Ulysse Martin (youle): i agree with:

  • shadowClipStart
  • shadowClipEnd
  • shadowFrustumSize
  • shadowBias
  • shadowBleedBias
  • shadowMapType
  • shadowColor
  • hasShadow

but i don't with :

  • shadowTextureNumber -> shadowTextureId, like bge.texture.Texture.bindId

and i don't know for :

  • mvpMatrix
Ulysse Martin (youle) edited edge metadata.

Update:

shadowBleedBias doc: The bias for reducing light-bleed on variance shadowmaps. --> The bias for reducing light-bleed on variance shadow maps.
hasShadowBuffer name: useShadow to fit the normal Blender API.
shadowMapType doc: Fix a typo: Variance instead of variance
viewMatrix name: shadowMatrix
shadowMatrix doc: http://www.blender.org/api/blender_python_api_2_76_2/gpu.html?highlight=gpu_dynamic_lamp_dynpersmat#gpu.GPU_DYNAMIC_LAMP_DYNPERSMAT
shadowTextureNumber name: shadowTextureId
shadowTextureId doc: The OpenGL shadow texture bind number/id.
prefix m_clipstart, m_clipend, m_bias, m_bleedbias, with shadow in the code (BL_BlenderDataConversion and RAS_ILightObject
shadowColor: read only and tuple (as RW can be handled in a separate patch as said Moguri.)

Thanks very much to HG1 for his review... If I made some mistakes again, a less scatterbrained person than me could take control over the patch.

test file: http://www.pasteall.org/blend/39947

I am sorry I miss two code style faults. We use a space before and after a question mark and colon.

You can wait with updating this, until you get the OK from the others.

source/blender/gpu/intern/gpu_material.c
2210

return lamp->tex ? GPU_texture_opengl_bindcode(lamp->tex) : -1;

2215

return lamp->dynpersmat ? (float *)lamp->dynpersmat : NULL;

Porteries Tristan (panzergame) edited edge metadata.

Other that the inline comment and the usage of Color for shadowColor, it's ok for me.

source/gameengine/Rasterizer/RAS_OpenGLRasterizer/RAS_OpenGLLight.cpp
195

The matrix should be initalized:

MT_Matrix4 mat;
mat.setIdentity()
retrun mat;
Angus Hollands (agoose77) edited edge metadata.

Looks good to me. Sorry for late reply.

Ulysse Martin (youle) updated this revision to Diff 5778.EditedJan 9 2016, 4:06 PM
Ulysse Martin (youle) edited edge metadata.

Few minor updates. Thanks all for your reviews and comments and help :)

Mitchell Stokes (moguri) edited edge metadata.
Campbell Barton (campbellbarton) edited edge metadata.

Generally seems fine, though not sure why you would want to access some of these settings - shadowBleedBias for eg.

source/gameengine/Ketsji/KX_Light.cpp
161–170

Prefer we use Python convention (underscore instead of camelCaps)

Thomas Szepe (hg1) added a comment.EditedJan 11 2016, 6:39 PM

Generally seems fine, though not sure why you would want to access some of these settings - shadowBleedBias for eg.

In an earlier version of this patch it does not return the 'shadowMatrix'. So we build our own with 'shadowClipStart', 'shadowClipEnd' and 'shadowFrustumSize'. So actually this values are only useful, if somebody want to build his own user matrix.

'shadowBias' and 'shadowBleedBias' are used for variance shadow maps (cutom GLSL shader), to get the same render resualt as in the viewport.
Look at the example file(s).

source/gameengine/Ketsji/KX_Light.cpp
161–170

@Campbell Barton (campbellbarton) @Mitchell Stokes (moguri)
Actually most of the BGE API use camelCase. There are only some which are using underscore.
Moguri decided (long discussion on IRC) to use camelCase for existing modules.

But actually this module use two functions with underscore (lin_attenuation, quad_attenuation) and two which are using lowercase (spotsize, spotblend).

Yes, maybe in this case it would be fin to use also underscore to don't have a third variation.
But I don't want to decide this.

Ulysse Martin (youle) updated this revision to Diff 5816.EditedJan 13 2016, 9:32 AM
Ulysse Martin (youle) edited edge metadata.

@Campbell Barton (campbellbarton):

  • About shadowBleedBias, and other shadow settings that seems useless: The main goal of this patch was to have the possibility to have shadows on custom GLSL materials (For the moment, when you write a custom GLSL material, you can't (unless you write a 2000 lines script) have shadows on it.) With this patch, you can use the "template" we've written to get the shadows settings from the lights and make your custom material in GLSL with shadows from Blender (the template use Blender source code to reconstruct shadows and we need shadowBleedBias and shadowBias for variance shadow maps): eg: gl_FragColor = myCustomMaterial * shadows (if shadow = vec4(1.0), the custom material won't change; if shadow = vec4(vec3(0.0), 1.0), the material will be black at the place of the shadow). Sorry for bad english.
  • About camelCase, I personnaly don't care if we use it or not, so I update the patch with camel Case, but I join another .diff with underscores instead of camelCase: http://www.pasteall.org/63392/diff

The patches are up to date with the last Blender sources. The test file is in one of my previous comments. If you want to test, you have to take the last test file.

Thanks again for your reviews and comments :)

source/gameengine/Ketsji/KX_Light.cpp
161–170

I thought we were in agreement to continue using camelCase for the BGE API until such time when we switch the whole API to be PEP8 compliant? I would prefer camelCase for consistency with the rest of the BGE API, even if the rest of this class isn't consistent with the rest of the API.

Thomas Szepe (hg1) edited edge metadata.

I think it is better to keep 'shadowClipStart', 'shadowClipEnd' and 'shadowFrustumSize'. It is better to have it if somebody need it.
So actually we also keep camelCase API.

This revision was automatically updated to reflect the committed changes.