Correctly support rotated texts#444
Conversation
A Vector captures a 2D direction in page space (length, normalized, dot product, perpendicular normal). TransformationMatrix::baselineVector() names the baseline (text advance) direction in page space: the matrix's first column (scaleX, shearX). Nothing consumes them yet; the glyph geometry and line-grouping that follow are built on these.
Font::getWidthForChar(s) drop their TransformationMatrix parameter and return the unscaled text-space width (w0·Tfs + Tc) rather than pre-multiplying by scaleX. getAdvanceWidth() takes a page-space direction Vector (default page X) and returns the signed projection of the reconstructed advance onto it -- a dot product against the baseline vector -- so the advance can later be measured along a rotated baseline. The default direction reproduces the previous advance · scaleX exactly, so the extracted text is unchanged. The perpendicular-to-baseline glyph height that rotated text needs lands with the baseline-aware strategy.
The word-break decision moves out of ContentStream into the strategy, because a gap between two runs is meaningful only relative to how that strategy laid them out. LineGroupingStrategy gains requiresSpaceBetween(); ContentStream keeps only the empty-run and already-spaced guards and asks the strategy. A MatrixOffsetSpacing trait holds the axis-aligned heuristic the two upright strategies share (TextOverlapStrategy, StrictLineGrouping): a space belongs when the gap between X offsets, less the previous run's reconstructed advance width, reaches WORD_BREAK_THRESHOLD of the em -- the same comparison ContentStream made inline before, so the extracted text is unchanged.
BaselineClusterStrategy is orientation-agnostic and two-pass. Pass 1 clusters runs into lines by their actual baseline -- baselines pointing nearly the same way whose glyph extents overlap along the baseline normal -- and orders each line along its baseline vector, so multi-run rotated text (a watermark, a vertical label, a diagonal overlay) is reassembled and reads in order at any angle rather than fragmenting to one line per run. Pass 2 segments the lines into orientation-bucketed blocks, orders the lines within a block along the block's own normal, and orders the blocks by their highest point on the page, so a rotated overlay stays contiguous instead of interleaving with the body it crosses. Lines and blocks are modelled as Line and Block value objects built on Vector; requiresSpaceBetween() measures the gap and the advance along the baseline, reducing to the axis-aligned comparison for horizontal text. getHeight() now measures the glyph extent perpendicular to the baseline (hypot(shearY, scaleY)) so rotated text keeps a non-zero height; this is what lets the strategy cluster rotated runs, and it changes grouping only where text is rotated. For horizontal text the baseline normal is the page Y axis and there is a single orientation bucket, so this is byte-identical to TextOverlapStrategy on every non-rotated page; Page::getText switches to it as the default. The only existing sample it changes is text-objects-across-multiple-streams (pages 5 and 7), whose rotated channel labels now read in baseline order with the repeated device labels separated by the reconstructed advance; its fixture is updated to match. TextOverlapStrategy remains as a non-default, axis-aligned alternative.
Regression fixtures exercising BaselineClusterStrategy, where the previous default fragmented rotated text to one line per run: - text-rotated-angles: one word per page rotated around the full circle (every 30 degrees), drawn glyph-by-glyph along its baseline; every angle is reassembled onto a single line. - text-rotated-watermark: a single 90-degree word drawn glyph-by-glyph, reassembled onto one line. - text-rotated-label: a LibreOffice document that emits a rotated word as per-glyph runs in the content stream; reassembled into one line. - text-rotated-overlay: a justified body paragraph crossed by two diagonal overlay paragraphs (35 and 265 degrees); each overlay stays contiguous and reads in order rather than interleaving with the body.
splitbrain
force-pushed
the
rotation-by-cluster-split
branch
from
4ffa5b8 to
426c28c
Compare
The fallback font size for content streams that show text without ever setting one lived as a bare literal at five call sites, and disagreed: getHeight() assumed 12 while the rest assumed 10. Funnel them all through TextState::getFontSize(), backed by one private DEFAULT_FONT_SIZE constant.
splitbrain
force-pushed
the
rotation-by-cluster-split
branch
from
426c28c to
6a975fa
Compare
|
Thanks for the contribution! As discussed in #430, PRs of this size are not reviewable. The fact that you were able to generate a 340 line WALKTHROUGH.md file on top of the 1000 line changes should have been an indication of that. I've documented the PR size limits in #445, and the pipeline will also fail from now on when there are more than 200 lines changed in the src folder.
I don't think it's fair to ask from me. I was generous by splitting up your previous PR #430, but doing this repeatedly puts unsustainable work on my plate. Splitting this into reviewable parts and then reviewing each would take at least 8 hours of my time. Time that I volunteer to the community by maintaining this package for free. Your options:
Thanks for your understanding. |
|
This PR introduces a complex new feature. None of the commits make any sense as their own pull request since they are building towards that common goal. I don't think the feature can be implemented in "less than 200 lines". The commits already split the PR into "logical changes". I spent about a week working on the previous PRs, this one and a branch implementing #34. I tried to make review as simple as possible offering to add inline or review comments and by providing the walkthrough. Ultimately I have a version of the library that works for me and I am thankful for the work you have done, which made it possible for me to build upon. But I can not spend more time on this. All my changes are available in https://github.com/cosmocode/prinsfrank-pdfparser/tree/dev - feel free to pick anything you want or just be inspired by it for your own implementation. PS: I am not mad at you. It's your project and you have to maintain it in a way that works for you. It was fun digging into the internals of the PDF format and I learned a lot. Thank you. |
|
I think it would be very possible to split up the changes in this PR in several smaller chunks:
Each of these changes is independtenly reviewable and testable. The WALKTHROUGH.md you attached has similar sections. I understand you've invested significant time into this work, and I appreciate the effort! However, reviewing and maintaining code is different from writing it. With PRs with more than 200 line changes, review quality drops. Smaller PRs ensure that I can properly understand and maintain every change that goes into the library, which is essential for maintaining trust with its users. I respect your decision to move on. Thank you for understanding my position, and thanks for sharing your learnings with the community. |
3 participants
This PR introduces a new default LineGrouping strategy named BaselineClusterStrategy. Instead of grouping text elements by their position on the page only, it groups them by their rotation as well. This will, for example, prevent watermarks to interweave with "normal" text. Or rotated glyph based text to be split into individual lines.
For horizontal text, the results are exactly the same as before.
Note that this changes the LineGrouping interface by moving the space decision into it - it now depends on the used strategy. A few data object classes were introduced to make the code cleaner.
I believe this to be the better overall strategy, but the old strategies are still available. If it's worth keeping them is up for discussion.
This was previously submitted as #431 but is now rebased on current main. I assume you will want to dissect it into multiple PRs again. I will keep rebasing this branch whenever parts of it land in main.
I include an (AI generated) WALKTHROUGH.md here (not committed). It might help reviewing the code. Feel free to ignore if that's not helpful to you.