Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(940)

Issue 4289041: BGE: Material replacement for texface options (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years ago by dfelinto
Modified:
12 years, 4 months ago
Reviewers:
benoit.bolsee, jesterKing, brechtvl, dingto, bf-codereview
Base URL:
https://svn.blender.org/svnroot/bf-blender/trunk/blender/
Visibility:
Public.

Description

[patch originally submitted to: http://projects.blender.org/tracker/?func=detail&aid=26461&group_id=9&atid=127 ] This patch follows (with small changes) the proposal presented here - http://wiki.blender.org/index.php/User:Dfelinto/TexFace Not implemented: Backward Compatibility. Please refer to the bf-committers for discussions on that. Other technical discussions, bugs, reviews, can happen here. Summary: ======== The idea here is to move the texface options into the material panel. See the wip.jpg and ui.jpg images for the final UI (the one in the proposal had to be changed). 1 - Some of the legacy problems 2.49 and 2.5x has with the texface system: ======================================================== 1.1) Shadow, Bilboard and Halo are mutual exclusive (in the code), yet you can select a face to be more than one mode. 1.2) Sort only works for blend Alpha yet it's an option regardless of the Transparency Blend you pick. 1.3) Shared doesn't affect anything in BGE. 1.4) ObColor only works for Text objects (old bitmap texts) when using Texture Face Materials. (not address yet, I so far ignored obcolor) 2 - Code that needs review: =================== 2.1) set_draw_settings_cached this function was changed considerably. I think I could pass the material instead of flags and do the guessing game inside the function. Thoughts? 2.2) defines, defines, defines They are a few defines with the same value. We had more (and even some hardcoded values :/). What I did was to avoid bringing the DNA_material_types.h defines all the way to the Rasterizer. So Rasterizer has its own defines. The code is commented to make it easy for grep them. I'm not sure, however, if this is a good idea - or if the best way to do it is to use the GEMAT_ defines everywhere. 2.3) As a user I can't understand how ZSorting would affect Add Alpha. I tried indeed and nothing happens. The BGE code, however, consider valid the combination Add + Sort. Standing to the perception that Add shouldn't have sorting, I brought the sort option as part of the blending menu, so we have opaque, add, clip, blend alpha and blend alpha sort. In the code I could, after setting the material to use zsort, to convert GEMAT_ ALPHA_SORT to GEMAT_ALPHA. It works fine indeed. However I prefered to create the GPU_BLEND_ equivalent of the GEMAT_ALPHA_SORT and deal with that in the gpu as if it's alpha. 3 - Random Notes: ============ 3.1) Now "Use Face Textures" in material Option panel will work in Multitexture even if there is no texture channel. 3.2) In FaceTexture mode it will use TexFace all the time, even if you don't check the "Use Texture Face" option in the UI. It's a matter of decision, since the code for either way is there. I decided by the solution that makes the creation of a material fast - in this mode the user doesn't need to mess with textures or this "Use Texture Face" option at all. I'm not strong in my opinion here. But I think if we don't have this then what is the point of the Texture Face mode? 3.3) I kept references for tface only when we need the image, UV or the tiling setting. It should help later when/if we split the Image and UV layers from the tface struct (Campbell and Brecht proposal). 3.4) Attached also two of the sample files I used for tests - shadeless.blend and everything.blend. [ actually I don't see where to add more files here, so you can get them in the original report - http://projects.blender.org/tracker/?func=detail&aid=26461&group_id=9&atid=127 ] 3.5) I don't expect anyone to understand the whole patch. It involves a lot of areas that I myself wasn't familiar before doing this code. But a review in the general structure and one or other specific area would be nice. Thanks, Dalai

Patch Set 1 #

Total comments: 27

Patch Set 2 : Update with suggestions from 1st reviews. Also test to see if new patch integrates well. #

Total comments: 7

Patch Set 3 : Update with alphablend rename + fix for a bug in GLSL 3dview (derived mesh was still using tface) #

Total comments: 4

Patch Set 4 : readfile fixed (it was an EOL problem) - backward compatibility partially implemented #

Patch Set 5 : New patch, doversion + backward compatibility (Help Menu) working #

Total comments: 3

Patch Set 6 : fixed Benoit's comments + problem with meshes with no material. #

Total comments: 9

Patch Set 7 : fix for alpha (from Benoit's comment) + fix for faces w no material (the mat was being decoded 2x) #

Patch Set 8 : materialpop fix was committed, so now I uncommented the material pop function #

Patch Set 9 : fix for Benoit's suggestions (over email). waiting bug/patch #28111 to be committed #

Patch Set 10 : update after mat_nr fix + convert mesh (UV) only once (using TF_CONVERTED): it's all good now #

Patch Set 11 : create materials when no materials existent (Ton's suggestion) + mode doversion inside liblink_mesh #

Patch Set 12 : No change, only updating the patch to 2.60 #

Patch Set 13 : doversion unified with material code. using BKE_report for error. next: alpha fix and I'm finished. #

Patch Set 14 : "Final" patch - alpha working (Brecht pls see drawobject.c). couldn't get BKE_report to popup error) #

Patch Set 15 : fixing bugs I found with my test files. one bug left: http://goo.gl/jExRs shows solid in 3d view #

Patch Set 16 : fixed a bunch of corner cases, glsl and multitexture should be good now. need to test more files #

Patch Set 17 : Rename TwoSided to Back Culling + set it on by default + fixes for multitexture - final builds tmr #

Patch Set 18 : reverted twosided by default - now back cull is off bu default #

Patch Set 19 : final functional patch. doversion could be faster if I flag the material as converted. but it work. #

Patch Set 20 : Final patch - to be committed tomorrow morning #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1112 lines, -435 lines) Patch
release/scripts/startup/bl_ui/properties_data_mesh.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +0 lines, -47 lines 0 comments Download
release/scripts/startup/bl_ui/properties_material.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +30 lines, -1 line 0 comments Download
release/scripts/startup/bl_ui/space_info.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -0 lines 0 comments Download
source/blender/blenkernel/BKE_blender.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -1 line 0 comments Download
source/blender/blenkernel/BKE_image.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +3 lines, -0 lines 0 comments Download
source/blender/blenkernel/BKE_material.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +6 lines, -0 lines 0 comments Download
source/blender/blenkernel/intern/DerivedMesh.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +1 line, -17 lines 0 comments Download
source/blender/blenkernel/intern/cdderivedmesh.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +0 lines, -46 lines 0 comments Download
source/blender/blenkernel/intern/customdata.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -1 line 0 comments Download
source/blender/blenkernel/intern/image.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +17 lines, -0 lines 0 comments Download
source/blender/blenkernel/intern/material.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 4 chunks +485 lines, -0 lines 0 comments Download
source/blender/blenkernel/intern/subsurf_ccg.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +0 lines, -15 lines 0 comments Download
source/blender/blenloader/intern/readfile.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +25 lines, -0 lines 0 comments Download
source/blender/editors/mesh/editmesh_mods.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +0 lines, -4 lines 0 comments Download
source/blender/editors/space_image/space_image.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -1 line 0 comments Download
source/blender/editors/space_logic/logic_ops.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 4 chunks +37 lines, -0 lines 0 comments Download
source/blender/editors/space_view3d/drawmesh.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 13 chunks +79 lines, -49 lines 0 comments Download
source/blender/editors/space_view3d/drawobject.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +9 lines, -8 lines 0 comments Download
source/blender/editors/space_view3d/view3d_view.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -1 line 0 comments Download
source/blender/editors/uvedit/uvedit_ops.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +0 lines, -2 lines 0 comments Download
source/blender/gpu/GPU_draw.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +3 lines, -3 lines 0 comments Download
source/blender/gpu/GPU_material.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +3 lines, -2 lines 0 comments Download
source/blender/gpu/intern/gpu_draw.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 chunks +41 lines, -39 lines 0 comments Download
source/blender/gpu/intern/gpu_material.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -1 line 0 comments Download
source/blender/makesdna/DNA_material_types.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +35 lines, -0 lines 0 comments Download
source/blender/makesdna/DNA_meshdata_types.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +2 lines, -0 lines 0 comments Download
source/blender/makesrna/intern/rna_material.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +63 lines, -0 lines 0 comments Download
source/blender/makesrna/intern/rna_mesh.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +6 lines, -0 lines 0 comments Download
source/blender/makesrna/intern/rna_scene.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -1 line 0 comments Download
source/blender/modifiers/intern/MOD_uvproject.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +0 lines, -1 line 0 comments Download
source/gameengine/Converter/BL_BlenderDataConversion.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 17 chunks +108 lines, -75 lines 0 comments Download
source/gameengine/Ketsji/BL_BlenderShader.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +2 lines, -2 lines 0 comments Download
source/gameengine/Ketsji/BL_BlenderShader.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +4 lines, -4 lines 0 comments Download
source/gameengine/Ketsji/BL_Material.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +5 lines, -4 lines 0 comments Download
source/gameengine/Ketsji/BL_Material.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -2 lines 0 comments Download
source/gameengine/Ketsji/KX_BlenderMaterial.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +2 lines, -1 line 0 comments Download
source/gameengine/Ketsji/KX_BlenderMaterial.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 15 chunks +46 lines, -41 lines 0 comments Download
source/gameengine/Ketsji/KX_PolygonMaterial.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +2 lines, -2 lines 0 comments Download
source/gameengine/Ketsji/KX_PolygonMaterial.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 5 chunks +14 lines, -12 lines 0 comments Download
source/gameengine/Rasterizer/RAS_IPolygonMaterial.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 6 chunks +17 lines, -8 lines 0 comments Download
source/gameengine/Rasterizer/RAS_IPolygonMaterial.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 11 chunks +34 lines, -15 lines 0 comments Download
source/gameengine/Rasterizer/RAS_IRasterizer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +5 lines, -9 lines 0 comments Download
source/gameengine/Rasterizer/RAS_OpenGLRasterizer/RAS_OpenGLRasterizer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +2 lines, -2 lines 0 comments Download
source/gameengine/Rasterizer/RAS_OpenGLRasterizer/RAS_OpenGLRasterizer.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 6 chunks +18 lines, -18 lines 0 comments Download

Messages

Total messages: 46
dfelinto
13 years ago (2011-03-11 18:37:09 UTC) #1
dfelinto
Some of my own comments in the code. (ps !! this tool is really nice) ...
13 years ago (2011-03-11 19:58:26 UTC) #2
jesterKing
Some small comments, nothing big. http://codereview.appspot.com/4289041/diff/1/source/gameengine/Converter/BL_BlenderDataConversion.cpp File source/gameengine/Converter/BL_BlenderDataConversion.cpp (right): http://codereview.appspot.com/4289041/diff/1/source/gameengine/Converter/BL_BlenderDataConversion.cpp#newcode93 source/gameengine/Converter/BL_BlenderDataConversion.cpp:93: #include "BKE_image.h" On 2011/03/11 ...
13 years ago (2011-03-11 20:42:59 UTC) #3
brechtvl
I only had a quick glance, will do proper review later, mainly testing out the ...
13 years ago (2011-03-11 20:47:50 UTC) #4
dingto
No need to use split http://codereview.appspot.com/4289041/diff/1/release/scripts/ui/properties_material.py File release/scripts/ui/properties_material.py (right): http://codereview.appspot.com/4289041/diff/1/release/scripts/ui/properties_material.py#newcode621 release/scripts/ui/properties_material.py:621: split = layout.split() No ...
13 years ago (2011-03-11 23:08:27 UTC) #5
dfelinto
Update with suggestions from 1st reviews. Also test to see if new patch integrates well.
13 years ago (2011-03-11 23:25:15 UTC) #6
dfelinto
"Done" most of the pointed issues - I sent a new patch. Let's see how ...
13 years ago (2011-03-11 23:25:53 UTC) #7
brechtvl
Some more naming nits. Overall: could we use a single name for "alpha mode", "blend ...
13 years ago (2011-03-12 12:14:29 UTC) #8
brechtvl
> 2.1) set_draw_settings_cached > this function was changed considerably. I think I could pass the ...
13 years ago (2011-03-12 12:19:51 UTC) #9
brechtvl
Some more comments. Beyond that, it looks fine to me and I also found no ...
13 years ago (2011-03-12 12:34:37 UTC) #10
dfelinto
Update with alphablend rename + fix for a bug in GLSL 3dview (derived mesh was ...
13 years ago (2011-03-15 02:34:08 UTC) #11
dfelinto
Comments over the last patch. 1)renamed all blend modes, alpha modes, ... to alphablend (using ...
13 years ago (2011-03-15 02:51:30 UTC) #12
brechtvl
Latest changes look good. On 2011/03/15 02:51:30, dfelinto wrote: > 4) as commented before I ...
13 years ago (2011-03-15 11:49:08 UTC) #13
dfelinto
Backward compatibility for non disputant materials - (Brecht this is the msg I can't go ...
13 years ago (2011-03-16 08:48:53 UTC) #14
dfelinto
the last patch set seemed broken. it was failing. let me try again
13 years ago (2011-03-16 09:24:19 UTC) #15
dfelinto
A new attempt of sending the 4th patch. this time I created it in windows. ...
13 years ago (2011-03-16 09:28:53 UTC) #16
dfelinto
It's no longer working. Maybe there is an incompatibility between by latest patch - http://dl.dropbox.com/u/3292898/texface/texface_online_4c.diff ...
13 years ago (2011-03-16 09:37:31 UTC) #17
dfelinto
readfile fixed (it was an EOL problem) - backward compatibility partially implemented
13 years ago (2011-03-16 16:34:51 UTC) #18
brechtvl
I would put the version patch in a separate function as you mentioned. The error ...
13 years ago (2011-03-17 09:55:00 UTC) #19
dfelinto
hello all, some update (with no patch): the reason I put this patch on hold ...
12 years, 9 months ago (2011-06-19 15:07:07 UTC) #20
dfelinto
New patch, doversion + backward compatibility (Help Menu) working
12 years, 9 months ago (2011-07-05 08:04:29 UTC) #21
dfelinto
On 2011/07/05 08:04:29, dfelinto wrote: > New patch, doversion + backward compatibility (Help Menu) working ...
12 years, 9 months ago (2011-07-05 08:11:29 UTC) #22
benoit.bolsee
http://codereview.appspot.com/4289041/diff/38001/source/blender/blenkernel/intern/material.c File source/blender/blenkernel/intern/material.c (right): http://codereview.appspot.com/4289041/diff/38001/source/blender/blenkernel/intern/material.c#newcode1478 source/blender/blenkernel/intern/material.c:1478: flag ^= TF_LIGHT; ^= is the in-place X-or operation, ...
12 years, 8 months ago (2011-07-25 22:32:27 UTC) #23
dfelinto
fixed Benoit's comments + problem with meshes with no material.
12 years, 8 months ago (2011-07-26 07:18:50 UTC) #24
benoit.bolsee
more comments. I haven't checked the do_version() function yet. http://codereview.appspot.com/4289041/diff/50001/source/blender/blenkernel/intern/material.c File source/blender/blenkernel/intern/material.c (right): http://codereview.appspot.com/4289041/diff/50001/source/blender/blenkernel/intern/material.c#newcode560 source/blender/blenkernel/intern/material.c:560: ...
12 years, 8 months ago (2011-07-26 21:07:50 UTC) #25
dfelinto
reply to Benoit's second batch of review comments. http://codereview.appspot.com/4289041/diff/50001/source/blender/blenkernel/intern/material.c File source/blender/blenkernel/intern/material.c (right): http://codereview.appspot.com/4289041/diff/50001/source/blender/blenkernel/intern/material.c#newcode560 source/blender/blenkernel/intern/material.c:560: memmove((*matar), ...
12 years, 8 months ago (2011-07-27 15:18:38 UTC) #26
dfelinto
fix for alpha (from Benoit's comment) + fix for faces w no material (the mat ...
12 years, 8 months ago (2011-07-27 16:30:43 UTC) #27
dfelinto
materialpop fix was committed, so now I uncommented the material pop function
12 years, 8 months ago (2011-07-27 21:58:24 UTC) #28
dfelinto
http://codereview.appspot.com/4289041/diff/50001/source/blender/blenkernel/intern/material.c File source/blender/blenkernel/intern/material.c (right): http://codereview.appspot.com/4289041/diff/50001/source/blender/blenkernel/intern/material.c#newcode1716 source/blender/blenkernel/intern/material.c:1716: // if(me->mat[a] == ma) material_pop_id(&me->id, a); uncommented the code ...
12 years, 8 months ago (2011-07-27 22:07:22 UTC) #29
dfelinto
12 years, 8 months ago (2011-07-27 22:07:24 UTC) #30
dfelinto
fix for Benoit's suggestions (over email). waiting bug/patch #28111 to be committed
12 years, 8 months ago (2011-07-29 09:14:10 UTC) #31
dfelinto
update after mat_nr fix + convert mesh (UV) only once (using TF_CONVERTED): it's all good ...
12 years, 8 months ago (2011-08-01 23:18:36 UTC) #32
benoit.bolsee
Hi Dalai, I didn't have much time anymore to look into the patch but I ...
12 years, 7 months ago (2011-08-08 15:49:23 UTC) #33
dfelinto
create materials when no materials existent (Ton's suggestion) + mode doversion inside liblink_mesh
12 years, 7 months ago (2011-08-16 21:11:05 UTC) #34
dfelinto
I talked with Ton and he suggested me to do most of the conversion at ...
12 years, 7 months ago (2011-08-16 21:21:06 UTC) #35
dfelinto
No change, only updating the patch to 2.60
12 years, 7 months ago (2011-09-02 05:41:18 UTC) #36
dfelinto
doversion unified with material code. using BKE_report for error. next: alpha fix and I'm finished.
12 years, 6 months ago (2011-09-04 00:19:21 UTC) #37
dfelinto
Benoit, I didn't commit it to 2.59. I trusted the code was good but didn't ...
12 years, 6 months ago (2011-09-04 00:40:09 UTC) #38
dfelinto
"Final" patch - alpha working (Brecht pls see drawobject.c). couldn't get BKE_report to popup error)
12 years, 6 months ago (2011-09-04 02:38:21 UTC) #39
dfelinto
fixing bugs I found with my test files. one bug left: http://goo.gl/jExRs shows solid in ...
12 years, 6 months ago (2011-09-12 08:24:00 UTC) #40
dfelinto
fixed a bunch of corner cases, glsl and multitexture should be good now. need to ...
12 years, 6 months ago (2011-09-14 09:05:16 UTC) #41
dfelinto
Rename TwoSided to Back Culling + set it on by default + fixes for multitexture ...
12 years, 6 months ago (2011-09-15 09:16:26 UTC) #42
dfelinto
reverted twosided by default - now back cull is off bu default
12 years, 6 months ago (2011-09-15 23:27:47 UTC) #43
dfelinto
final functional patch. doversion could be faster if I flag the material as converted. but ...
12 years, 6 months ago (2011-09-18 09:23:55 UTC) #44
dfelinto
Final patch - to be committed tomorrow morning
12 years, 6 months ago (2011-09-19 07:17:40 UTC) #45
dfelinto
12 years, 6 months ago (2011-09-19 20:03:24 UTC) #46
Patch committed revision: 40372

I included in this commit a rename from the Shading Mode "Texture Face" to
"Singletexture". This can be discussed in parallel and rolled back if people
disagree. The reason for including this is to make the commit consistent on its
own, and now the only real difference between Multitexture and the former
Texture Face is the support for multiple textures in the later.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b