Hi Jaap, I've just started a review, only looked at TabPlaneDragger change right now, but one issue that I've spotted it would be worth flagging for further discussion, I'll add my comments after the section below. FYI, I'm not the original author of osgManipulator so didn't do the original design/implementation work, so I like yourself have to learn about the design/implementation by reading the code. Unfortunately the original author isn't active in the community right now so we'll have to make do without their input on original design intent. I'll add my specific comments after the section your original comments below:
On 24 May 2012 16:04, Jaap Glas <[email protected]> wrote: > C> Added a default parameter to the following constructor in TabPlaneDragger > and TabPlaneDragger.cpp: > > TabPlaneDragger(float handleScaleFactor=20.0f); > > The reason for this is that the default OSG tab sizes are way bigger than > those we used in our application so far. And since handleScaleFactor_ > is already a (constant) class member, I see no objection against making > it user defined. > > A different solution may be preferred. For example, the handleScaleFactor > parameter might be added to the function setupDefaultGeometry(bool) instead. > Or a separate function setScaleFactor(float) might be added, although that > will only help when it is called before setting up the geometry. I've just looked at the virtual Dragger::setupDefaultGeometry() method and it doesn't have a bool parameter, so the TabPlaneDragger::setupDefaultGeometry(bool) hides this, which is a good programming approach, so I think it would be worth us fix this at the same time as merging your other changes. Having properties/options in the Dragger to things like sizing of the tabs and whether it should work two sided is reasonable, and I would expect these to be of general use to all draggers. However, if we have properties then users may well look to have the ability to adjust the property and see results that reflect these changes, and in the current approach supported by osgManipulator this would likely require a rebuild of the manipulator geometry. Alternatively passing the properties into the setupDefaultGeometry() would be another approach and would avoid having properties that are just read only, however, as each manipulator may add it's own extra specialist features to control via properties then keeping the interface extensible would be required. My gut feeling is that we could end up spending lots of time chasing new properties and trying to abstract the interface too much that it'd end up awkward to use and maintain. Of the two approaches putting properties to control the rendering into the dragger subclass feels most appropriate. Putting the properties into the dragger subclass then requires us to decide how to expose them - via the constructor like you have done is possible, and if go this approach I would suggest the (bool twoSidedHandle = true) from the TabPlaneDragger be moved into the constructor as well. You mention that this approach avoids the issue of properties being set after the call setupDefaultGeometry() and I'd agree that this is a useful approach to avoid misuse of the classes. I'm in two minds about it though - it's easy and robust and pretty straight forward but it isn't flexible, or promote any hierarchy of the properties within the Dragger subclasses. One thing I've never been too happy about is the need to call setupDefaultGeometry(). Having an explicit call to setupDefaultGeometry() does allow one to set properties that might be used in the actual setupDefaultGeometry() implementation, but it also offers the possibility that new users my forget to call the method, or that it gets called twice. These a pretty daft programming errors, so one can easily suggest that documentation can fix it, but still it points to weakness in the classes. I guess having this method being explicitly called does offer the opportunity to not call it, but have users implement own geometry by adding themselves. If they are doing this then subclassing might be the more appropriate approach. I'm inclined to to make the call to setupDefaultGometry() from with the constructor, or have it do as a lazy instantiation from with the update traversal. I'm also inclined to allow for setting of properties on a dragger that dirties the Dragger and have the geometry updated automatically or again leave the update to happen with the update traversal when the dragger is dirtied. Potentially property changes like colour or tab sizes might be something that doesn't require a full geometry rebuild, but could be done by just setting colour arrays/vertex positions. Wiring all of this up would complicate the implementation though, something for at now I'd want to avoid. I'm still thinking about how best to tackle this and would happily take suggestion from yourself and the rest of the community. Robert. _______________________________________________ osg-submissions mailing list [email protected] http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org
