Hi,
I am in need of transforms that include scaling, and find a TODO in the src
showing that this is on the table. If there is interest I would be
willing to try
to send a first patch implementing this.
Before I begin though it would be good to get clarification on a few items:
1. Still interest in getting this implemented?
2. Agreed that the best way is to do it is to change op.TransformOp to be a
full (2D) affine transformation matrix instead of simply an offset?
3. The API would this stay the same, with the following methods added
additionally to op.TransformOp:
- Scale(sx, sy float32, around f32.Point)
- Rotate(theta, float32, around f32.Point) // in radians
- Shear(sx, sy float32, around f32.Point)
It could of course be discussed if the methods should have the around point
or not since it is equivalent to two Offsets. I would however
argue that it makes
for much easier use, and likely with no measurable loss of
performance when (0,0)
4. The overall changes would thus be along the lines of:
- Add the methods in op.TransformOp and update accordingly
- Add some test cases to TransformOp
- Change opconst.TypeTransformLen to fit the full matrix
- Update the encoding (op package) and decoding code (ops package)
5. Is that understanding correct, initially I had assumed that the transforms
were handled by the GPU, but looking at the code it looks like it
is all done
on the CPU and that those changes should be sufficient?
/ Viktor
+1 on this proposal.
On Thu, Jun 4, 2020 at 3:07 PM Viktor Ogeman <viktor.ogeman@gmail.com> wrote:
>> Hi,>> I am in need of transforms that include scaling, and find a TODO in the src> showing that this is on the table. If there is interest I would be> willing to try> to send a first patch implementing this.> Before I begin though it would be good to get clarification on a few items:>> 1. Still interest in getting this implemented?> 2. Agreed that the best way is to do it is to change op.TransformOp to be a> full (2D) affine transformation matrix instead of simply an offset?> 3. The API would this stay the same, with the following methods added> additionally to op.TransformOp:> - Scale(sx, sy float32, around f32.Point)> - Rotate(theta, float32, around f32.Point) // in radians> - Shear(sx, sy float32, around f32.Point)> It could of course be discussed if the methods should have the around point> or not since it is equivalent to two Offsets. I would however> argue that it makes> for much easier use, and likely with no measurable loss of> performance when (0,0)> 4. The overall changes would thus be along the lines of:> - Add the methods in op.TransformOp and update accordingly> - Add some test cases to TransformOp> - Change opconst.TypeTransformLen to fit the full matrix> - Update the encoding (op package) and decoding code (ops package)> 5. Is that understanding correct, initially I had assumed that the transforms> were handled by the GPU, but looking at the code it looks like it> is all done> on the CPU and that those changes should be sufficient?>> / Viktor
I think it will be clearer without the around point.
Having both `Scale` and `ScaleAt`/`ScaleAround` shouldn't be a problem.
+ Egon
On Thu, Jun 4, 2020 at 10:07 PM Viktor Ogeman <viktor.ogeman@gmail.com> wrote:
>> 3. The API would this stay the same, with the following methods added> additionally to op.TransformOp:> - Scale(sx, sy float32, around f32.Point)> - Rotate(theta, float32, around f32.Point) // in radians> - Shear(sx, sy float32, around f32.Point)> It could of course be discussed if the methods should have the around point> or not since it is equivalent to two Offsets. I would however> argue that it makes> for much easier use, and likely with no measurable loss of> performance when (0,0)>> / Viktor
On Fri, Jun 5, 2020 at 8:46 AM Egon Elbre <egonelbre@gmail.com> wrote:
>> I think it will be clearer without the around point.> Having both `Scale` and `ScaleAt`/`ScaleAround` shouldn't be a problem.>> + Egon
Noted,
I sent https://lists.sr.ht/~eliasnaur/gio-patches/patches/10907 as I
think it will be
easier to discuss it based on code rather than text.
/Viktor
>>> Noted,>> I sent https://lists.sr.ht/~eliasnaur/gio-patches/patches/10907 as I> think it will be> easier to discuss it based on code rather than text.>> /Viktor
Aa,
And now i note that the change was more invasive than I first thought,
the GPU code needs
to be modified as well. I will give it a go to see if I can figure out
a good way to do that in a second
patch - but after a quick look it seems as if there are several
options and might need discussing -
should e.g. all the shaders be updated to have affine transforms
instead of just offset and scale? Or
are there other options?
I'm unable to comment on the code directly due to source hut bizarreness.
Anyways, code looks neat, I'll add quick comments here,
> +type TransformOp [6]float32
Instead of using an array use fields. Fields work better with Go
optimizations and give clarity in which order the matrix is layed out.
Also changing from struct to an array is a backwards incompatible
change and exposes the internals of transform op. It's probably better
to avoid exposing it. Similarly add the comment of the structure near
the type itself, commit messages are useful for reviewers but not for
people reading the final code.
> +func (t TransformOp) Scale(sx, sy float32) TransformOp {
Use a f32.Point as argument to be consistent with Offset.
> +func (t TransformOp) Shear(shx, shy float32) TransformOp {
Maybe here as well, however, I'm less sure about this one.
Also not sure whether these should be in radians or the matrix components.
Using radians is probably more intuitive.
> +func (t TransformOp) Rotate(θ float32) TransformOp {
Avoid θ, they look cool, but can easily break editors and tools.
> return t.Offset(origin.Mul(-1))
I think you want to add Neg() to f32.Point.
> + return t.Multiply(offsetMat(o))
I don't think you need to do a full multiply for offsetting, an add on
tx/ty fields should be sufficient.
AFAIR, similar things apply to other transforms as well. I usually use
Mathlab/Octave/Sympy to figure out the optimized formula. e.g. see
https://github.com/egonelbre/hexapod/blob/master/exp/analysis/geometry.py> + c, s := math.Cos(theta), math.Sin(theta)
Use math.Sincos to have lower overhead.
On Sat, Jun 6, 2020 at 9:02 AM Viktor Ogeman <viktor.ogeman@gmail.com> wrote:
>> >> >> > Noted,> >> > I sent https://lists.sr.ht/~eliasnaur/gio-patches/patches/10907 as I> > think it will be> > easier to discuss it based on code rather than text.> >> > /Viktor>> Aa,>> And now i note that the change was more invasive than I first thought,> the GPU code needs> to be modified as well. I will give it a go to see if I can figure out> a good way to do that in a second> patch - but after a quick look it seems as if there are several> options and might need discussing -> should e.g. all the shaders be updated to have affine transforms> instead of just offset and scale? Or> are there other options?
FYI I implemented affine transforms 6 months ago but it was never
merged: https://lists.sr.ht/~eliasnaur/gio-patches/patches/9212
On Sat, Jun 6, 2020 at 11:18 AM Egon Elbre <egonelbre@gmail.com> wrote:
>> I'm unable to comment on the code directly due to source hut bizarreness.>> Anyways, code looks neat, I'll add quick comments here,>> > +type TransformOp [6]float32> Instead of using an array use fields. Fields work better with Go> optimizations and give clarity in which order the matrix is layed out.>> Also changing from struct to an array is a backwards incompatible> change and exposes the internals of transform op. It's probably better> to avoid exposing it. Similarly add the comment of the structure near> the type itself, commit messages are useful for reviewers but not for> people reading the final code.>> > +func (t TransformOp) Scale(sx, sy float32) TransformOp {> Use a f32.Point as argument to be consistent with Offset.>> > +func (t TransformOp) Shear(shx, shy float32) TransformOp {> Maybe here as well, however, I'm less sure about this one.> Also not sure whether these should be in radians or the matrix components.> Using radians is probably more intuitive.>> > +func (t TransformOp) Rotate(θ float32) TransformOp {> Avoid θ, they look cool, but can easily break editors and tools.>> > return t.Offset(origin.Mul(-1))>> I think you want to add Neg() to f32.Point.>> > + return t.Multiply(offsetMat(o))>> I don't think you need to do a full multiply for offsetting, an add on> tx/ty fields should be sufficient.>> AFAIR, similar things apply to other transforms as well. I usually use> Mathlab/Octave/Sympy to figure out the optimized formula. e.g. see> https://github.com/egonelbre/hexapod/blob/master/exp/analysis/geometry.py>> > + c, s := math.Cos(theta), math.Sin(theta)>> Use math.Sincos to have lower overhead.>> On Sat, Jun 6, 2020 at 9:02 AM Viktor Ogeman <viktor.ogeman@gmail.com> wrote:> >> > >> > >> > > Noted,> > >> > > I sent https://lists.sr.ht/~eliasnaur/gio-patches/patches/10907 as I> > > think it will be> > > easier to discuss it based on code rather than text.> > >> > > /Viktor> >> > Aa,> >> > And now i note that the change was more invasive than I first thought,> > the GPU code needs> > to be modified as well. I will give it a go to see if I can figure out> > a good way to do that in a second> > patch - but after a quick look it seems as if there are several> > options and might need discussing -> > should e.g. all the shaders be updated to have affine transforms> > instead of just offset and scale? Or> > are there other options?