On 6 May 2013 13:26, Alexandre Bergel alexandre.bergel@me.com wrote:
Igor did not want to send them because he does not want to look like an asshole criticizing but I think that if we want roassal to be really a success we should.
Sure, making this email public is the best to do. As researchers we should embrace feedback, even if not positive.
- Point 1: wasting cycles to do conversion
ROPharoCanvas>>line: aPoint to: aPoint2 width: aSmallInteger color: aColor
nativeCanvas line: (self virtualToRealPoint: aPoint) to: (self virtualToRealPoint: aPoint2) width: aSmallInteger color: aColor.wasting cycles to convert from your local coordinate space into global one on every single operation is not a good idea. Guess why :)
Athens provides the user-space coordinate system for geometry by default.. so that you don't have to maintain it by yourself.
So if you want a real system these points should be addressed ;)
Maintaining virtual coordinates is essential for zooming and scrolling the whole view. Athens has indeed fast primitives to do this. We will probably consider using them this once Athens will be part of Pharo.
- Point 2: missing transformation matrix
You don't even have a transformation as a concept.. and instead of simple and straightforward affine matrix which people learn in schools today: a Camera [ ... ] see this #virtualToRealPoint: calls? Athens provides this for free. Do not do it the hard way: (do not transform poor vector multiple times to get there.) Athens do it for you using math: (v * M1) * M2 = v * (M1*M2)
We thought about using matrixes. And the nice things is to be able to do rotation, thing that we cannot do right now in Roassal because we do not have matrixes. We are wondering whether we should wait for Athens to be part of Roassal to use matrix or doing it our way.
By the way, using a matrix implies more operations that Roassal actually does, because doing v * M has more operations than the method #virtualToRealPoint:. However this does not really matter since this is rather cheap.
are you sure that v*M has more operations?
AtthensAffineTransform>>transform: aPoint | px py | px := aPoint x. py := aPoint y. ^ Point x: (sx*px +(shx*py) + x) y: (shy*px + (sy*py) + y)
4 multiplications 4 additions
virtualToRealPointNoTrunc: aPoint "Return a virtual point from a one expressed in the real coordinates" | visibleBounds offset | visibleBounds := self bounds. offset := self position. ^ ((aPoint x asFloat - offset x * realExtent x / visibleBounds width) asFloat) @ ((aPoint y asFloat - offset y * realExtent y / visibleBounds height) asFloat)
2 subtractions 2 muls 2 divisions + float conversions (why they are there?)
+ converting back to integers
virtualToRealPoint: aPoint "Return a virtual point from a one expressed in the real coordinates" ^ (self virtualToRealPointNoTrunc: aPoint) asIntegerPoint
+ adding extra point in ROAbstractCanvas (2 additions more)
virtualToRealPoint: aPoint "Return a real point from a one expressed in the virtual coordinates"
^ (camera virtualToRealPoint: aPoint) + offset
And things like:
virtualToRealRectangle: aRectangle "Return a rectangle with real coordinates from one expressed in the virtual coordinates" ^ (self virtualToRealPoint: aRectangle origin) corner: (self virtualToRealPoint: aRectangle corner)
this makes sense only if both coordinate systems not rotated respective to each other. Once you have rotation, this method loses all geometrical sense and very dangerous to use.
- Point 3 about design
ROAbstractArrow ROArrow ROReversedArrow ROHorizontalArrow ROReversedHorizontalArrow ROVerticalArrow ROReversedVerticalArrow
Why do we have all these classes? why they are not...
... traits? Well... because there is little support in Pharo to work with traits. Another reason is traits exist only in Pharo.
no, i was wondering why you need more than a single class, representing an arrow which connects two arbitrary points. Instead you have only horizontal or vertical + separate subclasses for different arrow direction.. so, either i don't understand something, or if i right, this needs to be thrown away.
Cheers, Alexandre
-- _,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;: Alexandre Bergel http://www.bergel.eu ^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;.