letsfindaway / OpenBoard

I'm using this fork to contribute features and fixes to the upstream project. In order to create good pull requests, I'm rebasing my feature branches, squashing and reordering commits, etc. If you fork this repository be aware that my development branches may rewrite history without prior notice.
http://openboard.ch/
GNU General Public License v3.0
9 stars 0 forks source link

PDF export scaling issue #93

Closed letsfindaway closed 1 year ago

letsfindaway commented 2 years ago

Differences between the documents

Property Mac Linux Factor ubz
Document paper size 196 × 279 mm 210 × 300 mm 1,07 210 × 300 mm
Font size "ça vient de la..." 9,3 pt 10,6 pt 1,14 11,0 pt
Position "ça vient de la..." 1,15 / 2,92 cm 1,27 / 3,09 cm 1,10
Font size "Des Helvètes" 15,87 pt 18,1 pt 1,14
Top margin until line 2,5 cm 1,3 cm 2,9 cm
Left margin 2,7 cm 2,9 cm 1,9 cm
Right margin 1,6 cm 0,8 cm 1,9 cm
Width of blue box 14 cm 16 cm 1,14 16 cm

So we see that both parts coming from the original document like the blue box and right and left margin as well as parts coming from OpenBoard like the added "ça vient de la..." are affected and are exported differently. BTW: It looks again different when I do the export here on openSUSE using 1.7-dev. However here pages 1 and 2 are quite ok, while on page 3 there is a remarkable offset. Form the header I can also see, that the PDF document is scaled differently on these pages. That's strange and I would never expect that.

letsfindaway commented 2 years ago

The journey of a PDF through OpenBoard

Let's take a look at the journey of a PDF from import over displaying and amending to export with merging. How does this happen? What scaling factors are used?

As an example we will focus on the blue box on the first page and check its size and position.

The original PDF document

The original document has a size of 210 x 300 mm. For a 1:1 scaling on my monitor I have to zoom it to 106.5%. But this does not matter, it is just the scaling for my system. However I set this scaling and measure the size and position of the box with this setting.

The blue box has the following dimensions:

Import to OpenBoard

Now let's check how the blue box look like when this PDF document is imported to OpenBoard. On my monitor without additional zoom it has the following dimensions:

If I however use the ruler to measure the dimensions, I get the following results:

Export using PDFMerger

To be able to analyze the input documents used by the PDFMerger we start OpenBoard with the -v command line argument. This puts it into verbose mode, where the overlay PDF is kept in the output folder. So we can analyze this intermediate document, too.

First of all, the overlay has the same size than the original document, 210 x 300 mm. I was then printing the original PDF and the overlay PDF and move the sheets on top of each other. Here I can see that the overlay already has the wrong scaling and does not fit.

There are indeed some complicated calculations for scaling the overlay, and we should analyze what they intend to do. I would assume that the calculation is easy: we just have to know how the document was scaled to be the background, and that's it.

letsfindaway commented 2 years ago

Analysis of scaling calculation

Here is the code used to calculate the transformation together with the values I got for the four pages when debugging it:

    // Original datas
    double xAnnotation = qRound(annotationsRect.x());                   // -442 / -397 / -397 / -397
    double yAnnotation = qRound(annotationsRect.y());                   // -567 / -567 / -567 / -567
    double xPdf = qRound(pdfItem->sceneBoundingRect().x());             // -397 / -397 / -397 / -397
    double yPdf = qRound(pdfItem->sceneBoundingRect().y());             // -567 /-567 / -567 / -567
    double hPdf = qRound(pdfItem->sceneBoundingRect().height());        // 1133 / 1133 / 1133 / 1133

    // pageSize: (793, 1133) / (793, 1133) / (793, 1133) / (793, 1133)
    // annotationsRect: (838.3, 1132.9) / (793, 1132.9) / (793, 1132.9) / (793, 1132.9)
    // Exportation-transformed datas
    double hScaleFactor = pageSize.width()/annotationsRect.width();     // 0.945956 / 0.999913 / 0.999913 / 0.999913
    double vScaleFactor = pageSize.height()/annotationsRect.height();   // 1.000087 / 1.000087 / 1.000087 / 1.000087
    double scaleFactor = qMin(hScaleFactor, vScaleFactor);              // 0.945956 / 0.999913/ 0.999913 / 0.999913

    double xAnnotationsOffset = 0;
    double yAnnotationsOffset = 0;
    double hPdfTransformed = qRound(hPdf * scaleFactor);                // 1072 / 1133 / 1133 / 1133

    // Here, we force the PDF page to be on the topleft corner of the page
    // mScaleFactor: 0.75 / 0.75 / 0.75 / 0.75
    double xPdfOffset = 0;
    double yPdfOffset = (hPdf - hPdfTransformed) * mScaleFactor;

    // Now we align the items
    xPdfOffset += (xPdf - xAnnotation) * scaleFactor * mScaleFactor;    // 31.926032 / 0 / 0 / 0
    yPdfOffset -= (yPdf - yAnnotation) * scaleFactor * mScaleFactor;    // 45.75 / 0 / 0 / 0

    // If the PDF was scaled when added to the scene (e.g if it was loaded from a document with a different DPI
    // than the current one), it should also be scaled here.
    qreal pdfScale = pdfItem->scale();                                  // 1.0000003 / 1.0000003 / 0.93749 / 1.0625

    TransformationDescription pdfTransform(xPdfOffset, yPdfOffset, scaleFactor * pdfScale, 0);
    TransformationDescription annotationTransform(xAnnotationsOffset, yAnnotationsOffset, 1, 0);

When looking at the final TransformationDescription we see that the PDF document is scaled, while the annotations have no transformation. Both xAnnotationsOffset and yAnnotationsOffset are initialized to 0 and not changed, and the scaling factor is fixed to 1. But the PDF document is scaled and shifted.

