Grail

Merge lp://staging/~oif-team/grail/pivot-disambiguation into lp://staging/grail

  1. pivot-disambiguation
  2. Merge into trunk
Proposed by Chase Douglas
Status: Merged
Merged at revision: 158
Proposed branch: lp://staging/~oif-team/grail/pivot-disambiguation
Merge into: lp://staging/grail
Diff against target: 707 lines (+96/-365)
6 files modified
include/grail.h (+13/-20)
include/grail.h.orig (+0/-277)
src/grail-frame.c (+65/-52)
src/grail-init.c (+0/-2)
tools/grail-test-mtdev.c (+2/-3)
tools/grail-transform.c (+16/-11)
To merge this branch: bzr merge lp://staging/~oif-team/grail/pivot-disambiguation
Reviewer Review Type Date Requested Status
Jussi Pakkanen (community) Approve
Stephen M. Webb (community) Approve
Review via email: mp+61443@code.staging.launchpad.net

Description of the change

The main crux of this merge request is to rework the pivot/anchor point calculation for the transformation matrix and other properties of the grail elements. This will hopefully remove any ambiguity around the pivot point. Mathematically, the output is equivalent to before if you take into account the full transformation matrix and center point.

I threw in two niceties: show the pivot point in red on top of any touch points in grail-transform, and removal of an erroneously added file.

To post a comment you must log in.
Revision history for this message
Jussi Pakkanen (jpakkane) wrote :

Lots of code removed. That's always good.

You change the header so this is clearly an API break. I assume that library versions have already been incremented since last release?

Detection against div by zero is done with this: if (div == 0). However floats seldom go to zero due to rounding errors. Maybe something like 'if abs(div) < SOME_THRESHOLD_DELTA_VALUE'?

Does the struct now have all info needed for Grip transformations? It would seem to me that these would be rotation_center and center are the transformation center points. Is this correct?

Revision history for this message
Chase Douglas (chasedouglas) wrote :

> Lots of code removed. That's always good.

Most of it is just the removal of an erroneous file, but I'll take any breaks I can get :).

> You change the header so this is clearly an API break. I assume that library
> versions have already been incremented since last release?

The library major version number has been incremented as part of the grail 2 work. This work hasn't been released yet, so the bump is still valid.

> Detection against div by zero is done with this: if (div == 0). However floats
> seldom go to zero due to rounding errors. Maybe something like 'if abs(div) <
> SOME_THRESHOLD_DELTA_VALUE'?

Good point, I'll fix this up in a new commit.

> Does the struct now have all info needed for Grip transformations? It would
> seem to me that these would be rotation_center and center are the
> transformation center points. Is this correct?

The rotation_center is not related to the transformation matrix, only the center is. Libgrip should expose the center point. The rotation_center is merely an extra piece of information for those interested in handling transformations without translation, but this isn't possible in the legacy geis v1/libgrip implementation.

Thanks!

lp://staging/~oif-team/grail/pivot-disambiguation updated
161. By Chase Douglas

Use a small threshold for numerical stability for rotation_center

Revision history for this message
Chase Douglas (chasedouglas) wrote :

Please review the appended commit for numerical stability checking.

Revision history for this message
Stephen M. Webb (bregma) wrote :

I would prefer the math markup to use doxygen-compatible markup, but I think that is better done in a separate documentation pass.

This merge does what it says it's going to do, so I have no objections. We need to keep the ball moving on this work.

review: Approve
Revision history for this message
Jussi Pakkanen (jpakkane) wrote :

Looks good.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: