HI David, I have just done a quick review and spotted the use of mutable variables in place of passing parameters. This is not good practice as it makes the plugin single threaded. I understand the need to avoid passing an ever increasing number of the parameters, but the way to tackle is to create an options object that packs this variables together and then you just pass a reference to to object, rather than the individual variables.
Could you make such a changes to the code and resubmit. Thanks, Robert. On Fri, Oct 31, 2008 at 10:20 AM, David Spilling <[EMAIL PROTECTED]> wrote: > Dear Robert, > > Please find attached a patch for the OBJ loader. > > PROBLEM > Several file formats (OBJ, 3DS) specifically identify the texture maps they > include as diffuse/ambient/specular etc. There is no clear and consistent > way to map these incoming textures to texture units, which an application > may want to do in order to apply shaders. Other formats (e.g. OpenFlight) > directly specify the texture unit, and so this problem does not arise. > > EXISTING CODE > The OBJ reader only reads "map_Kd" which it applies at texture unit 0, and > "map_opacity" which it applies at texture unit 1. It ignores any other > textures in the file. > > SOLUTION > The solution proposed is twofold. > Firstly, the options string is parsed for pairs of texture type/texture > units, for example "DIFFUSE=0 BUMP=3". If any are found, then the relevant > texture units are used for applicable found textures in the file. Texture > types other than those specified, e.g. if the file also contained a > SPECULAR_EXPONENT map, are not applied (it is assumed that the user knows > best) > Secondly, if no options are provided, then texture units are allocated to > the various maps in the file in a fixed _order_. The order has been defined > intending to retain existing functionality and not break peoples existing > code. The order is "DIFFUSE OPACITY AMBIENT SPECULAR SPECULAR_EXPONENT BUMP > DISPLACEMENT REFLECTION". So if an incoming file has map_Kd (diffuse), > map_Ks (specular) and bump (bump), then the diffuse would be allocated to 0, > specular to 1, and bump to 2. > > DISCUSSION > This is intended slightly as a strawman so feel free to knock down. The > intention behind the fixed order is so that we can use the same fixed order > in other plugins (e.g. 3DS) so that consistent behaviour is seen. With that > it mind, it might be more appropriate to extend the Options class or > something else at OSG level to fix the order in a plugin non-specific way, > but that's for later I guess (where would it go?). > This is also slightly related to the whole issue of shader management > (shaders applied by the app rather than bundled with a model need to know > which texture units are for what...) > > OTHER STUFF > 1) As usual, different modelling packages provide slightly different files, > so I may not have captured all the possible map nomenclature. I've followed > what I've seen, and what the spec says. On that note, the spec also says > that "map_opacity" should really be "map_d", but I've left it in for > backward compatibility. > > 2) The treatment of "refl" as a map has been changed to what I think is > correct - i.e. loading a specific reflection map. The previous code would > use any declared diffuse map as the reflection map, which I think is wrong, > but don't have reflection mapped OBJ models to test this behaviour on. > > 3) The OBJ format's idea of "CLAMP" has been included. > > 4) I _think_ the use of the scale and offset parameters on a texture is the > reciprocal of what it should be, but I've left it for the moment as none of > the packages I have produce files with it in. > > TESTED > Based on OSG 2.7.3. Tested under Vista, VC+ 9.0 (i.e. VC++2008 Express). > > Hope that makes sense, > > David > > _______________________________________________ > osg-submissions mailing list > [email protected] > http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org > > _______________________________________________ osg-submissions mailing list [email protected] http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org
