Hi Robert, > Rather than have you spend more time refactoring once > again it's probably best for me to take your latest work and refactor > it, or use as it for inspiration for a refactor of the existing code > myself.
Please, do it. You are definitely better in design patterns. John On Thursday 04 of October 2012 14:33:07 Robert Osfield wrote: > Hi Jan, > > Thanks for the changes and resubmissions. I've just had a quick look > and remain puzzled why you are quite so fond of inheritance and still > feel it's the not the best way to go about this type of functionality, > as I've said several times now I believe the best way would be to > factor out the common functionality into a helper class rather via > inheritance. Rather than have you spend more time refactoring once > again it's probably best for me to take your latest work and refactor > it, or use as it for inspiration for a refactor of the existing code > myself. > > Thanks, > Robert. > > On 4 October 2012 12:31, PCJohn <[email protected]> wrote: > > Hi Robert, > > in my big stress, I managed to do my best to improve RayIntersector > > design. I am not sure if I understood your email in full, so finally, I > > created new class AbstractRayIntersector that encapsulates all the common > > functionality of Ray and LineSegment intersectors. I added constructors > > for starting point and direction for RayIntersector. I am personally very > > happy with the design. Let me know if something should be polished > > further. But feel free to change anything yourself. > > John > > > > On Tuesday 25 of September 2012 11:55:13 Robert Osfield wrote: > >> Hi John, > >> > >> I feel that you design and implementations tend to favour inheritance > >> ("is a") over composition ("has a") rather too much, generally in OO > >> programming it's better perfer composition over inheritance. So in > >> this particular case the common functionality that might be shared > >> between RayIntersection and LineSegmentIntersector would be not done > >> via inheritance but composion, so each one would have it's own > >> instance of class that provides the common functionality, this common > >> class would likely be a protected member and not part of the public > >> interface. It could even be that the common class just gets created > >> on the fly when it's needed - like the functors that are currrently > >> used to do the low level intersections. > >> > >> Robert. > >> > >> On 24 September 2012 09:04, PCJohn <[email protected]> wrote: > >> > Hi Robert, > >> > > >> > I am highly loaded this week, so I am responding late. Anyway, > >> > RayIntersector is on top of my programming priorities that are, > >> > unfortunately, pushed down by> > >> > > >> > some other non-programming responsibilities this week. Anyway: > >> >> First up I wouldn't suggest subclassing RayIntersector > >> >> from LineSegmentIntersector > >> > > >> > I was following different way of thinking. However, I think, your is > >> > more > >> > appropriate. I will try to rewrite it as you said and made both classes > >> > derived from osgUtil::Intersector. An alternative would be to make one > >> > more > >> > intermediate class. However, I have some design problems that I would > >> > like > >> > to ask you before I start changes: > >> > > >> > The biggest problem is with LineSegmentIntersector::Intersection > >> > struct. I > >> > would like to keep the structure the same between RayIntersector and > >> > LightSegmentIntersector to make the transition between using one and > >> > the > >> > second class as easy as possible. What is your suggestion here? Do you > >> > think that such thing is achievable? Or, should we just copy'n'paste > >> > the > >> > struct? > >> > > >> > If we manage to use the same Intersection struct for > >> > LineSegmentIntersector > >> > and RayIntersector, where we should place it? Should we create > >> > intermediate > >> > class between Intersector and Ray/LineSegmentIntersector for it? What > >> > name > >> > would be appropriate for the class? > >> > > >> >> the ratio now gets sorted distance which really isn't good > >> > > >> > I do not understand this sentence. Can you clarify it for me? Or, maybe > >> > I > >> > was not clear myself. Ratio still gets ratio and this is always true > >> > for > >> > LineSegmentIntersector. New variable "distance" gets distance and that > >> > is > >> > true for both - LineSegmentIntersector and RayIntersector. And as there > >> > is the same result by sorting Intersections by ratio and by distance, I > >> > changed operator < to sort by distance because it will work for both > >> > Intersectors. > >> > > >> > Some functions seems so standard for me, like enter(), leave(), > >> > reset(), > >> > and my getCoordinateTransformation(), and the most people would > >> > probably > >> > want to reuse them in their classes without any change. Thus, I am > >> > tempted to move them directly to osgUtil::Intersector class. They are > >> > mostly present as abstract methods there, so they would have standard > >> > implementation. Would you support this idea? > >> > > >> > Thanks for the answer. > >> > John > >> > > >> > On Thursday 20 of September 2012 15:20:33 Robert Osfield wrote: > >> >> Hi John, > >> >> > >> >> I haven't dived into to do a review yet but from your description I am > >> >> bit concerned. First up I wouldn't suggest subclassing RayIntersector > >> >> from LineSegmentIntersector, this might seem convinient in terms of > >> >> reusing code but from a conceptual and design standpoint it's > >> >> inapprpriate. Conceptuall for RayIntersector to be subclassed from > >> >> LineSegmentIntersector it would have to follow that RayIntersector "is > >> >> a" LineSegmentIntersector, or in more english terms Ray "is a" Line > >> >> Segment, which it's not, rather it's more like the other way round a > >> >> Line Segment is a special case of Ray. When you get mixing of > >> >> concepts like these it's almost always a inappropriate choice of OO > >> >> design. An example of how you've ended mixing the design is that the > >> >> ratio now gets sorted distance which really isn't good. > >> >> > >> >> If one wishes to reuse functionality then it would be appropriate to > >> >> refactor LineSegmentIntersector implementation so that the > >> >> functionality sits outwith LineSegmentIntersector and then both > >> >> RayIntersector and LineSegmentIntersector would reuse this common > >> >> functionality via a common class that provides that functionality. > >> >> > >> >> In terms of how one specifies the ray, I can see value in providing > >> >> set up via Vec4d pair for homogeneous coordinates for those who are > >> >> happy working with homogeneous coords, which should in theory include > >> >> all graphics programmers... but I suspect quite a few in the community > >> >> might struggle given how sometimes they struggle on lesser concepts. > >> >> So having the feature work so there is some means of working in Vec3d > >> >> position and Vec3d direction, such as as simply mapping these to a > >> >> Vec4d(position,1.0), Vec4d(direction,0.0) internally would be > >> >> appropriate. > >> >> > >> >> Thoughts? > >> >> Robert. > >> >> > >> >> On 19 September 2012 15:24, PCJohn <[email protected]> wrote: > >> >> > Hi Rui and Robert, > >> >> > > >> >> > I am attaching new RayIntersector class for infinite picking rays, > >> >> > derived > >> >> > from LineSegmentIntersector. I tried to share as much code as > >> >> > possible. > >> >> > Send me any comments if you have. > >> >> > > >> >> > The most important decision, I made: I am using two Vec4d for start > >> >> > and > >> >> > end > >> >> > point to specify them in homogeneous coordinates (w coordinate can > >> >> > be > >> >> > set > >> >> > to zero to indicate point at infinity). I considered it more general > >> >> > to > >> >> > alternative option to specify it by two Vec3d, one for start point > >> >> > and > >> >> > second for direction. But it can be changed. Or, some constructor > >> >> > getting > >> >> > point and direction can be easily added. Ideas? > >> >> > > >> >> > Second decision: LineSegmentIntersector::Intersection is now sorted > >> >> > by > >> >> > distance (not ratio). distance is now prefered as it contains always > >> >> > valid > >> >> > value while ratio is valid when using LineSegmentIntersector or when > >> >> > RayIntersector's ray is of finite length. > >> >> > > >> >> > Rui: I hope that RayIntersector will work for your cases. If not, > >> >> > let > >> >> > me > >> >> > know. > >> >> > > >> >> > Robert: In short, I overriden intersect(), intersects() and > >> >> > intersectAndClip() methods of LineSegmentIntersector by homogeneous > >> >> > versions for RayIntersector. Then, I was forced to replace some > >> >> > occurences of LineSegmentIntersectors by RayIntersector in OSG > >> >> > sources. > >> >> > Finally, I tested it on osgPick example and I improved it a little > >> >> > bit. > >> >> > > >> >> > One note: Picking for objects at infinity is not supported yet as > >> >> > this > >> >> > is > >> >> > not what people are usually doing. But it might be not so difficult > >> >> > to > >> >> > implement it with RayIntersector. > >> >> > > >> >> > Summary of changes: > >> >> > - RayIntersector class introduced for infinite ray support and > >> >> > infinite > >> >> > projection matrix > >> >> > - osgPick example extended to demonstrate infinite geometry and > >> >> > infinite > >> >> > projection matrix functionality > >> >> > > >> >> > John > >> >> > _______________________________________________ > >> >> > osg-submissions mailing list > >> >> > [email protected] > >> >> > http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-opensce > >> >> > neg > >> >> > rap > >> >> > h.org > >> >> > >> >> _______________________________________________ > >> >> osg-submissions mailing list > >> >> [email protected] > >> >> http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscene > >> >> gra > >> >> ph. org > >> > > >> > _______________________________________________ > >> > osg-submissions mailing list > >> > [email protected] > >> > http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-opensceneg > >> > rap > >> > h.org > >> > >> _______________________________________________ > >> osg-submissions mailing list > >> [email protected] > >> http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegra > >> ph. org > > > > _______________________________________________ > > osg-submissions mailing list > > [email protected] > > http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegrap > > h.org > _______________________________________________ > 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
