8000 Draft: Transformable elements proposal by Febbe · Pull Request #5596 · xournalpp/xournalpp · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Draft: Transformable elements proposal #5596

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.

Sign up for GitHub

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

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

Draft: Transformable elements proposal #5596

wants to merge 6 commits into from

Conversation

Febbe
Copy link
Collaborator
@Febbe Febbe commented Apr 1, 2024 •
edited
Loading

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 •
edited
Loading

@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.

Febbe added 6 commits April 2, 2024 13:04
@Febbe
clang-format improvement,
96872a7 
automatical handling of attributes may decrease readability:
No good default, therefore using 'Leave'
@Febbe
move Point from ::utl:: to xoj::util::
8d44f69 
 - added some constexpr to Rectangle and Point
 - made CairoWrapper `[[nodiscard]]`, its a guard and must not be discarded
@Febbe
Fixed Memory leaks for some GLibGuards, added out_ptr
83ce925 
 - similar to c++23 out_ptr
@Febbe
Added a 2D-Matrix 'Matrix' for affine transformations.
54e6642
@Febbe
trailing return types previously done in xournalpp#5596
ef1216f 
 - seperation of changes, which do not belong to xournalpp#5596
 - increases readability of xournalpp#5596
@Febbe
Implements general element rotation
85b5193 
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
@Febbe Febbe force-pushed the transformable_elements branch from 6ac6868 to 85b5193 Compare April 2, 2024 12:08
@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) |

@GithubAnon0000
Copy link

But testers and interested people can test it out and give feedback.

  1. How can I test this? Do I open this and build xournalpp according to the build instructions?
  2. What would you need to be tested, @Febbe? Just if rotating stuff works? Or also if errors / warnings are printing if build with debug flags on? Are there certain things / steps a "test" should include?

@Febbe
Copy link
Collaborator Author
Febbe commented Aug 28, 2024

But testers and interested people can test it out and give feedback.

  1. How can I test this? Do I open this and build xournalpp according to the build instructions?
  2. What would you need to be tested, @Febbe? Just if rotating stuff works? Or also if errors / warnings are printing if build with debug flags on? Are there certain things / steps a "test" should include?

Yes, you have to checkout my branch and build xournalpp yourself.

It's a proposal not a final implementation. So general Feedback about the look and feel is enough. E.g. a feature is missing. Rotating and scaling behaves unexpected.

Developers and maintainers should check build warnings and console/debug warnings themself before they merge. This is nothing a user should take care of.

@GithubAnon0000
Copy link

Building

So I build xournalpp from your repo on debian testing, which was inside gnome-boxes (a vm).

  1. Build xournalpp according to build instructions.
    · Install dependencies according to debian build instructions.
    · compile according to compile instructions.
mkdir build
cd build
cmake .. -DCMAKE_INSTALL_PREFIX=install
cmake --build .
cmake --build . --target install
./install/bin/xournalpp
  1. Installed texlive-latex-extra to fix latex not working in xournalpp.

Testing

