Re: [Gnash-commit] gnash backend/render_handler.h backend/render_h...

[strk]

On Wed, Feb 28, 2007 at 05:25:26PM +0000, Udo Giacomozzi wrote:

> -     virtual void get_invalidated_bounds(rect* bounds, bool force) = 0;
> +     virtual void add_invalidated_bounds(InvalidatedRanges& ranges, bool 
> force) = 0;

As I was asking on IRC.. can we find a default implementation for this ?
Like adding NOthing, or adding World ?
make check doesn't work because this function is pure virtual.
Not a big deal, I can make one, but I had the same problem this
morning, with get_invalidated_bounds (that's why I asked).


>  void
> -video_stream_instance::get_invalidated_bounds(rect* bounds, bool /*force*/)
> +video_stream_instance::add_invalidated_bounds(InvalidatedRanges& ranges, 
> +     bool /*force*/)
>  {
> -     bounds->expand_to_point(-1e10f, -1e10f);
> -     bounds->expand_to_point(1e10f, 1e10f);
> +     rect bounds;
> +     bounds.set_world();
> +     ranges.add(bounds.getRange());

This can become:

Range2d<float> bounds; bounds.setWorld();
ranges.add(bounds);


> Index: libgeometry/snappingrange.h

I would have liked this to be called SnappingRanges2d.h
(file name follows class defined in it).
Not a big deal, but it's easier to find things..

> +template <typename T>
> +class DSOLOCAL SnappingRanges2d
> +{
> +public:
> +     typedef geometry::Range2d<T> RangeType;
> +     
> +     /// distance (horizontally *plus* vertically) below ranges are snapped
> +     /// You MUST initialize this! 
> +     T snap_distance;

If initialization is a MUST, what about taking it in the constructor ?

> +     /// if set, only a single, outer range is maintained (extended). 
> +     bool single_mode;       

Maybe snap_distance==0 could mean single_mode=true ?
BTW, I think "single_mode" could be a template parameter, which would
improve performance by not checking for this everytime.
Not sure about syntax, maybe something like:

template <typename T, bool single_mode>
class ...

And then provide specialization for single_mode=true|false

Also, do we need snap_distance and single_mode to be public ?


> +     SnappingRanges2d() {
> +             snap_distance = 0;
> +             single_mode = false;
> +     }

Initialization lists are to be preferred:

        SnappingRanges2d()
                :
                snap_distance(0),
                single_mode(false)
        {}

> +     /// Add a Range to the set, merging when possible and appropriate
> +     void add(const RangeType& range) {
> +             if (range.isWorld()) {
> +                     setWorld();
> +                     return;
> +             }
> +             
> +             if (range.isNull()) return;
> +             
> +             if (single_mode) {
> +             
> +                     // single range mode
> +             
> +                     if (_ranges.empty()) {
> +                             RangeType temp;
> +                             temp.setNull();

You can assert(temp.isNull()) right after constructor (Null is the default).
        Range2d(RangeKind kind=nullRange);


> +                             _ranges.push_back(temp);
> +                     }
> +                     
> +                     _ranges[0].expandTo(range);
> +             
> +             } else {        
> +             
> +                     // multi range mode
> +             
> +                     for (int rno=0; rno<_ranges.size(); rno++) {

It seems that even if template-based, vector.size() takes some e time,
so it's better to call it only once in loops, unless the size is going to 
change.
BTW, in your case you'd better using iterators, given the possibility to switch
to std::list (seen in your comments)

> +     /// returns true, when two ranges should be merged together
> +     inline bool snaptest(const RangeType& range1, const RangeType& range2) {

How about renaming this to shouldMerge() or similar ?
And, does the 'inline' part make any sense in a templated class ?

> +             // when they intersect anyway, they should of course be merged! 
> +             if (range1.intersects(range2)) 
> +                     return true;
> +                     
> +             // simply search for the minimum x or y distances
> +     
> +             T xdist = 99999999;
> +             T ydist = 99999999;

No point in initializing these here, right ?
Can initialize xdist = abs(xa1-xa2) below..

> +             T xa1 = range1.getMinX();
> +             T xa2 = range2.getMinX();
> +             T xb1 = range1.getMaxX();
> +             T xb2 = range2.getMaxX();
> +             T ya1 = range1.getMinY();
> +             T ya2 = range2.getMinY();
> +             T yb1 = range1.getMaxY();
> +             T yb2 = range2.getMaxY();
> +             
> +             xdist = absmin(xdist, xa1-xa2);
> +             xdist = absmin(xdist, xa1-xb2);
> +             xdist = absmin(xdist, xb1-xa2);
> +             xdist = absmin(xdist, xb1-xb2);
> +
> +             ydist = absmin(ydist, ya1-ya2);
> +             ydist = absmin(ydist, ya1-yb2);
> +             ydist = absmin(ydist, yb1-ya2);
> +             ydist = absmin(ydist, yb1-yb2);
> +             
> +             return (xdist + ydist) <= snap_distance;
> +
> +     } 
> +             
> +     /// Resets to NULL range
> +     void setNull() {
> +             _ranges.clear();
> +     }
> +     
> +     /// Resets to one range with world flags
> +     void setWorld() {
> +             if (isWorld()) return;
> +             
> +             RangeType world;
> +             world.setWorld();
> +             
> +             _ranges.clear();
> +             _ranges.push_back(world);
> +     }
> +     
> +     /// Returns true, wenn the ranges equal world range
> +     bool isWorld() const {
> +             return ( (size()==1) && (_ranges.front().isWorld()) );
> +     }
> +     
> +     /// Returns true, when there is no range
> +     bool isNull() const {
> +             return size()==0;

_ranges.empty();

> +     }
> +     
> +     /// Returns the number of ranges in the list
> +     int size() const {

size_t !!
Or, more strictly, SnappingRanges2d<T>::size_type, which should
be typedef'ed to container::size_type

> +     /// Returns the range at the specified index
> +     RangeType getRange(int index) const {
> +             assert(index>=0);

size_type !! (it's unsigned)
Again, you should use 'iterators' instead, or this
will be wasted when switching to std::list.

> +     /// Returns true if any of the ranges contains the point
> +     bool contains(T x, T y) const {

I think we want contains(const Range2d<T>& ) too..

> +private:
> +
> +     inline T absmin(T a, T b) {
> +             if (b<0) b*=-1;
> +             return min(a,b);

You're assuming a is positive here. Not very clean.

--strk;