Add support for material diffuse, specular… in KX_BlenderMaterial python proxy. And use mathutils in KX_BlenderMaterial for the specular and diffuse color in KX_BlenderMaterial.
Details
Details
- Reviewers
Campbell Barton (campbellbarton) Sybren A. Stüvel (sybren) Thomas Szepe (hg1) Daniel Stokes (kupoman) Dalai Felinto (dfelinto) Mitchell Stokes (moguri) Angus Hollands (agoose77) Inês Almeida (brita_) - Commits
- rBS145ab8c49efe: BGE: Extend Python API for KX_BlenderMaterial (specular, diffuse…)
rB145ab8c49efe: BGE: Extend Python API for KX_BlenderMaterial (specular, diffuse…)
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- ge_python_material
Event Timeline
Comment Actions
The tilte "BGE : Fix forgetting of material diffuse, specular color ect… in KX_BlenderMaterial" sounds bad for me.
For me "BGE : Fix: Add forgotten material API for diffuse, specular color ect… in KX_BlenderMaterial" sounds better.
Talk to agoose77 about a better title.
I only style and code checked it. I don't make a compile or other tests.
| doc/python_api/rst/bge_types/bge.types.KX_BlenderMaterial.rst | ||
|---|---|---|
| 96 | I think 0.0 and 1.0 looks better for float | |
| source/gameengine/Ketsji/KX_BlenderMaterial.cpp | ||
| 1036 | CHECK_PYTHON_FLOAT_NON_ZERO implicate the it checks for values higher ten zero. | |
| 1037 | 0.0f | |
Comment Actions
Better title: "BGE: Extend Python API for KX_BlenderMaterial (specular, diffuse ...)"
| source/gameengine/Ketsji/KX_BlenderMaterial.cpp | ||
|---|---|---|
| 945 | "repeting" -> "repeating" | |
Comment Actions
Fix macro name, comment, range check and doc, and add a check for Material to avoid crash in blenderplayer.
Comment Actions
You should have left the material check in it's own patch instead of combining it with this one. Also, I don't know if @Campbell Barton (campbellbarton) will approve of the macro usage since I want to say there was some talk about not using these kinds of macros on the mailing list some time back. I'm not too keen on it myself, but I see not wanting to duplicate the code. I also don't like that a user can set a material property and silently have it do nothing. In other words, we shouldn't fail silently if we don't have a material. But, this also makes me wonder why we even allow the material to be NULL. It really should point to the default material. I'll probably have to talk more with @Dalai Felinto (dfelinto) about this.
Comment Actions
Hmm, looks like Phabricator was messing with me and the material check isn't in this patch as well. The other issues still stand. Another issue I see is that the docs are rather useless. Can we copy some UI strings from the material RNA to use as docs?
| source/gameengine/Ketsji/KX_BlenderMaterial.cpp | ||
|---|---|---|
| 1036–1040 | With some of the Blender 2.4x API we had errors raised if the values were outside the range and it was very problematic. The problem is, if you calculate a value, float-precision can mean you get outside the range by some small margin. And theres no really good way to figure out what a reasonable epsilon might be. In practice scripts ended up having to hard code the clamp limits before assigning any calculated values... at that point its just causing annoyance. Another issue is when limits are wrong or change, you introduce breakage. Just clamp the input, if clamping is making development confusing, (silently hiding problems) we can always have some debug option a script author can enable, which wants when a value is set out of range (and clamped). | |
| doc/python_api/rst/bge_types/bge.types.KX_BlenderMaterial.rst | ||
|---|---|---|
| 122–133 | Why not use mathutils.Color ? | |
| doc/python_api/rst/bge_types/bge.types.KX_BlenderMaterial.rst | ||
|---|---|---|
| 122–133 | There are nothing in the BGE to support Color python type. | |
Comment Actions
The RNA strings for diffuse_color and specular_color aren't too helpful (although I at least prefer their wording), but the strings for the other attributes (alpha, emit, hardness, specular_intensity, diffuse_intensity) look good.
| source/gameengine/Ketsji/KX_BlenderMaterial.cpp | ||
|---|---|---|
| 1071–1074 | Also for integers ? | |
| doc/python_api/rst/bge_types/bge.types.KX_BlenderMaterial.rst | ||
|---|---|---|
| 122–133 | If i implement mathutils color for KX_BlenderMaterial i will have to also impelement it in object color, in KX_VertexProxy ect… to keep it consistent. | |
| source/gameengine/Ketsji/KX_BlenderMaterial.cpp | ||
|---|---|---|
| 1205 | To be consistent with Blender, the emit value should be clamped to FLT_MAX. | |
Comment Actions
Looks good to me pending an okay from Campbell.
| source/gameengine/Ketsji/KX_BlenderMaterial.h | ||
|---|---|---|
| 107 | Very minor nit-pick, but I prefer function definitions to be in the source file, and not the header file. | |
| source/gameengine/Ketsji/KX_BlenderMaterial.cpp | ||
|---|---|---|
| 1205 | In the UI doc it says that emit range is [0, 2]. | |
| source/gameengine/Ketsji/KX_BlenderMaterial.cpp | ||
|---|---|---|
| 1205 | Emit [0, 2] and the colors [0, 1] are only soft limited to this ranges, the hard limit range is [0 ,FLT_MAX]. | |
| doc/python_api/rst/bge_types/bge.types.KX_BlenderMaterial.rst | ||
|---|---|---|
| 122–133 | +1 for using mathutils.Color where we use colours. It may be more work to make the existing code consistent, but I don't think that should stand in the way of doing the right thing. | |