Just out of curiosity I fixed pdfScale to 1.0. After that, the merged document looked fine! So we should probably look at the comment above this line:

If the PDF was scaled when added to the scene (e.g if it was loaded from a document with a different DPI than the current one), it should also be scaled here.

Where is the scale of the PDF item set and how? Why is it different from 1 for pages 3 and 4?

Setting the scale occurs in the UBSvgSubsetAdaptor:844:

    qreal pdfScale = qreal(proxy->pageDpi())/currentDpi;
    // qDebug() << "pdfScale " << pdfScale;

    // If the PDF is in the background, it occupies the whole page; so we can simply
    // use that information to calculate its scale.
    if (isBackground) {
        qreal pageWidth = mScene->nominalSize().width();
        qreal pageHeight = mScene->nominalSize().height();

        qreal scaleX = pageWidth / pdfItem->sceneBoundingRect().width();
        qreal scaleY = pageHeight / pdfItem->sceneBoundingRect().height();

        pdfScale = (scaleX+scaleY)/2.;
    }

    pdfItem->setScale(pdfScale);

pdfScale starts with a calculation based on DPI, which returns 1 in my case for all four pages. The following calculation for a background PDF however returns different scaling factors for page 3 and 4.

letsfindaway commented 2 years ago

Proposed solution

My proposal would be to calculate the pdfScale in UBExportFullPDF in the same way as the DPI ratio is calculated in UBSvgSubsetAdaptor:

So instead of

    qreal pdfScale = pdfItem->scale();

we write

    qreal currentDpi = UBApplication::displayManager->logicalDpi(ScreenRole::Control);
    qreal pdfScale = qreal(pDocumentProxy->pageDpi())/currentDpi;

At least for me this fixes the problem and it looks reasonable. However I must admit that I still do not fully understand all that scaling... And the question remains, whether a fixed 1.0 would not be the right thing: we do not want to change the original PDF, we just want to add an additional layer of annotations.

kaamui commented 2 years ago

Hi Martin,

thank you for this great analysis. I'll try to complete this thread with my own tests to pursue the discussion.

On the SVG side

I looked at the OpenBoard pages and saw that, for some reason page 3 (page002.svg) and 4 (page003.svg) had stored a different value for the width, height and transform attributes of the pdf object.

page 1 & 2 : 
<foreig...  width="792.739" height="1132.86" transform="matrix(0.999156, 0, 0, 0.999156, -396.869, -566.929)" .../>
page 3
<foreig...  width="744.13" height="1062.99" transform="matrix(1.06577, 0, 0, 1.06577, -396.869, -566.929)" .../>
page 4
<foreig...  width="843.347" height="1204.72" transform="matrix(0.940382, 0, 0, 0.940382, -396.869, -566.929)" .../>

I extracted the original pdf and re-imported it to see what would be the new pages produced by OpenBoard 1.6.3 and for the 4 pages I got this :

- on a linux machine with 1.6.3
...  width="792.739" height="1132.86" transform="matrix(1, 0, 0, 1, -396.869, -566.929)" ...

- on a macbook with 1.6.3
...  width="1057.32" height="1510.81" transform="matrix(1, 0, 0, 1, -529.159, -755.906)" ...

After that I tested something else, reproducible with these steps :

  1. import original ubz on the macbook
  2. modified page 1 and 3 so pdf attributes are rewritten
  3. export as ubz
  4. import new ubz on the linux machine
  5. export as pdf

the result is that only the page 2 is now correctly scaled.

The svg files now contain :

page 1
... width="1057.32" height="1510.81" transform="matrix(0.749367, 0, 0, 0.749367, -396.869, -566.929)" ...

page 2
... width="792.739" height="1132.86" transform="matrix(0.999156, 0, 0, 0.999156, -396.869, -566.929)" ...

page 3
... width="1057.32" height="1510.81" transform="matrix(0.749367, 0, 0, 0.749367, -396.869, -566.929)" ...

page 4 
... width="843.347" height="1204.72" transform="matrix(0.940382, 0, 0, 0.940382, -396.869, -566.929)" ...

On the code side

I found that the fix I had added with 1a425b015d and maintained in 493b6ab72 was not applied on the UBExportFullPDF::persistsDocument function (to call sceneSize instead of nominalSize)

sceneSize is defined as follows

https://github.com/letsfindaway/OpenBoard/blob/4b040fcf0ed0829c3e2cb260175224a07e23447e/src/domain/UBGraphicsScene.cpp#L2557-L2573

As you noted the pdf scale calculation is based on the same sceneBoundingRect() when the scene is loaded for the first time

https://github.com/letsfindaway/OpenBoard/blob/4b040fcf0ed0829c3e2cb260175224a07e23447e/src/adaptors/UBSvgSubsetAdaptor.cpp#L840-L865

https://github.com/letsfindaway/OpenBoard/blob/d4b91d8d34744ff33ca4d16e02dd60041cd82c52/src/adaptors/UBExportFullPDF.cpp#L209-L243

So I was expecting the replacement of QSize pageSize = scene->nominalSize(); with QSize pageSize = scene->sceneSize(); to fix the issue, but it didn't. What happens is that if you launch OpenBoard, and try to export the document, each scene will be loaded to be exported. When called on UBSvgSubsetReader::loadScene, pdfItem->sceneBoundingRect() returns :

page 1
QRectF(-396.869,-566.929 793.069x1132.9)
page 2
QRectF(-396.869,-566.929 793.069x1132.9)
page 3
QRectF(-396.869,-566.929 845.943x1208.43)
page 4
QRectF(-396.869,-566.929 746.417x1066.26)

But right after, at export (when persistsDocument is called), it returns :

page 1
QRectF(-396.869,-566.929 793.069x1132.9)
page 2
QRectF(-396.869,-566.929 793.069x1132.9)
page 3
QRectF(-396.869,-566.929 793.069x1132.9)
page 4
QRectF(-396.869,-566.929 793.069x1132.9)