Tested inserting and rotating…

  1. …image: worked perfectly fine! (used test.png)
  2. …latex: worked perfectly fine! (e.g. x^2)
  3. …text: fails! (see xournalpp_text_fails.mp4 and log below)
  4. …pen strokes: worked perfectly fine (except in the video it didn't. Steps to reproduce in "Pen Strokes not Rotatable")!
  5. …highlighter strokes: worked perfectly fine!
Log of writing text into xournalpp document (rotation not tested, since it's impossible) 

(com.github.xournalpp.xournalpp:8871): Pango-CRITICAL **: 10:25:25.297: pango_layout_set_attributes: assertion 'layout != NULL' failed
		
(com.github.xournalpp.xournalpp:8871): Pango-CRITICAL **: 10:25:25.297: pango_layout_set_text: assertion 'layout != NULL' failed
		
(com.github.xournalpp.xournalpp:8871): Pango-CRITICAL **: 10:25:25.297: pango_layout_index_to_pos: assertion 'layout != NULL' failed
		
(com.github.xournalpp.xournalpp:8871): Pango-CRITICAL **: 10:25:25.304: pango_cairo_update_layout: assertion 'PANGO_IS_LAYOUT (layout)' failed
		
(com.github.xournalpp.xournalpp:8871): Pango-CRITICAL **: 10:25:25.304: pango_layout_get_context: assertion 'layout != NULL' failed
		
(com.github.xournalpp.xournalpp:8871): Pango-CRITICAL **: 10:25:25.304: pango_context_set_matrix: assertion 'PANGO_IS_CONTEXT (context)' failed
		
(com.github.xournalpp.xournalpp:8871): Pango-CRITICAL **: 10:25:25.304: pango_cairo_show_layout: assertion 'PANGO_IS_LAYOUT (layout)' failed
		
(com.github.xournalpp.xournalpp:8871): Pango-CRITICAL **: 10:25:25.857: pango_cairo_update_layout: assertion 'PANGO_IS_LAYOUT (layout)' failed
		
(com.github.xournalpp.xournalpp:8871): Pango-CRITICAL **: 10:25:25.857: pango_layout_get_context: assertion 'layout != NULL' failed
		
(com.github.xournalpp.xournalpp:8871): Pango-CRITICAL **: 10:25:25.857: pango_context_set_matrix: assertion 'PANGO_IS_CONTEXT (context)' failed
		
(com.github.xournalpp.xournalpp:8871): Pango-CRITICAL **: 10:25:25.857: pango_cairo_show_layout: assertion 'PANGO_IS_LAYOUT (layout)' failed
		
(com.github.xournalpp.xournalpp:8871): Pango-CRITICAL **: 10:25:25.955: pango_cairo_update_layout: assertion 'PANGO_IS_LAYOUT (layout)' failed
		
(com.github.xournalpp.xournalpp:8871): Pango-CRITICAL **: 10:25:25.955: pango_layout_get_context: assertion 'layout != NULL' failed
		
(com.github.xournalpp.xournalpp:8871): Pango-CRITICAL **: 10:25:25.955: pango_context_set_matrix: assertion 'PANGO_IS_CONTEXT (context)' failed
		
(com.github.xournalpp.xournalpp:8871): Pango-CRITICAL **: 10:25:25.955: pango_cairo_show_layout: assertion 'PANGO_IS_LAYOUT (layout)' failed

–––> Rotation works fine, except for text which cannot be typed and the problem with pen strokes (see xournalpp_rotated.png).

Pen Strokes not Rotatable

Steps to reproduce:

  1. try to write something with the text tool.
  2. It fails (see log and video above).
  3. draw with the pen.
  4. Select stroke from pen.
  5. It cannot be rotated anymore (but latex, image, highlighter are not affected).

Loading file after saving it.

I know you said the following (but I still tested it):

Also, it's not possible, to store the transformations.

Testing loading saved file after insertion of rotated latex and pictures…

  1. …fails! (used test.xopp, result: xournalpp_file_opened_after_rotation.png. See log below.)
Log (loading saved file which includes rotated objects) 

(com.github.xournalpp.xournalpp:9070): Gtk-WARNING **: 10:32:09.340: drawing failure for widget 'GtkXournal': out of memory
		
(com.github.xournalpp.xournalpp:9070): Gtk-WARNING **: 10:32:09.340: drawing failure for widget 'GtkViewport': out of memory
		
(com.github.xournalpp.xournalpp:9070): Gtk-WARNING **: 10:32:09.340: drawing failure for widget 'GtkScrolledWindow': out of memory
		
(com.github.xournalpp.xournalpp:9070): Gtk-WARNING **: 10:32:09.340: drawing failure for widget 'GtkBox': out of memory
		
(com.github.xournalpp.xournalpp:9070): Gtk-WARNING **: 10:32:09.340: drawing failure for widget 'GtkPaned': out of memory
		
(com.github.xournalpp.xournalpp:9070): Gtk-WARNING **: 10:32:09.340: drawing failure for widget 'GtkBox': out of memory
		
(com.github.xournalpp.xournalpp:9070): Gtk-WARNING **: 10:32:09.340: drawing failure for widget 'GtkBox': out of memory
		
(com.github.xournalpp.xournalpp:9070): Gtk-WARNING **: 10:32:09.340: drawing failure for widget 'GtkBox': out of memory
		
(com.github.xournalpp.xournalpp:9070): Gtk-WARNING **: 10:32:09.340: drawing failure for widget 'GtkOverlay': out of memory
		
(com.github.xournalpp.xournalpp:9070): Gtk-WARNING **: 10:32:09.340: drawing failure for widget 'GtkApplicationWindow': out of memory

–––> loading fails with out of memory error (but multiple gb of memory are available and free)

Interessting enough this doesn't happen on my debian oldstable, which is running xournalpp v1.2.3 (gtk 3.24.24, compiled: mar 2 2024, 08:02:26) (see xournalpp_debian_oldstable.png).

Files and checksums

xournalpp_pull_5596.zip

sha512sum

771c9c5c3bdbaa50a7c3622884025b025d41d567017fcef68e1866f352bfda04e541ffedb71fcb0fc713bc6e8222a67d009c703be2084706081a1d5d270c49cc ./xournalpp_pull_5596.zip

sha256sum

66dbd4ff5ae7f5d3fe39ec9f9fa1c2b7ee0815db5c2b5f5ebfbd28cdacecfcbd ./xournalpp_pull_5596.zip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Febbe Febbe

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
Milestone
v1.3.0
Development

Successfully merging this pull request may close these issues.

Rotate the added text Image rotation support Text rotation
3 participants
@Febbe @rolandlo @GithubAnon0000
0