source/blender/collada/DocumentImporter.cpp
944	Would think variable names can be improved here, also perhaps using array instead of decoupled variables. Like: int emission[3];
947	Space between "if" and "(".
948	Indentation seems to be broken.
954	Same.
source/blender/collada/EffectExporter.cpp
191	Seems you're unintentionally modifying material bitflags here.
236	Same as above.

This revision now requires changes to proceed.Feb 11 2015, 4:41 PM

Gaia Clary (gaiaclary) added inline comments.Feb 11 2015, 5:13 PM

source/blender/collada/DocumentImporter.cpp
944	float emission[3]; float ambience[3]; Then you maybe can make use of the Blender's vector functions for adding the colors...
964	What happens when ambient and emission are defined? then the sum could become > 1. What will blender do in that case? is this possible after all? (maybe the collada specifications tell how to merge ambient and emission... Could you add some minimal comment here please?)

fixed indentation, some more code changes to comply with specification

<constant> cannot have diffuse, ambient, specular, shininnes as its child so color is solely based on
the emission color, so we map emission color to material's color.

Removed the addition of ambient color.

uploading files

files.tar.gz160 KBDownload

Gaia Clary (gaiaclary) added inline comments.Mar 1 2015, 3:11 PM

source/blender/collada/EffectExporter.cpp
119	Why not make one method: void EffectsExporter::writeShader(COLLADASW::EffectProfile &ep, COLLADASW::ShaderType, Material *ma ) This would avoid duplication of code.

As discussed with @Gaia Clary (gaiaclary) , merged writePhong and writeConstant to write_phong_or_constant,
and shifted the bool is_shadeless up one Level

Gaia Clary (gaiaclary) added inline comments.Mar 1 2015, 8:10 PM

source/blender/collada/EffectExporter.cpp
115	i think there is no need to use the parameter is_shadeless here. See below line 195
194	no need to call this function here. line 195 below already does the trick.
239	Having double negation sounds not right to me. I would rather use: if(is_shadeless) {...} else {...} or maybe: instead of is_shadeless, use boolean has_shade...
245	you could move line 241 and line 245 below the if/else block.
258	add a line break and indent here
source/blender/collada/EffectExporter.h
66 ↗	(On Diff #3662)	This is not a good name. Please do not name function of the kind "do_this_or_that(...)" better use either write_phong() write_constant() write_...() or write_for_shader(Enumeretion shader_type) whatever makes more sense.

Saurabh Wankhade (sauraedron) updated this revision to Diff 3680.Mar 5 2015, 4:43 PM

looks ok to me.

source/blender/collada/DocumentImporter.cpp
803	reduce redundancy: else if (shader == COLLADAFW::EffectCommon::SHADER_CONSTANT) { ma->mode = MA_SHLESS; } else { // default - lambert ma->diff_shader = MA_DIFF_LAMBERT; }
source/blender/collada/EffectExporter.cpp
211	something wrong with indentation here?