re: [2/5] d3dx9: Generate effect instances from materials for mesh loading.

[Dan Kegel]

+    static const char diffuse_paramname[] = "Diffuse";
+    static const char power_paramname[] = "Power";
+    static const char specular_paramname[] = "Specular";
+    static const char emissive_paramname[] = "Emissive";
+    static const char ambient_paramname[] = "Ambient";
+    static const DWORD basic_paramname_sizes = sizeof(diffuse_paramname) +
+                                               sizeof(power_paramname) +
+                                               sizeof(specular_paramname) +
+                                               sizeof(emissive_paramname) +
+                                               sizeof(ambient_paramname);
...

It might be more readable to define material_effects[] up next
to where those paramnames are defined, since it kind of
clutters the loop, and it's nicer to keep the whole table definition
together.  (Also, at least
conceptually, basic_paramname_sizes could be computed by a loop
over the table.  Then you wouldn't need those five _paramname variables,
you could just use string constants.  But if the strlen's would be
expensive, I guess that's ok.)

A comment or two about the layout of the output buffer might
be nice.

Can materials and material_ptr be const pointers?
- Dan

[2/5] d3dx9: Generate effect instances from materials for mesh loading.

[Dylan Smith]

Hi Dan,

On Wed, May 25, 2011 at 10:31 AM, Dan Kegel <dank(a)kegel.com> wrote:
> A comment or two about the layout of the output buffer might
> be nice.

The layout doesn't group all values, param names, or defaults, so it
actually seems to be best described using loops. Does the the
following comment help summarize the layout?

/* effects buffer layout:
 *
 * D3DXEFFECTINSTANCE effects[num_materials];
 * for (effect in effects)
 * {
 *     D3DXEFFECTDEFAULT defaults[effect.NumDefaults];
 *     for (default in defaults)
 *     {
 *         *default.pParamName;
 *         *default.pValue;
 *     }
 * }
 */
> Can materials and material_ptr be const pointers?

ID3DXBuffer_GetBufferPointer doesn't take a const, so no for
materials, but material_ptr can be const.


The rest of the advice I took (see the attached patch).

Thanks for the patch review.

[2/5] d3dx9: Generate effect instances from materials for mesh loading.

[Dan Kegel]

On Wed, May 25, 2011 at 6:23 PM, Dylan Smith <dylan.ah.smith(a)gmail.com> wrote:
> The layout doesn't group all values, param names, or defaults, so it
> actually seems to be best described using loops. Does the the
> following comment help summarize the layout?

Yes.

Patch looks more readable now, thanks.  Maybe an assert
at the bottom as a sanity check that the final output
pointer is exactly at the end of the buffer would be good.
- Dan