Merge lp://qastaging/~andreas-pokorny/mir/add-geometry-transformation into lp://qastaging/mir

FAILED: Continuous integration, rev:1473
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~andreas-pokorny/mir/add-geometry-transformation/+merge/211733/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1111/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1298
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1296
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/878
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/843
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/843/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/848
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/848/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/879
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/879/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/821
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4992

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1111/rebuild

All points I mentioned yesterday...

(1) "Transformation" is probably too generic a term for something with very specific operations like this. It needs a name that better distinguishes it from a common "mat4 transformation".

(2) Rotation is somewhat limited. We presently support rotations around arbitrary axes whereas this proposal limits that to only 3 possible axes. Although given the render_surfaces demo is probably going to be the only user of this, that's OK, but...

(3) As render_surfaces is the only user of rotations, rotations should not be explicitly built into the class. They should be provided by render_surfaces as simply a matrix.

(4) Only supporting one order of operations is too restrictive:
161 + * The transformation is defined as:
162 + * translate(translation) * translate(size*scale/2) * rotation(y,x,z) * scale * translate(-size*scale/2)
Transformations are arbitrary and should always be arbitrary. Imposing only one order and fixed set of operations would be an unacceptable step backwards.

Overall, I think the Transformation class is a mistake. Too restrictive in that it makes transformations less flexible, and more complicated at the same time. We should only deal in mat4's and never try to disassemble a mat4 into separate transformations.

I think the right answer is to expose a simple interface to convert screen coordinates (touch/pointer coords) into surface space by adding a function:

  geometry::Point Surface::screen_to_local(geometry:Point const& p) const
  {
      // calculate some answer using transformation(), top_left() and size() info.
      // You may even wish to ignore per-surface transformations and only return valid results
      // when transformation() == identity. In which case it could be as trivial as:
      // return p - top_left();
      // or more complicated like:
      // auto centre = top_left() + (size() / 2);
      // return inverse(transformation()) * (p - centre);
  }

That's a function instead of a class. It doesn't even need to be a method of Surface because it just uses existing public interface functions.

> All points I mentioned yesterday...
>
> (1) "Transformation" is probably too generic a term for something with very
> specific operations like this. It needs a name that better distinguishes it
> from a common "mat4 transformation".

I was looking for a new name.

> (2) Rotation is somewhat limited. We presently support rotations around
> arbitrary axes whereas this proposal limits that to only 3 possible axes.
> Although given the render_surfaces demo is probably going to be the only user
> of this, that's OK, but...

(2 and 4) Current status is a super set of what was in use so far.
Translation & scaling in the android input stack, and rotations and translation in our examples/surface code.
I want to enhance the interface in later steps. I think the necessary features in that domain are neither arbitrary nor infinite.

> (3) As render_surfaces is the only user of rotations, rotations should not be
> explicitly built into the class. They should be provided by render_surfaces as
> simply a matrix.

I disagree to such an interface, because
* makes the interface more complicated to use
* requires the caller to track additional state since manipulating a matrix incrementally is not practical.
* trivial positioning and orientation cases are not easy to detect.

> (4) Only supporting one order of operations is too restrictive:
> 161 + * The transformation is defined as:
> 162 + * translate(translation) * translate(size*scale/2) *
> rotation(y,x,z) * scale * translate(-size*scale/2)
> Transformations are arbitrary and should always be arbitrary. Imposing only
> one order and fixed set of operations would be an unacceptable step backwards.

see (2) and

>
> Overall, I think the Transformation class is a mistake. Too restrictive in
> that it makes transformations less flexible, and more complicated at the same
> time. We should only deal in mat4's and never try to disassemble a mat4 into
> separate transformations.

transformations that will be used in animations are arbitrary, and must remain so. You can't divide an arbitrary transformation matrix into a set of meaningful simple ones. Consider what happens in a cube-style animation where the windows fly away into the screen and layer onto a cube. The cube rotates around a point in space that is "inside the screen" and around an arbitrary axis determined by the mouse drag direction. That is a non-trivial surface transformation which involves translations and rotations in more combinations than this proposal supports. A 4x4 matrix is enough to represent such a transformation. However you cannot unwind that matrix into the simple set of steps used to create the matrix.

I recall sabdfl has used the cube example multiple times when asking about capabilities. Compiz/Ubuntu can do it right now and of course we need to build something better than Compiz.

And of course, multiple animations run at once. Not just one. So surface->transformation() could represent any number of animations being applied simultaneously.