Thus, when the initial pdfScale value is retrieved, it no longer corresponds to the sceneBoundingRect initially associated.

Next step for me is to track what is affecting this value between the two calls. and understand if it should be affected, and if not to fix it. If it is supposed to be affected depending on the screen+machine I'm using, I wonder what is the interest to persist it.

On the concept side

To be honest, I'm not quite familiar with DPI and other technical aspects involved here. I'm not sure of what would be the correct answer between the two fixes (use pdf sceneBoundingRect or change pdfScale calculation) we found. There is a lot of aspects involved in this issue, and I wonder if some information is redundant, what is still useful, what could be simplified, improved.

As you said, all we want to guarantee is that a pdf document and its annotations match perfectly on every machine we open it on, and that scale remains approximately the same on every device, as far as possible.

PS : a simple workaround is to force the save of each page that is not scaled correctly by quickly modify/save/undo before exporting the document to PDF. But you'll have to relaunch OpenBoard in between so the document is reloaded with the modified attributes.

Edit : setScale is not just a setter of a mScale data member but performs the scaling (sounds obvious but for some reason I was not expecting it).

qDebug() << "loadScene : before setScale => sceneBoundingRect = " << pdfItem->sceneBoundingRect();
pdfItem->setScale(pdfScale);
qDebug() << "loadScene : after setScale => sceneBoundingRect = " << pdfItem->sceneBoundingRect();

gives  : 
loadScene : before setScale => sceneBoundingRect =  QRectF(-396.869,-566.929 594.801x849.676)
loadScene : after setScale => sceneBoundingRect =  QRectF(-396.869,-566.929 793.069x1132.9)

so, indeed, it cannot work as is.

If the scaling is performed each time the scene is loaded, pdfScale should be ignored when calculating the transformationDescription scale value of the pdf as it has already been applied. What do you think ?

kaamui commented 2 years ago

After this journey through pdf scale calculation, I think your pull request is the best answer, as it also takes into account the use case of a document with a different DPI, as stated in the commentary.

If you share the same conclusion, I'll accept the pull request.

I wonder what's involved in the determination of the width, height and transform attributes of the pdf, apart from DPI, and if DPI varies upon screens, or only OS.

letsfindaway commented 2 years ago

Hi @kaamui,

after reconsidering your analysis and the approaches to solve them, I still think my proposal is ok. Additionally I try to add some more considerations here.

Why and when is PDF scaled?

At first thought one may argue that the background PDF should not be scaled and just overlaid with the annotations. OpenBoard takes this approach as long as the annotations fit on the page. As soon as annotations are made outside of the original area of the PDF page, scaling is performed: The PDF page is shrunk so that all annotations fit on the output page. Additionally, proper positioning is necessary. This is what the code in UBExportFullPDF is doing. There should not be any additional scaling for that anywhere else, because this is just a matter of export.

DPI values

DPI values are present for the screen as well as for the PDF document. Let's start with the screen. Here I already did some investigations during my work on the display manager, and here are my results:

The logical DPI is taken when specifying coordinates with Qt. So if you have e.g. a widget of size 40x100px, then this is expressed as logical pixels. Taking the above example of a 125% scaling, this widget occupies 50x125 physical pixels on that display.

All the "normal" Qt functions working on size, position etc are using logical pixels. Due to the scaling described above, the actual physical pixels may not even be integer numbers. Therefore it is advisable to use the floating point functions for sizes when something must really be accurate. All the QGraphics... objects already do this.

Now for the PDF: Here I have much less information. I just assume that the DPI value is used when rendering graphics or an image to a bitmap in the PDF. So I assume it is more a quality thing than a scaling thing. It affects exporting in the mScaleFactor, but I think that is just for the API of the PDF merger.

Final question

Now my final question: will you fix this on the dev branch or on the 1.7-dev branch? My pull request is currently written and targeted to 1.7-dev. I can rework it for dev, if you plan a 1.6.4 version before the 1.7 release.

kaamui commented 2 years ago

Please do. I think 1.7 is not going to be released soon, and I find this issue quite important (we have some configurations where teachers prepare their work on a mac, export it, use it on a linux (and thus modify it), so the use case is probably going to happen again).

Thank you for your help !

kaamui commented 2 years ago

There should not be any additional scaling for that anywhere else, because this is just a matter of export.

I think scaling is done when the scene is loaded because it is also a matter of rendering. The pdf item (QGraphicsItem) needs to be scaled to fit the parameters of the current scene, otherwise it could look too small or too big depending on the width, height and transform values that could have been persisted on another configuration

letsfindaway commented 2 years ago

Please do. I think 1.7 is not going to be released soon, and I find this issue quite important (we have some configurations where teachers prepare their work on a mac, export it, use it on a linux (and thus modify it), so the use case is probably going to happen again).

Thank you for your help !

I will modify this pull request to work with 1.6 and 1.7 code. Currently it depends on the new display manager. I will add some preprocessor magic to do the right thing for 1.6 and 1.7.

letsfindaway commented 2 years ago

There should not be any additional scaling for that anywhere else, because this is just a matter of export.

I think scaling is done when the scene is loaded because it is also a matter of rendering. The pdf item (QGraphicsItem) needs to be scaled to fit the parameters of the current scene, otherwise it could look too small or too big depending on the width, height and transform values that could have been persisted on another configuration

Yes, but the exported PDF should keep the size. As an example: I imported a very small PDF, more or less a letter stamp. For display, it was scaled to occupy the full screen. The background grid also appeared "larger". When exporting again, the resulting PDF has the same small page size as the original. So yes, scaling occurs for rendering. But this is not relevant for export, where the PDF scaling is relative to the original PDF size.

kaamui commented 2 years ago

Yes, but the exported PDF should keep the size.

Why it should be kept ? For me, what a user should be assured is that what he sees in OpenBoard is what he gets in exported PDF. Like what you tried to achieve with background grid and color.

