[boost] Re: geometry classes code [not so long ;)]

From: Aleksey Gurtovoy (alexy_at_[hidden])
Date: 2000-01-02 15:48:21

> > // ???
> > // by the way - why original boost operator templates return 'T' instead
> > // 'const T'?
> >
> >
file://---------------------------------------------------------------------
-----
> > -
> > template <class T, class U>
> > class multipliable2
> >   {
> >   friend const T operator *( T x, const U& y ) { return x *= y; }
> >   friend const T operator *( const U& y, T x ) { return x *= y; }
> >   };
> >
> >
file://---------------------------------------------------------------------
-----
> > -
> > template <class T, class U>
> > class dividable2
> >   {
> >   friend const T operator /( T x, const U& y ) { return x /= y; }
> >   };
> >
> Okay, I'm putting my foot down: we are going to have to implement all of
the
> 2-type versions of the operator templates with this workaround for VC++
> compatibility anyway. We can simply derive the specializations from the
> versions with the '2' for those platforms which support that syntax.
> Question: should we deprecate the use of the specialization (LEGAL C++
> LANGUAGE FEATURE) so that we can one day simplify the library!??!
>
I don't think we must take the line of least resistance - especially against
compiler writers ;)

> >     unit_type cols_num()  const;
> >     unit_type rows_num()  const;
>
> cols_num and rows_num are nondescriptive and redundant. Furthermore, if
this
> is pure geometry (e.g. size<double>) there are no rows or columns.
>
You are right. These will be removed.

> >     self& set( unit_type new_x, unit_type new_y );
> This should be called "assign" for uniformity with the STL.
>
Ok.

> >     self& x( unit_type new_y );
> >     self& y( unit_type new_x );
> >     self& col( unit_type new_col );
> >     self& row( unit_type new_row );
>
> How many different ways do we need to set a coordinate?
>
> We have p.x() = 1, p.x(1), p.col() = 1, p.col(1).
>
> Too large an interface can be harmful, since there will be no single usage
> idiom established. I argue strongly for the removal of the row/col
> interfaces, and also for the removal of one of the x/y interfaces,
probably
> the one returning non-const references.
>
Well, I agree with you... but which the x/y interface did you mean - one
which returns non-const references to 'unit_type' or another, that returns
non-const references to 'self'? ;) (I think you meant unit_type& x();
unit_type& y();, but I will be better to ask).

> >     // moving
> >     self& move_by( unit_type x_offset, unit_type y_offset );
> >     self& move_by( const point<unit_type>& offset );
> >     self& move_by( const size<unit_type>& offset );
>
> The last two move_by are completely redundant interface, since we have +=.
> Since it's easy to write p += point<T>(x, y), I would dispense with the
> first as well.
>
Probably you are right again... I've added 'move_by' with an aim to allow to
write something like p.move_by( 5, 3 ).align( n ), but I agree with your
comments about 'align' below, and by now a code like p.move_by( 5,
3 ).x( 10 ) seems already not so attractive to me as it was yesterday ;).
What do you think about a chaining of member function calls in general? I
have never heard anything about this subject from any person who can made me
change my mind, so I'll glad to hear something about it from members of this
group.

> >
> >     // aligning to grid
> >     self& align( unit_type granule );
> >     self& align( const size<unit_type>& granule );
>
> I don't think these belong in this class. Rounding is essentially a scalar
> operation having nothing to do with vectors in the mathematical sense.
>
> >   private:
> >     unit_type x_;
> >     unit_type y_;
> >   };
>
> It certainly looks at first glance like there's an opportunity to save
some
> code duplication by factoring parts of point and size into a common base
> class.
>
I thought about it, but by the time I was posting the code, it seems
non-obvious that we would decide to deal with 'size' at all, so I left the
refactoring until the best times.

