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

[Udo Giacomozzi]

Hello strk,

Wednesday, February 28, 2007, 8:47:22 PM, you wrote:
>> -     virtual void get_invalidated_bounds(rect* bounds, bool force) = 0;
>> +     virtual void add_invalidated_bounds(InvalidatedRanges& ranges, bool 
>> force) = 0;

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

If you insist. I made it intentionally pure virtual so that I find
missing detail implementations at compile time, which is very helpful
in our large class structure.


>> Index: libgeometry/snappingrange.h

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

Most of the files in gnash are lowercase. Also class names are mostly
lowercase (I was looking too much at Ranges2d class when choosing the
name). BTW, we should agree to some standard naming convention
(CamelCase vs. under_score etc.).

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

s> Maybe snap_distance==0 could mean single_mode=true ?

That's not the same.


s> BTW, I think "single_mode" could be a template parameter, which would
s> improve performance by not checking for this everytime.

Performance gain would be near zero and it would complicate things,
since it would lead to two unrelated SnappingRange2d classes, while
all server/* classes use the same, ie. don't care about single_mode.


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

Of course. Or you can make them private and provide access methods to
them if you prefer.



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

Looked cleaner.


>> +                     for (int rno=0; rno<_ranges.size(); rno++) {

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

That's why I didn't bother about _ranges.size(). I expect someone
changing this to iterators. I failed on it since I'm not very
experienced with them and my time is limited.


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

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

Why not? How are these two related?
snaptest() is a method that's called very often and thus should be
optimized.


>> +             T xdist = 99999999;
>> +             T ydist = 99999999;

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

I'm avoiding abs() since it depends on variable type.


s> size_type !! (it's unsigned)
s> Again, you should use 'iterators' instead, or this
s> will be wasted when switching to std::list.
..
s> I think we want contains(const Range2d<T>& ) too..

You are free to change this.


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

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

It's a private, speed-critical function for snaptest() only. No need
to check for "a", which can't be negative.

Udo