Let's say you have an A4 document, where you insert an A5 pdf page, that is scaled in OpenBoard. Should you have an exported PDF where the initially A5 page looks smaller than the other pages, where it's not the case in OpenBoard ?

letsfindaway commented 2 years ago

Let's say you have an A4 document, where you insert an A5 pdf page, that is scaled in OpenBoard. Should you have an exported PDF where the initially A5 page looks smaller than the other pages, where it's not the case in OpenBoard ?

Explain more about that. How should OpenBoard handle that?

What I tried:

I created a new OpenBoard document by using the Import menu in the Document view. The imported PDF was single page and very small, just 90x40mm. This creates a page in OpenBoard which resembles that size, when compared to the grid size, but is scaled to fill the screen. I think I can add this advertising here;) : grafik You can't see it very good here, but the grey page size box has exactly the size of the document. Then I added more pages from a PDF document with size A4 by dragging a PDF document from the file viewer to the board. After that, the thumbnails look as follows: grafik (Note that I had to close and reopen the document to refresh the thumbnails, but this is another issue). The first page has the 90x40mm size, the second and third have A4 and are also presented like that on the board.

When I now export that document, then the resulting PDF has again exactly those sizes: grafik And that is exactly what I expect and want.

Not the pages are scaled by OpenBoard when importing, only the view of the scene is scaled.

kaamui commented 2 years ago

Explain more about that. How should OpenBoard handle that?

My point was that an OpenBoard document should have a default format when imported (like A4 or depending on the first page imported) and everything imported (like your 90x40 example) should be treated as an object inserted in a page of this format. Also because exported documents can be printed and I wonder what would be the behavior with plenty different formats on a same document. But I'll agree that it is questionable. I'm not sure at this point what is the best (i.e most wanted) behavior.

We should continue this discussion, but there is more ! A user contacted me with a PDF export scaling issue that is not the same, and occurring with the 1.6.4rc-0913 version too. Other the years, this user created a document that is a really good one to practice and challenge OpenBoard export functions ^^.

I think I've found a fix but I still don't fully understand the full picture of a PDF's journey through OpenBoard and would like your advice/review on the issue and on the fix, if you will, as you seem to better understand what's happening here.

My personal understanding is that the m11/m22 (I don't think we make it possible for them to differ) transformations of the pdfItem of a page are not taken into account when pdfMerger PageDescription is set, thus making pdfMerge not aware that the width and height it uses are relative to the scale used.

I think the scaling has to be provided to PdfMerger and is used here :

https://github.com/letsfindaway/OpenBoard/blob/d4b91d8d34744ff33ca4d16e02dd60041cd82c52/src/pdf-merger/Page.cpp#L157

here's the document, if you want to try it out : https://drive.google.com/file/d/1co516X2zXg1PlmA8WXYPjfWeGJRVMpcL/view

And the fix I added here : https://github.com/letsfindaway/OpenBoard/blob/d4b91d8d34744ff33ca4d16e02dd60041cd82c52/src/adaptors/UBExportFullPDF.cpp#L245

is TransformationDescription pdfTransform(xPdfOffset, yPdfOffset, scaleFactor * pdfScale * pdfItem->sceneTransform().m11(), 0);

Thank you in advance, if you have time and you're OK to look at it.

kaamui commented 2 years ago

I also noted that the exported PDF does not match perfectly, even with my fix :

exported PDF : image

OpenBoard view : image

It feels to me like a small gap in the y axis, so maybe not related to the scaling issues.

letsfindaway commented 2 years ago

I will have a look at this over the weekend. Bad weather forecast here, so indoor activity is ok ;)

letsfindaway commented 2 years ago

Observations

Where to look

Let's take a step back. The problem is not necessarily in UBExportFullPDF. Other ideas:

Transformations

The transformations applied to a QGraphicsItem are quite complex. Here the respective paragraph from the documentation of that class:

Transformations

QGraphicsItem supports projective transformations in addition to its base position, pos(). There are several ways to change an item's transformation. For simple transformations, you can call either of the convenience functions setRotation() or setScale(), or you can pass any transformation matrix to setTransform(). For advanced transformation control you also have the option of setting several combined transformations by calling setTransformations(). Item transformations accumulate from parent to child, so if both a parent and child item are rotated 90 degrees, the child's total transformation will be 180 degrees. Similarly, if the item's parent is scaled to 2x its original size, its children will also be twice as large. An item's transformation does not affect its own local geometry; all geometry functions (e.g., contains(), update(), and all the mapping functions) still operate in local coordinates. For convenience, QGraphicsItem provides the functions sceneTransform(), which returns the item's total transformation matrix (including its position and all parents' positions and transformations), and scenePos(), which returns its position in scene coordinates. To reset an item's matrix, call resetTransform(). Certain transformation operations produce a different outcome depending on the order in which they are applied. For example, if you scale an transform, and then rotate it, you may get a different result than if the transform was rotated first. However, the order you set the transformation properties on QGraphicsItem does not affect the resulting transformation; QGraphicsItem always applies the properties in a fixed, defined order:

  • The item's base transform is applied (transform())
  • The item's transformations list is applied in order (transformations())
  • The item is rotated relative to its transform origin point (rotation(), transformOriginPoint())
  • The item is scaled relative to its transform origin point (scale(), transformOriginPoint())

Which of those functions are we using when importing the PDF? And which shall we use to query the overall scaling and transformation? I think the latter is clearly written here: sceneTransform() should return the combined transformations.

letsfindaway commented 2 years ago

Ideas for fixes

Remove duplicate scaling

With this information one fix is obvious:

// Change
    TransformationDescription pdfTransform(xPdfOffset, yPdfOffset, scaleFactor * pdfScale, 0);
// not to 
    TransformationDescription pdfTransform(xPdfOffset, yPdfOffset, scaleFactor * pdfScale * pdfItem->sceneTransform().m11(), 0);
