[vdr] Re: BUGFIX: cOsd::CanHandleAreas()

[Klaus Schmidinger <Klaus.Schmidinger@cadsoft.de>]

Reinhard Nissl wrote:
> 
> Hi,
> 
> Klaus Schmidinger wrote:
> 
> >>>>To have overlap detection work properly and to return reasonable error codes
> >>>>to the user, CanHandleAreas() now first checks the alignment of all areas and
> >>>>performs overlap detection afterwards.
> >>>
> >>>I don't quite see what the advantage is in having an extra 'for' loop.
> >>>The error messages are still the same, aren't they?
> >>>
> >>>Shouldn't
> >>>
> >>>  for (int i = 0; i < NumAreas; i++) {
> >>>      if (Areas[i].x1 > Areas[i].x2 || Areas[i].y1 > Areas[i].y2 || Areas[i].x1 < 0 || Areas[i].y1 < 0)
> >>>         return oeWrongAlignment;
> >>>      for (int j = i + 1; j < NumAreas; j++) {
> >>>          if (Areas[i].Intersects(Areas[j]))
> >>>             return oeAreasOverlap;
> >>>          }
> >>>      }
> >>>
> >>>do just the same?
> >>
> >>Well, the difference is that your version might return early with
> >>oeAreasOverlap without having checked Area[j] for correct alignment first.
> >>
> >>I think, it's better to first check all areas for alignment and then for
> >>overlapping as the latter "requires" the former.
> >
> > Ok, the I'll do it this way:
> >
> > --- osd.c       2004/06/15 20:29:42     1.55
> > +++ osd.c       2004/07/18 09:23:03
> > @@ -602,15 +602,18 @@
> >
> >  eOsdError cOsd::CanHandleAreas(const tArea *Areas, int NumAreas)
> >  {
> > +  eOsdError Result = oeOk;
> >    for (int i = 0; i < NumAreas; i++) {
> > +      if (Areas[i].x1 > Areas[i].x2 || Areas[i].y1 > Areas[i].y2 || Areas[i].x1 < 0 || Areas[i].y1 < 0)
> > +         return oeWrongAlignment;
> >        for (int j = i + 1; j < NumAreas; j++) {
> > -          if (Areas[i].Intersects(Areas[j]))
> > -             return oeAreasOverlap;
> > -          if (Areas[i].x1 > Areas[i].x2 || Areas[i].y1 > Areas[i].y2 || Areas[i].x1 < 0 || Areas[i].y1 < 0)
> > -             return oeWrongAlignment;
> > +          if (Areas[i].Intersects(Areas[j])) {
> > +             Result = oeAreasOverlap;
> > +             break;
> > +             }
> >            }
> >        }
> > -  return oeOk;
> > +  return Result;
> >  }
> >
> >  eOsdError cOsd::SetAreas(const tArea *Areas, int NumAreas)
> >
> > This avoids the extra 'for' loop and reports oeWrongAlignment before any
> > oeAreasOverlap.
> 
> I'm sorry Klaus, but why do you want to omit the extra 'for' loop?
> 
> I think, that you want to omit it for performance reasons.

Not really - I'd just like to avoid it ;-)

> But is it really
> better to perform many overlap detections in the inner loop to finally realize
> in the last run of the outer loop, that the last area has a wrong alignment?

Well, in case everything is ok (which is the normal case during production
operation) it will have to run through both loops completely, anyway.
The only case where it wouldn't run through all loops cmpletely is in case
of an error - and that's something the programmer will have to fix, anayway.

Klaus