Hi John, On Thu, May 27, 2010 at 8:35 AM, PCJohn <[email protected]> wrote: > another day with manipulators finished for me. Sending the update now. Fixed > points 1,2. -> Hopefully, they are resolved.
Yes, these issues now looked to be resolved. > Points 4 and 5 stay for the future. Unfortunately, not enough time for it > now, although that would be very useful. I've been looking at the SphericalManipulator and basic OrbitManipulator and their behaivor looks to be pretty near identical. The main difference just seems to be the public interface w.r.t. setting the angles. These two are so close that it looks like SphericalManipulator can be dropped as it's only in the OSG-2.9.dev series, not in OSG0-2.8, with the possibility of merging extra set methods into OribitManipulator. TrackballManipulator is also now just a shell, the only thing it adds is the default behaviour. To me it doesn't feel right to just have a class to set a default setting and having it just for backwards compatibility. However, TrackballManipulator has been available since even before OSG-1.0, so it's not something we can just drop. It feels like perhaps we could just rename OrbitManipulator to TrackballManipulator, and change the default setting to behavior like the original TrackballManipulator. OrbitManipulator might be a little more generic name for this type of manipulator, but I'd personally rather go with a slightly different name than have class proliferation where we can otherwise avoid it. I also also wondering about the FirstPersonManipulator, might it and UFOManipulator have an overlap that we could merge behaviors and just have one. The name of the UFOManipulator is pretty obscure, FirstPersonManipulator will mean far more to most developers so it's the one I'd be inclined to go for and drop UFOManipulator as a seperate class and just make sure it's most of it's behaviour can be ported into FirstPersonManipulator. > Submission comments: > It contains modified manipulators + > osgViewer/view.cpp - new manipulators are using CoordinateFrame and > excessive number of INFO messages were produced to the console These changes are way too complicated for what they achieve, extra complication == extra chance of bugs in my book so not appropriate. These are just INFO messages, one could just change this to DEBUG_INFO if you want to quieten them down further. > osgViewer.cpp - added two new manipulators Thanks I've merged this but retained the SphericalManipulator inclusion for now so we can do side by side testing. Once I've done some more testing I'll check the changes in. > Let me know about problems, your comments, or something I overlooked, etc. > Thanks for making indentation work. A couple of things I've noticed in testing. The direction of zoom associated with the mouse wheel movement is opposite to that of what we adopted for the mouse wheel in the 2.9.x series. 2.8.x and before doesn't support mouse wheel so this isn't a big issue about breaking compatibility. It is a change though. Personally I found the 2.9.x convention more intuitive, but I can certainly see that others might find the opposite more intuitive or simply that other apps use the convention you've adopted. I'm not sure if this is constomizable in your code yet, will need to do into the code. Another observation is the you've used const double/const float for method parameters, in the early days of the OSG I experimented with this as well with the optimistic belief that it'd provide compiler with more information about being able to optimize, however I've never come across a real benefit for it. In theory one might also catch bugs by forcing const, but there is a downside - the methods are just more difficult to read and maintain as there is simply more keywords in the way of what is important to the what the code is doing. For the last decade I've moved to only using const when it's actual required, and to avoid making code more long winded that it need be, and const built in types for parameters is a case for me where it's just too long winded to justify. You'll note that all cases of const built in types have been purged from the rest of the OSG so I believe it's appropriate to stick to this same coding style for the new osgGA classes. Cheers, Robert. _______________________________________________ osg-submissions mailing list [email protected] http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org