// but to
    TransformationDescription pdfTransform(xPdfOffset, yPdfOffset, scaleFactor * pdfItem->sceneTransform().m11(), 0);

because sceneTransform() already includes scale(). But still we have these little ugly offsets...

No rounding

Another thing is to remove al these calls to qRound and to use QSizeF etc in all places. But even that does not fix the small offsets.

letsfindaway commented 2 years ago

My point was that an OpenBoard document should have a default format when imported (like A4 or depending on the first page imported) and everything imported (like your 90x40 example) should be treated as an object inserted in a page of this format. Also because exported documents can be printed and I wonder what would be the behavior with plenty different formats on a same document. But I'll agree that it is questionable. I'm not sure at this point what is the best (i.e most wanted) behavior.

Just note that the page sizes on the exported document are quite different: grafik I think the PDF viewer should cope with the question how to bring these pages to an A4 format paper, and typically they offer some options like "Original size", "fit to page", etc.

letsfindaway commented 2 years ago

More about the journey of a PDF

Import

When importing a PDF as background, the PDF's page size is set as scene nominal size:

void UBImportPDF::placeImportedItemToScene(UBGraphicsScene* scene, UBGraphicsItem* item)
{
    UBGraphicsPDFItem *pdfItem = (UBGraphicsPDFItem*)item;
    pdfItem->setPos(-pdfItem->boundingRect().width() / 2, -pdfItem->boundingRect().height() / 2);
    scene->setAsBackgroundObject(pdfItem, false, false);
    scene->setNominalSize(pdfItem->boundingRect().width(), pdfItem->boundingRect().height());
}

The unit of the size is determined by the DPI value set for the renderer. UBImportPDF sets this to the physical DPI of the desktop, e.g. 96. This means that these values are measured in pixels of the screen where the document was imported. I think it is an unlucky choice to make such a value dependent on the environment of the import, but this is OpenBoard's history.

in the test documents, there is a scaling factor of 0.75 in the transformation matrix and the nominal size is also smaller by factor 0.75. This can probably happen if someone opens this document on a computer with a DPI of 72, and the scale factors are adapted. Probably there are also other mechanisms. It is however important to note that this is possible.

Things would be easier if OpenBoard could use one unit of measurements in the persisted document not depending on any screen resolutions of the arbitrary computer where this document was created or modified.

Saving and loading scenes

As the size is persisted as ub:nominal-size, we know the size of the background PDF when later loading it even before we read the PDF document. Indeed, when the UBSvgSubsetAdaptor later creates the scene, it does not take the nominal size from the PDF document, but from that attribute. In theory this could lead to mismatches between those two, but I have not seen that in practice.

Export

For export you now carefully have to consider the unit of each parameter you're working with and the scale factors which should be applied.

kaamui commented 2 years ago
  • However with this fix the other document (Helvetes) which I analyzed in the beginning again has remarkable offsets.

Oh that's strange ! I obviously tested it before starting thinking my change could be a fix. I tested numerous files and I didn't have any of the big scaling issues we previously had..

I'll make more tests to see how I can reproduce it.

And I'll try your fix, but I wonder : in UBExportFullPDF::persistsDocument, with the current implementation, pdfScale refers to a ratio between the pageDpi and the "currentDpi", where in UBSvgSubsetAdaptor::loadScene it is calculated using a ratio between the scene's nominal size and the bounding rect of the pdf item, when the pdf is in background. So in this case, I don't see how the first ratio could still be taken into account in the matrix.

letsfindaway commented 2 years ago

And I'll try your fix, but I wonder : in UBExportFullPDF::persistsDocument, with the current implementation, pdfScale refers to a ratio between the pageDpi and the "currentDpi", where in UBSvgSubsetAdaptor::loadScene it is calculated using a ratio between the scene's nominal size and the bounding rect of the pdf item, when the pdf is in background. So in this case, I don't see how the first ratio could be taken into account in the matrix.

I'm still struggling with all those scaling factors. Let me try to put them in words, and hope, that it is correct:

Having scene coordinates using the DPI of the computer where the document was created and others using the DPI where the document is processed is very ugly and I do not really know why this should be necessary at all. There is no need to adapt any items to the DPI value of the display. An exception may be PDF rendering, but rendering only! Here it makes sense to specify the resolution to the renderer.

I wish some future version of OpenBoard could use a device independent and uniform coordinate system, e.g. using Points or Millimeters. Everything else should be an issue of displaying a scene, but should never be needed to be saved in the document's svg files.

And let me now compare some of the SVG files we have:

Attribute Helvètes
page003.svg
Energie
page002.svg
Business card
page001.svg
ub:nominal-size 793x1133 531x766 340x151
pageDpi 96 96 96
foreignObject width 843.347 707 339
foreignObject height 1204.72 1020.33 150
foreignObject transform matrix(0.940382, 0, 0, 0.940382, -396.869, -566.929) matrix(0.75, 0, 0, 0.75, -265.5, -383) matrix(1, 0, 0, 1, -170, -75.5)

Observations:

kaamui commented 2 years ago
  • pageDpi does not vary for those files. This is an additional variable, where I do not have and example with another value.

on the file I attached the pageDpi of all pages - except page007 - is 72

https://drive.google.com/file/d/1co516X2zXg1PlmA8WXYPjfWeGJRVMpcL/view

kaamui commented 2 years ago

Having scene coordinates using the DPI of the computer where the document was created and others using the DPI where the document is processed is very ugly and I do not really know why this should be necessary at all. There is no need to adapt any items to the DPI value of the display. An exception may be PDF rendering, but rendering only! Here it makes sense to specify the resolution to the renderer.

I totally agree about the necessity of simplifying the overall handling of these aspects. I'm still having a hard time bringing all the pieces together though. The nominal-size concept/implementation makes me uncomfortable...

