Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft: Transformable elements proposal #5596

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Febbe
Copy link
Collaborator

@Febbe Febbe commented Apr 1, 2024

This PR tends to implement one of the most desired enhancements (#918), to rotate Images, Text and Latex.
For this, I replaced every explicit position, scale and rotation state in the Elements with affine transformations.
They are never applied to the data, instead the affine transformation is handled to cairo, which itself applies an affine transformation anyway.
This has the advantage,

  • that we get rotation support out of the box via cairo and
  • that we can reduce computing power, since only one transformation is applied to each Shape
  • when cairo drawing uses hardware acceleration, strokes now also will use hardware acceleration for the transformation

Currently, this is not ready to merge, since several code changes added bugs, which are not fixed yet.
But testers and interested people can test it out and give feedback.
Also, it's not possible, to store the transformations.

Animation

fixes #918
fixes #2040
fixes #3622
fixes #4899

It will require changes to #3931 and will simplify the implementation.
Only a mirror matrix must be applied to the selection with a pivot point/dimension.
E.g. Matrix(pivot) * mirrorMatrix * Matrix(-pivot)

@Febbe Febbe self-assigned this Apr 1, 2024
@Febbe Febbe added this to the v1.3.0 milestone Apr 1, 2024
@Febbe Febbe added enhancement breaking change This is or requires a change that will break existing files if there is no migration code. difficulty::hard do not merge This PR is not yet ready to be merged file format Requires changes to the file format PDF Related to PDF features Performance labels Apr 1, 2024
@Febbe
Copy link
Collaborator Author

Febbe commented Apr 2, 2024

@xournalpp/core we need to discuss what we want to do with the file format.
My proposal is, to

  • advance the version of it and
  • to add the ability, to export to an older version.
  • old versions should still be loadable.

But this will be again a lot of work.

automatical handling of attributes may decrease readability:
No good default, therefore using 'Leave'
 - added some constexpr to Rectangle and Point
 - made CairoWrapper `[[nodiscard]]`, its a guard and must not be discarded
 - seperation of changes, which do not belong to xournalpp#5596
 - increases readability of xournalpp#5596
This PR tends to implement one of the most desired enhancements (xournalpp#918), to rotate Images, Text and Latex.
For this, I replaced every explicit position, scale and rotation state in the Elements with affine transformations.
They are never applied to the data, instead the affine transformation is handled to cairo, which itself applies an affine transformation anyway.
This has the advantage,

   - that we get rotation support out of the box via cairo and
   - that we can reduce computing power, since only one transformation is applied to each Shape
   - when cairo drawing uses hardware acceleration, strokes now also will use hardware acceleration for the transformation
@rolandlo
Copy link
Member

rolandlo commented Apr 3, 2024

@xournalpp/core we need to discuss what we want to do with the file format. My proposal is, to

* advance the version of it and

* to add the ability, to export to an older version.

* old versions should still be loadable.

But this will be again a lot of work.

That sounds reasonable. How would you export a rotated images/text/TeXImages though to an older version?

@rolandlo
Copy link
Member

rolandlo commented Apr 3, 2024

  • when cairo drawing uses hardware acceleration,

Does it though? I do not think so. For hardware acceleration one has to use the Snapshot/Render Nodes approach in Gtk 4. I have created an example of that in Workbench (the Snapshot demo).

@Febbe
Copy link
Collaborator Author

Febbe commented Apr 3, 2024

@xournalpp/core we need to discuss what we want to do with the file format. My proposal is, to

* advance the version of it and

* to add the ability, to export to an older version.

* old versions should still be loadable.

But this will be again a lot of work.

That sounds reasonable. How would you export a rotated images/text/TeXImages though to an older version?

My idea is, to render the rotated PDFs onto a PDF surface and put it as binary data into the xournalpp file.

@Febbe
Copy link
Collaborator Author

Febbe commented Apr 3, 2024

  • when cairo drawing uses hardware acceleration,

Does it though? I do not think so. For hardware acceleration one has to use the Snapshot/Render Nodes approach in Gtk 4. I have created an example of that in Workbench (the Snapshot demo).

I am totally not sure. I've read something about, that this is implementation defined. Could be, that it only works on Linux with OpenGL or that I misinterpreted the section I've read.

But nevertheless this approach is also somewhat we definitely should do, when we want to switch to render nodes.
It also only applies one matrix transformation per rendering, instead of the extra transformations done for selections.

It can also simplify, the selection handling in general, since the selection just has to apply its transformation.

@rolandlo
Copy link
Member

rolandlo commented Apr 3, 2024

I am totally not sure. I've read something about, that this is implementation defined. Could be, that it only works on Linux with OpenGL or that I misinterpreted the section I've read.

The OpenGL support was removed and has basically unmaintained for a long time before. See e.g. https://news.ycombinator.com/item?id=39442886

I just wanted to argue about this fact about Cairo hardware acceleration, not about the general approach (which makes sense). Yes, in the Snapshot/RenderNodes world using a Gsk.TransformNode or gtk_snapshot_transform_matrix is much simpler than doing extra calculation.

One thing that looked troublesome to me with the transformation matrix approach is scaling strokes, since the stroke segments get distorted when the transformation is not a similarity. This makes a difference when scaling just in one direction. How do you plan to deal with that?

@Febbe
Copy link
Collaborator Author

Febbe commented Apr 3, 2024

One thing that looked troublesome to me with the transformation matrix approach is scaling strokes, since the stroke segments get distorted when the transformation is not a similarity. This makes a difference when scaling just in one direction. How do you plan to deal with that?

Right, I have this on my list. @bhennion has the approach, to add an extra parameter in his transformation matrix (don't have his branch on the hand right now).
But I would like to split the stroke size from the basic transformation.

My approach for strokes would be, to also add a transformation matrix for the stroke size, and to scale it like:

| TM * (z,z) |

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This is or requires a change that will break existing files if there is no migration code. difficulty::hard do not merge This PR is not yet ready to be merged enhancement file format Requires changes to the file format PDF Related to PDF features Performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rotate the added text Image rotation support Text rotation
2 participants