[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gnash-commit
Subject:    Re: [Gnash-commit] gnash backend/render_handler.h backend/render_h...
From:       strk <strk () keybit ! net>
Date:       2007-02-28 19:47:22
Message-ID: 20070228194722.GG24604 () keybit ! net
[Download RAW message or body]

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;



[prev in list] [next in list] [prev in thread] [next in thread]