And replying to other your email:
>
> These nice parallels notwithstanding, I worry about using the name 'size'
> this way since it is used for the STL containers to mean something
> different. It really does mean something more like "difference" in this
> case, so I guess my preference for 'extent' doesn't work too well either.
>
> Suppose we called it 'delta' (ick)?
>
> I suggest the member function name 'extent' should be used to get the size
> of a rectangle, and should return a delta.
>
Actually I think that 'extent' as an alternative to 'size' is pretty good.
But 'delta' is not bad too, so I'm not sure which of them we must choose...
And member function 'size' will be definitely renamed to 'extent'.

> >
file://---------------------------------------------------------------------
-----
> > -
> > // geometry::rect
>
> I hate abbreviations. 'rectangle' has worked very well for me in my code
and
> has never been a burden.
>
Ok, it will be renamed.

> >   public:
> >     // constructors
> >     rect();
>
> You need more comments everywhere in this header. For example, above: 'A
> canonical empty rectangle (0,0), (0,0)'
>
> You could use some of the characters that you spend on making horizontal
> lines with your comments which do nothing for readability for description
> instead.
>
Well, what can I say.. You are right again...

> Also, I know we usually put this down to personal style but so far we have
> avoided the 'indented brace style' in boost. I'm just asking that we
uphold
> the tradition.
>
I have no problems with it, and it'll be changed.

> >     rect( unit_type left, unit_type top, unit_type right, unit_type
bottom
> > );
> >     rect( const point<unit_type>& origin, const point<unit_type>&
corner );
>
> Should be corner1, corner2. This is much more useful and means there are
> fewer ways to use rect erroneously.
>
Ok.

> >     rect( const point<unit_type>& origin, const size<unit_type>&
rect_size
> > );
> >     rect( const point<unit_type>& origin );
>
> What does the above constructor do?
It constructs an empty rectangle with the top-left point == 'origin' ;) I
found this useful some time ago, but I'm beginning to think on your part -
this is redudant.

> >     // accessors - const
> >     const point<unit_type>& origin() const;
> >     const point<unit_type>& corner() const;
>
> Sorry, I hate these names. A rectangle has 4 corners. Which one do you get
> when you call corner()? Is the rectangle's origin at its center? (I know
it
> isn't of course).
>
> >     const point<unit_type>& left_top()      const;
> >     const point<unit_type>& right_bottom()  const;
>
> I don't know why, but I always say top_left and bottom_right. I think this
> order is more natural for native English speakers. Do these names seem
weird
> to anyone else?
>
I'm not a native English speaker (as probably you've noticed already), so I
can't say anything about this subject ;). But what other people think about
this?

> What about bottom_left and top_right? My rectangles have those, too.
>
and mine - they are below in the code ;)

> >     self& set_empty();
>
> >     self& normalize();
>
> What does the above do?
>
> N.B. point and rect are generally cheap to assign, so I would consider
> eliminating functions like set_empty() which can easily be expressed as 'r
=
> rectangle<T>()'. I tend to favor a more minimal interface, though I can
see
> the readability advantage of set_empty().
>
'set_empty' will be removed.

> >
> >     self& origin( const point<unit_type>& new_origin );
> >     self& corner( const point<unit_type>& new_corner );
> >
> >     self& left_top( const point<unit_type>& new_left_top );
> >     self& right_bottom( const point<unit_type>& new_right_bottom );
>
> I don't think I believe in returning a reference to self from a mutating
> function. Please convince me.
>
Is this an answer to my question about the chaining of member function
calls?

> >
> >     // moving
> >     self& move_by( const size<unit_type>& offset );
> >     self& move_to( const point<unit_type>& new_origin );
>
> Too much redundancy for my taste. r += offset and r =
rectangle(new_origin,
> r.size()) work nicely.
>
This relates to the previous comment...

