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-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 > _______________________________________________ osg-submissions mailing list [email protected] http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org