I wish some future version of OpenBoard could use a device independent and uniform coordinate system, e.g. using Points or Millimeters.

Or Pixels.

Everything else should be an issue of displaying a scene, but should never be needed to be saved in the document's svg files.

That's my feeling too.

kaamui commented 2 years ago
  • pageDpi does not vary for those files. This is an additional variable, where I do not have and example with another value.

on the file I attached the pageDpi of all pages - except page007 - is 72

https://drive.google.com/file/d/1co516X2zXg1PlmA8WXYPjfWeGJRVMpcL/view

Aaaaaand there is a pageDpi per page but only one per UBDocumentProxy. The last page has a DPI of 96, so it is the value taken for the whole document. Thus the 72/96 ratio is never applied and the need to add the m11 comes for this document to be exported correctly...

kaamui commented 2 years ago

So we finally don't need DPI at all, as you suspected, because having a document with multiple svgs, coming from other documents for example, with multiple DPI values, is supposed to happen, but not handled by the application !

My new proposal :

// If the PDF was scaled when added to the scene (e.g if it was loaded from a document with a different DPI
// than the current one), it should also be scaled here.
qreal pdfScale = pdfItem->sceneTransform().m11();

TransformationDescription pdfTransform(xPdfOffset, yPdfOffset, scaleFactor * pdfScale, 0);
kaamui commented 2 years ago
  • The reason is a fix I made for saving the sizes of other items, because for all QGraphicsItem subclasses the bounding rect has a 0.5 pix margin around the actual area, which is now subtracted. However the UBGraphicsPDFItem brings its own calculation, which does not include that margin.

Can you make a quick PR for this one ?

letsfindaway commented 2 years ago

Can you make a quick PR for this one ?

done. Indeed, when doing the margin correction you have to be careful to do it only when the graphics item actually adds a margin.

letsfindaway commented 2 years ago

Going back to the small offset during PDF export. I carefully inspected the overlay PDF and the final PDF and found out:

letsfindaway commented 2 years ago

So we finally don't need DPI at all, as you suspected, because having a document with multiple svgs, coming from other documents for example, with multiple DPI values, is supposed to happen, but not handled by the application !

My new proposal :

// If the PDF was scaled when added to the scene (e.g if it was loaded from a document with a different DPI
// than the current one), it should also be scaled here.
qreal pdfScale = pdfItem->sceneTransform().m11();

TransformationDescription pdfTransform(xPdfOffset, yPdfOffset, scaleFactor * pdfScale, 0);

Agreed.

letsfindaway commented 2 years ago
  • pageDpi does not vary for those files. This is an additional variable, where I do not have and example with another value.

on the file I attached the pageDpi of all pages - except page007 - is 72

https://drive.google.com/file/d/1co516X2zXg1PlmA8WXYPjfWeGJRVMpcL/view

Note that this cannot be handled correctly by OpenBoard. In the UBSvgSubsetAdaptor the pageDpi attribute is processed as follows:

                QStringRef pageDpi = mXmlReader.attributes().value("pageDpi");

                if (!pageDpi.isNull())
                    proxy->setPageDpi(pageDpi.toInt());

This means even if it is an attribute of the page, it is stored in an object representing the document. Finally the value from the last page will win. I think we should remove the pageDpi from the document proxy and attach it to the scene.

kaamui commented 2 years ago

Note that this cannot be handled correctly by OpenBoard. In the UBSvgSubsetAdaptor the pageDpi attribute is processed as follows:

Yep ! => https://github.com/letsfindaway/OpenBoard/issues/93#issuecomment-1257884108

letsfindaway commented 2 years ago

I think I finally found the reason for the small offsets!!!!!!

It is about rounding issues. The main problem is the following:

When importing the PDF, the nominal size of the scene is set to the size of the PDF page here:

void UBImportPDF::placeImportedItemToScene(UBGraphicsScene* scene, UBGraphicsItem* item)
{
    UBGraphicsPDFItem *pdfItem = (UBGraphicsPDFItem*)item;
    pdfItem->setPos(-pdfItem->boundingRect().width() / 2, -pdfItem->boundingRect().height() / 2);
    scene->setAsBackgroundObject(pdfItem, false, false);
    scene->setNominalSize(pdfItem->boundingRect().width(), pdfItem->boundingRect().height());
}

The pdfItem->boundingRect() is float, but nominalSize is only integer precision. So the nominal size is typically slightly smaller than the actual PDF document page. Later when exporting we take again the nominal size to determine the page size. However the PDF page does not completely fit to that because the fractional digits have been truncated. Now PDF coordinates are originated at the bottom left, not the top left corner. The PDF is rendered relative to this origin. But it now extends a little bit above the top margin of the annotations - it is shifted.

I changed the nominal size in the SVG and added 1 in each direction. After that, the annotations fit much better. I think we still have to remove some other rounding, but the main reason is most probably there.

