This patch adds a new API which allow us to access light shadow settings from python. The new API can be used to write custom GLSL materials with shadows.
Details
Details
- Reviewers
Campbell Barton (campbellbarton) Brecht Van Lommel (brecht) Porteries Tristan (panzergame) Thomas Szepe (hg1) Daniel Stokes (kupoman) Mitchell Stokes (moguri) Angus Hollands (agoose77) - Commits
- rBSc4c2bd1350a5: BGE: Allow access to light shadow settings with python
rBc4c2bd1350a5: BGE: Allow access to light shadow settings with python
Diff Detail
Diff Detail
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
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 .
Comment Actions
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
Comment Actions
@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))
Comment Actions
@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!
Comment Actions
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.
| source/blender/makesdna/DNA_lamp_types.h | ||
|---|---|---|
| 107 | 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 | Or maybe a flag. | |
Comment Actions
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?
Comment Actions
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?
Comment Actions
It should be possible to add member functions to RAS_ILightObject (and RAS_OpenGLLight) to get calculated values on the fly from GPULamp.
Comment Actions
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?).
Comment Actions
@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
Comment Actions
I'll leave reviewing the game engine side changes to others.
| source/blender/gpu/GPU_material.h | ||
|---|---|---|
| 304 ↗ | (On Diff #5701) | 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. | |
Comment Actions
@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
Comment Actions
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. | |
| 287–300 | I haven't tested it but I think writing the color will not work. If you want to change it, to make the color writable, then a color callback should be used here. | |
| 289 | KX_LightObject *self = static_cast<KX_LightObject *>(self_v); | |
Comment Actions
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 :)
Comment Actions
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. | |
Comment Actions
@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)
Comment Actions
@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
Comment Actions
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
Comment Actions
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; | |
Comment Actions
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 ↗ | (On Diff #5733) | The matrix should be initalized: MT_Matrix4 mat; mat.setIdentity() retrun mat; |
Comment Actions
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) | |
Comment Actions
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) 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. | |
Comment Actions
@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. | |
Comment Actions
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.