Regarding "complicated", what I am suggesting is a simple function of only a few lines. That is less complicated than this proposal. Although my previous example contained a mistake. It should read:

  geometry::Point Surface::screen_to_local(geometry:Point const& p) const
  {
      auto centre = top_left() + (size() / 2);
      return (inverse(transformation()) * (p - centre)) + centre;
  }

Admittedly, that does require a full inverse.

> transformations that will be used in animations are arbitrary, and must remain
> so. You can't divide an arbitrary transformation matrix into a set of
> meaningful simple ones. Consider what happens in a cube-style animation where
> the windows fly away into the screen and layer onto a cube. The cube rotates
> around a point in space that is "inside the screen" and around an arbitrary
> axis determined by the mouse drag direction. That is a non-trivial surface
> transformation which involves translations and rotations in more combinations
> than this proposal supports. A 4x4 matrix is enough to represent such a
> transformation. However you cannot unwind that matrix into the simple set of
> steps used to create the matrix.

The issue is rather the lack of a z co-ordinate that would require ordering of the surfaces before they hit the screen. And the lack of perspective transformation inside the renderer. And then of course the then lacking but necessary ray casting to find the surface for input hit testing - or usage of points vs polygon to do hit testing.

Regarding rotations this case is already possible but requires calculating the final orientation of the surface as three angles and its position. Also not optimal to do.

Again I dont think that interface of the Thing meant to be an encapsulation of the transformations defining orientation and position and scaling of surface is done or complete - it is roughly what we have in use right now. So yes both implementation and interface will change a lot if we get the time to improve. But I think that is something we might evolve when turning mir::scene into a more complete scene graph?

> I recall sabdfl has used the cube example multiple times when asking about
> capabilities. Compiz/Ubuntu can do it right now and of course we need to build
> something better than Compiz.
>
> And of course, multiple animations run at once. Not just one. So
> surface->transformation() could represent any number of animations being
> applied simultaneously.
>
> Regarding "complicated", what I am suggesting is a simple function of only a
> few lines. That is less complicated than this proposal. Although my previous
> example contained a mistake. It should read:
>
> geometry::Point Surface::screen_to_local(geometry:Point const& p) const
> {
> auto centre = top_left() + (size() / 2);
> return (inverse(transformation()) * (p - centre)) + centre;
> }
>
> Admittedly, that does require a full inverse.

Additionally that does not simplify the example above either. And as you noticed you would inverse on each transformation, which is something I thought we both agreed to be bad.

I believe we can find a better way to describe arbitrary placed surfaces than to do:
  void set_transformation( mat4 worldToLocal, mat4 localToWorld);

@tranformation being just completely the wrong approach
personally I can see that a generic and yet simple (meaning non-arbitrary) convenience interface might still be useful, as in the definition of convenient. I guess the test question to ask is...is that something people would find useful to use in general, e.g. is it common enough??
i see the argument for rotation it'd only used by render_surfaces demo today, but would there be other customers potentially?

@ providing a simple function
looks like more arguing ahead :) might just be worth it to deliver a simple function to progress. Please consider.

@inverse - why is it bad ?

@ finding a better way for arbitrary surface positioning and mat4
my thinking would be, some conveniences are worthhwile, but at the same time, surely there's a breakover point where you would just use set_transformation( mat4 worldToLocal, mat4 localToWorld);
It goes back to the test question...if we build a better interface, who will use it?

"lack of a z coordinate" is both untrue, and not an issue. It's important to remember that stacking depth does not and will not affect the Z coordinate used to render a surface. The stacking depth only affects the surface's position in the render list. Surfaces are all given zero Z coordinates by default, but that can and will change as the result of transformations.

We do support Z coordinates, although they're all set to zero right now. And we can easily support perspective projection in the renderer. Just no one has implemented it yet.

That does raise the issue of you technically having to include the display_transformation to get accurate input coordinates, which we're not doing. I've repeated this point every time we've discussed the topic. You can either choose to ignore full-screen transformations for input handling, or somehow work to extract that info from the renderer. Either is feasible but I'd prefer the former.

Always remember: a 4x4 matrix represents ANY number of transformations in ANY order. That's a fact that all graphics programmers depend on. And we should not in any way limit the number or order of transformations that a single matrix can represent.

Basic 3D perspective implemented:
https://code.launchpad.net/~vanvugt/mir/perspective/+merge/212085

Strange world. This all started when I noticed that the transformation in BasicSurface was not used properly in the class and in input stack - ignored in some cases. When I suggested to fix that there were performance concerns, and concerns that we never need transformations at all. So I thought a way to "support" both by making it easy to detect possible short cut, not because it has a performance impact, but to avoid the concerns.

If we can settle on making mir calculate co-ordinates correctly in every case I am happy to not have that MP go into devel.