In fact, we should refactor nominal size to be a QSizeF. What do you think? Too big a change for 1.6? And it still does not help for existing documents :( Or just adding 1 to the nominal size? Where? When?

kaamui commented 2 years ago

No the 1.6.4 is frozen now (except if the test team finds something we did not), and I hope the next version to be 1.7 ^^

Note that with the current 1.6.4, I didn't have any small offset in my macbook pro where I had some in my linux (for the same document of course). So there's maybe something else in the equation.

About nominal size, I agree, but I'm also not comfortable with the format used to store it. Also, I wonder if there would not be a better way to represent or even structure OpenBoard, about pages sizes.. too many unknowns for me at this point.

letsfindaway commented 2 years ago

Ok, fine with that. I will still investigate in better calculation of the page sizes and scaling factors for 1.7.

kaamui commented 2 years ago

Thank you ! Your help was really precious on this issue ! Glad you're on the project !

letsfindaway commented 2 years ago

To save my current work I created the new branch https://github.com/letsfindaway/OpenBoard/tree/improve-pdf-export-scaling. I will test on this further but for me it provides some advantages

Further work will be to make the UBExportFullPDF more configurable, especially regarding scaling, but not only:

kaamui commented 1 year ago

Hi Martin,

I thought we were done with this bug ! A user sent me a document where he encountered an issue at PDF export. I don't have much information, but I have the document used and an example of two exports showing a difference on the second page exported. Apparently happening on 1.6.4 (I asked for confirmation that the user is using 1.6.4 at home too). I can't reproduce it at all, but the issue is quite visible on the first PDF...

24.03.23 Les 9 situations Export 1.pdf

24.03.23 Les 9 Situations Export 2.pdf

24.03.23 Les 9 situations.zip (to rename to .ubz)

I looked at the files and I must say I'm a bit lost with all the parameters we use in the svgs. I wonder how many of these parameters are truly redundant. As you already helped me in dissecting the whole process, I thought it would be a good idea to share it with you again.

Do you think it is related to scaling issues we encountered, or is it something else ? Did you encounter something in your rework that could explain such a case ?

It might be a pdf-merger issue too. I remember that we removed any use of the DPI in the calculation, but the issue is again occurring for somebody that made his document in a Mac and imported it in an Ubuntu, where he encountered the issue. So it might be related, once again, to scaling issues.

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:ub="http://uniboard.mnemis.com/document" xmlns:xhtml="http://www.w3.org/1999/xhtml" version="1.1" ub:version="4.8.0" ub:uuid="61273a81-0492-46fe-b56a-af92b6748526" viewBox="-652 -476 1386 1010" ub:nominal-size="1204x851" ub:dark-background="false" ub:crossed-background="false" ub:ruled-background="false" pageDpi="110">
    <rect fill="white" x="-652" y="-476" width="1386" height="1010"/>
    <foreignObject requiredExtensions="http://ns.adobe.com/pdf/1.3/" xlink:href="objects/{561c06cd-3ca0-4eed-98e1-a8a71c24ecfc}.pdf#page=2" x="0" y="0" width="1286.22" height="909.449" transform="matrix(1, 0, 0, 1, -602.185, -425.787)" ub:z-value="-1000001.000000" ub:background="true" ub:uuid="00000000-0000-0000-0000-000000000000" ub:layer="-2000"/>
</svg>

What disturbs me is that we can end up with such differences :

Either some of these parameters are not used and should be removed, or they are and they should be synced when page is persisted.

letsfindaway commented 1 year ago

Scaling is still a mess in OpenBoard. I again looked into that, and also in my PR https://github.com/OpenBoard-org/OpenBoard/pull/677. From my point of view, a basic problem is, that the scaling stored in the SVG e.g. as pageDPI or as the ratio between nominal size, viewBox size and PDF size are mixed with DPI values and screen sizes on the target platform, and that it is not clear which value is used for what purpose.

The PDF size of 1286x909 is expressed in dots using the DPI value of 110. If you calculate millimeters from that, you end up with DIN A4 (297x210mm), which is the size of the original PDF as it is embedded in the UBZ. So the pageDPI should say: "Hey, all point values in this document are to be interpreted with a DPI of 110 whenever you want to convert them to millimeters".

The nominal size is also derived from the PDF size during import. But here already another scale factor is applied. XPDFRenderer::pageSizeF applies a factor of this->dpiForRendering / 72.0, where dpiForRendering seems to be fixed to 96. So it returns a page size expressed in dots with 96 DPI. And in fact, when I import the PDF into a new document, then I see exactly this factor between nominal size and PDF size. But we have a different factor in the document you provided. So I assume the UBZ we have here was created with some earlier version of OpenBoard, which was even uglier.

Side note: Unfortunately we cannot see the OpenBoard version which created this document. Probably a good idea to add this to the metadata? Here we currently only find the document UB version of 4.8.0, which does not tell us much.

The viewBox is not very important. It is calculated from the bounding box of all items in a scene, and is used again when opening the document to display it in a reasonable way. But I think we can leave that out for now.

A problem in the given document is definitely that PDF size and nominal size do not match, as it would be expected. During export, the nominal size is used to get the PDF page size here: https://github.com/letsfindaway/OpenBoard/blob/b474fa37027ba910bf236e49b7316d525c6d0821/src/adaptors/UBExportFullPDF.cpp#L224 But this does not contain the correct value! So the PDF is created with the wrong size.

Even if page 1 in the export looks quite ok, it has a different size from the original (278x197 instead of 297x210). I cannot completely follow why it is exactly that size, but it has to do with the fact that a wrong nominal size is used during export.

Please reconsider my PR linked above. Here I only used the PDF size from the PDF document, not the nominal size. When there is a background PDF, then I don't use any mScaleFactor, which involves the current display resolution when it is calculated here: https://github.com/letsfindaway/OpenBoard/blob/b474fa37027ba910bf236e49b7316d525c6d0821/src/adaptors/UBExportFullPDF.cpp#L67-L68 And because of that, it does not have to make corrections like here: https://github.com/letsfindaway/OpenBoard/blob/b474fa37027ba910bf236e49b7316d525c6d0821/src/adaptors/UBExportFullPDF.cpp#L256-L258

letsfindaway commented 1 year ago

Future recommendations

I would like some recommendations about how to handle these parameters in the future. Note, this is not covered by my PR.

Resolution independent documents

First of all and most important the UBZ document shall be independent of the resolution of the monitor which was attached to the computer when creating the document. This means:

Separate PDF paper size from PDF scene representation size

Separate the size of the PDF document from the size it has on the screen. This means:

Keep nominalSize

This is the size of the grey frame on the board. Nothing else, especially not a PDF background size, even if you might initialize it that way.

Drop viewPort

This is calculated from the scene content when saving a document. It can as easily be calculated the same way when opening it again. No need to save it in the document.

Use floating point everywhere

Especially make systemScaleFactor more accurate. This factor should be so that a 72 points on the scene are 25.4 mm on the control display when the zoom factor is 100%. Currently it tries to do something like that, but very inaccurate.

Also at any place dealing with scene coordinates, DPI values, sizes etc. use floating point.

Nominal size, background grid size and display size

Currently the scene nominal size is scaled to the display using the systemScaleFactor in a way that the nominal area occupies about the complete screen, no matter what screen resolution and screen size we have. Then a background grid size is computed in a way, that it is about 1 cm on the screen. See UBBoardController::initBackgroundGridSize.

This leads to the fact that documents created on small screens have fewer lines in the nominal size than those created on large screens. Of course this is a tradeoff between different arguments, but I would propose:

Of course this would change the current behavior and some users would be surprised about that. But only then we can have reproducible behavior and portable documents.

kaamui commented 1 year ago

Side note: Unfortunately we cannot see the OpenBoard version which created this document. Probably a good idea to add this to the metadata? Here we currently only find the document UB version of 4.8.0, which does not tell us much.

Yep, totally agree, but the version persisted will be the last one. We would like to know if a document had ever been persisted with other versions, don't we ?

letsfindaway commented 1 year ago

Side note: Unfortunately we cannot see the OpenBoard version which created this document. Probably a good idea to add this to the metadata? Here we currently only find the document UB version of 4.8.0, which does not tell us much.

Yep, totally agree, but the version persisted will be the last one. We would like to know if a document had ever been persisted with other versions, don't we ?

Just as we have a "created" and "modified" date, we could have a "created-version" and "modified-version" attribute.

kaamui commented 1 year ago

Side note: Unfortunately we cannot see the OpenBoard version which created this document. Probably a good idea to add this to the metadata? Here we currently only find the document UB version of 4.8.0, which does not tell us much.

Yep, totally agree, but the version persisted will be the last one. We would like to know if a document had ever been persisted with other versions, don't we ?

Just as we have a "created" and "modified" date, we could have a "created-version" and "modified-version" attribute.

Yes, but my point was "what if you create a document with 1.6.4, then use it one day with 1.5.4 or 1.6.3 or whatever, and then modify it again on another machine where 1.6.4 is installed"

kaamui commented 1 year ago

Side note: Unfortunately we cannot see the OpenBoard version which created this document. Probably a good idea to add this to the metadata? Here we currently only find the document UB version of 4.8.0, which does not tell us much.

Yep, totally agree, but the version persisted will be the last one. We would like to know if a document had ever been persisted with other versions, don't we ?

Just as we have a "created" and "modified" date, we could have a "created-version" and "modified-version" attribute.

Are you suggesting a document created with 1.6.4 can no longer be altered with a 1.6.3 in a way that would create an issue at PDF Export ?

letsfindaway commented 1 year ago

I just checked the problematic document with my PR you merged. It is not perfect. So the issue remains open. grafik

letsfindaway commented 1 year ago

Are you suggesting a document created with 1.6.4 can no longer be altered with a 1.6.3 in a way that would create an issue at PDF Export ?

No. Compatibility must be expressed by the existing ub:version. As long as this stays at 4.8.0, the documents must be compatible. The version information I suggested should just help in debugging to see, what happened to the document.

But you're right: then we would have to record the whole history, including platform information and screen resolutions. That's not feasible.

kaamui commented 1 year ago

I just checked the problematic document with my PR you merged. It is not perfect. So the issue remains open.

Wow ! Didn't notice ! For this particular issue, seems to me that is is not directly related to the "pdf export" scaling issue though.

Here the issue is the fact that the imported PDF is not perfectly bounded in the grey rectangle. This means that it is possible to import a PDF, export the created OpenBoard document to another machine and end up with a bounding box not corresponding to the initial one. I don't think it is supposed to happen.

image

The initial purpose of this grey delimitation is a mystery for me at this point. I initially thought, some years ago, that it was useful to indicate what is visible or not in the extended screen. I also though that it is suppose to indicate the limitations of what will be exported to PDF. But in the mean time, you just need to have annotations in the overlay in a larger area for this limit to not be considered....

Even internally, the oldest users are not sure of what was the initial purpose. Another possibility is that I'm right about one of the things I suppose but that a bug was introduced, that I didn't notice, and now it no longer works as expected...

It's like the "page size" feature. For me it's not clear at all of what it is supposed to do... I can't imagine for the users.

kaamui commented 1 year ago

I just checked the problematic document with my PR you merged. It is not perfect. So the issue remains open. grafik

Just tested and for me the part "Ich g" is just not visible at export, as I expected it

kaamui commented 1 year ago

HI @letsfindaway,

I forgot to share with you the last information I had about this case :

The user will look after this particular issue, and will try to confirm me that he just have to reexport it in order to have it exported correctly. Anyway, if redoing the export solves the issue on the user's side, it is less critical, and potentially an issue we don't have the hand on...

letsfindaway commented 1 year ago

HI @letsfindaway,

I forgot to share with you the last information I had about this case :

* The user is pretty sure he never used any other version than 1.6.4

Ok, so the idea of putting the OpenBoard version into the metadata would not help here. I generally think now that it is not very helpful.

* When exporting was not good, just redoing it (he could not say if it was right after or a few hours), apparently on the same machine (but didn't seem too confident about this) and the export was OK.

Huch, that's strange. It would be really interesting if something was different between these two exports, e.g. the monitor configuration?

The user will look after this particular issue, and will try to confirm me that he just have to reexport it in order to have it exported correctly. Anyway, if redoing the export solves the issue on the user's side, it is less critical, and potentially an issue we don't have the hand on...

Yes, but anyway we have to consider PR 750. For me the situation is as follows:

For me I found that without that some of the problematic documents, especially those with pageDpi = 110 had big offsets when exporting. No wonder when my screen has 96 dpi.

letsfindaway commented 1 year ago

Closing. Currently there are no unresolved PDF export scaling issues.