> >     // inflating/deflating
> >     self& inflate( unit_type delta = 1 );
> >     self& inflate( unit_type x_delta, unit_type y_delta );
> >     self& inflate( const size<unit_type>& delta );
> >
> >     self& inflate_size( unit_type delta = 1 );
> >     self& inflate_size( unit_type x_delta, unit_type y_delta );
> >     self& inflate_size( const size<unit_type>& delta );
>
> What's the difference between inflate_size and inflate? I had to look at
the
> code to figure it out. inflate_size is nondescriptive IMO and
unneccessary:
> r.bottom_right(r.bottom_right + delta) is much clearer.
>
I'll remove 'inflate_size'.

> >
> >     // aligning
> >     self& align( unit_type granule );
> >     self& align( const size<unit_type>& granule );
>
> Again, I think this is outside the scope of geometry.
>
And now I think so too.

> >     // testing
> >     bool is_empty() const;
>
> Should be called empty() for uniformity with STL.
>
Ok.

> >     int  hit_test( const point<unit_type>& p )    const;
>
> Outside the scope of geometry again, I think.
>
It was a 'stamp of the past' ;)

> >     bool inside( const rect<unit_type>& other )   const;
> >     bool contains( const point<unit_type>& p )    const;
> >     bool contains( const rect<unit_type>& other ) const;
>
> If you have 'contains' I don't think you need 'inside'
>
Probably...

> >     // unionization / intersection
> >     self& operator |=( const rect<unit_type>& other );
> >     self& operator &=( const rect<unit_type>& other );
>
> you also need:
> bool intersects(const rect<unit_type>&) const
>
Actually I thought that ( rect1 & rect2 ).is_empty() will perform the same
task, but it seems too complicated. I will add 'intersects'

> >
> > // rect comparison/intersection test
> > template<class T> bool operator ==( const rect<T>& r1, const rect<T>&
r2 );
> > template<class T> bool operator &( const rect<T>& r1, const rect<T>&
r2 );
>
> where's the free operator|() ?
>
Here:

template<class T>
class rect
...
  , andable< rect<T> >
  , orable< rect<T> >
=)

> > template<class T>
> > inline
> > rect<T>&
> > rect<T>::normalize()
> >   {
>
> This is surprising. I expected "normalize" to move the top_left to 0,0 for
> some reason.
> You could easily arrange for rectangles to always be normalized. I believe
> that to be a superior arrangement. Rectangles created un-normalized would
be
> made purely empty at construction time (bottom_right = top_left). This
makes
> everything else much simpler.
>
I agree with you here.

> > inline
> > int
> > rect<T>::hit_test( const point<unit_type>& p ) const
> >   {
> >   // return a bitmask created by the following sheme
> >   // (0000 - point inside rect):
> >   //
> >   // 1010 | 0010 | 0110
> >   // -----+------+-----
> >   // 1000 | 0000 | 0100
> >   // -----+------+-----
> >   // 1001 | 0001 | 0101
> >   //
> >   return    ( ( p.x() < left() )  << 3 )
> >           | ( ( p.x() > right() ) << 2 )
> >           | ( ( p.y() < top() )   << 1 )
> >           | ( ( p.y() > bottom() ) )
> >           ;
> >   }
>
> The utility of this is completely un-obvious to me. Also I think it
contains
> fencepost errors. The point (0,0) should be inside the rectangle(0,0,1,1)
>
It will be removed.

> > template<class T>
> > bool
> > operator &( const rect<T>& r1, const rect<T>& r2 )
> >   {
> >   return  !( r2.left()    >= r1.right()
> >           || r2.top()     >= r1.bottom()
> >           || r2.right()   <= r1.left()
> >           || r2.bottom()  <= r1.top()
> >           );
> >   }
>
> Huh?!?! If operator&= does intersection, then so should the free
operator&!
> This is terribly surprising!
>
Actually there is a free operator& which does intersection ( class rect: ...
andable< rect<T> >), but I was so stupid that instead a providing the 'bool
intersect' member I decided to add a free 'bool operator&' - without the
least thought about how it would work. Sorry.

And that's all. I'm going to fix all these stuff today and to post a new
version. Thanks for your time, Dave!!!
-Alexy.