Details

Reviewers

Sybren A. Stüvel (sybren)
Thomas Szepe (hg1)
Jorge Bernal (lordloki)

Commits

rBSfb0dd596e9a5: BGE : KX_VertexProxy support for more than 2 UV channel.
rBfb0dd596e9a5: BGE : KX_VertexProxy support for more than 2 UV channel.

Summary

I have added an optional named "index" argument for methode get/setUV, I have also modified the and set to deprecated methodes setUV2 and getUV2 : the doc was wrong and the methode can't be called anyway because it declared as VARARG in the .h and convert directly the args value to a vector in the .cpp.

Diff Detail

Repository: rB Blender
Branch: ge_multi_uv_channel

Event Timeline

Porteries Tristan (panzergame) updated this revision to Diff 3982.Apr 15 2015, 7:15 PM

Porteries Tristan (panzergame) retitled this revision from to BGE : KX_VertexProxy support for more than 2 UV channel..

Porteries Tristan (panzergame) updated this object.

Porteries Tristan (panzergame) added reviewers: Thomas Szepe (hg1), Sybren A. Stüvel (sybren), Jorge Bernal (lordloki).

Thomas Szepe (hg1) added inline comments.Apr 16 2015, 7:43 PM

source/gameengine/Converter/BL_BlenderDataConversion.cpp
518 ↗	(On Diff #3982)	Why you removed the UV check? If you remove this check the other texture coordinates ( environment, reflection, object, normal, orco and tangent maps) will be also converted. I don't know if this will cause problems. Did you check all possible setting? Did you also checked the other settings it in combination with UV scroll and the video texture?

Porteries Tristan (panzergame) added inline comments.Apr 16 2015, 7:53 PM

source/gameengine/Converter/BL_BlenderDataConversion.cpp
518 ↗	(On Diff #3982)	Without this we can convert other UV layer, and methode getUV(index) with an index of an unused UV don't return always (0.0, 0.0, 0.0). I think that is just an optimization.

As you mentioned get/setUV() will using the wrong UV map if a material using multiple textures and one of the textures are disabled. But this bug was also before. If you want to fix that you should do this in a separate patch.

SetUV2() was broken since blender 2.66a, In my opinion we can remove it, because we only can break compatibility to older files then blender 2.66. If the others are against removing the old API, you need to return a deprecated message and change the documentation too.

UV_Scroll_27x.blend242 KBDownload

source/gameengine/Converter/BL_BlenderDataConversion.cpp
518 ↗	(On Diff #3982)	No the cheek is necessary. If you disable a texture in between or set the texture to an other type as UV, then the previous UV map will be used. I think that was a fix in Blender 2.66. See my UV-Scroll test file. I have tested getUV(index) and setUV(uv, index) and it is working. So this change is not necessary to get it working.

Add deprecated messages.

Uncomment check.

inline comments

doc/python_api/rst/bge_types/bge.types.KX_VertexProxy.rst
129	would it be better getUV(index=0) as it seems that index = 0 by default?
138	same as before, would it be better setUV(uv, index=0) as it seems that index = 0 by default?

Fix keyarguments in the doc.

It looks good for me.

This revision is now accepted and ready to land.May 6 2015, 6:54 PM

Inline comments

doc/python_api/rst/bge_types/bge.types.KX_VertexProxy.rst
30	The list of all -> A list of all the
source/gameengine/Ketsji/KX_VertexProxy.cpp
426	Why remove this, but still use it inside KX_VertexProxy::PySetUV?

Other than the little inline comments, it looks ok to me.

source/gameengine/Ketsji/KX_VertexProxy.cpp
615	it should be "getUV(1)"
626	Same as before, it should be "setUV(uv, 1)"

Porteries Tristan (panzergame) added inline comments.May 6 2015, 10:33 PM

source/gameengine/Ketsji/KX_VertexProxy.cpp
426	It's a redundant call see line 435, and it will say that if the list is wrong a part of it is apply, i think that is a wrong behaviour.

Angus Hollands (agoose77) added inline comments.May 6 2015, 10:37 PM

source/gameengine/Ketsji/KX_VertexProxy.cpp
426	Looks good, didn't see other code block.

Jorge Bernal (lordloki) added inline comments.May 6 2015, 10:37 PM

source/gameengine/Ketsji/KX_VertexProxy.cpp
426	it is repeated at line 435 (out of the for loop)

"The list of…" -> "A list of…"

Fix deprecated message

Porteries Tristan (panzergame) updated this object.May 6 2015, 10:53 PM

Closed by commit rBfb0dd596e9a5: BGE : KX_VertexProxy support for more than 2 UV channel. (authored by Porteries Tristan (panzergame)). · Explain WhyMay 6 2015, 10:57 PM

This revision was automatically updated to reflect the committed changes.

Why were neither @Campbell Barton (campbellbarton) nor myself added as reviewers to this patch? KX_VertexProxy.get/setUV(2) were supposed to be deprecated. I had talked with @Porteries Tristan (panzergame) about this previously on IRC, and I would much rather see issues with KX_VertexProxy.uvs fixed (as opposed to bringing back deprecated features).