From 935992e72c70e390edc8ab8215489f3623e9540a Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 30 Aug 2026 11:05:38 +0200 Subject: [PATCH 1/3] feat(svm): clip the drawing to what the metafile clips it to MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `CLIPREGION` and the two actions that intersect a region into it were skipped like any unimplemented action, so anything a metafile drew outside its own clip was shown in full. Every metafile a document carries sets one: 1124 of the 1125 harvested from the fixtures intersect a rectangle, 50 intersect a region. An svg `clip-path` names one shape and `ISECT…` intersects, so two clips become two groups, one inside the other. The state's clip is therefore a stack of shapes: `ensure_clip` keeps the groups the next drawing action still wants, closes the ones it does not and opens what is missing, which also makes a `POP` that restores the clip nothing special. A region streams as a band list - a union of rectangles - and from version 2 also as the poly-polygon those were rasterised from, which is the better outline where it is there. Both go into one ``: disjoint bands make union and even-odd the same thing, and a poly-polygon wants even-odd anyway. Two details worth naming: vcl does not re-scale a clip when the map mode changes (`SetMapMode`: "clip regions are not re-scaled"), so the shape is transformed once, where the action sets it; and a file that sets its drawing area and then intersects the region of the same rectangle gets one group, not two. `MOVECLIPREGION` stays unimplemented - it occurs nowhere in the corpus and would have to move path data that is already written out. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01CmCr22NW6wPQKiQidk96bq --- CHANGELOG.md | 4 + src/odr/internal/svm/PLAN.md | 23 +++++- src/odr/internal/svm/svm_format.cpp | 70 ++++++++++++++++++ src/odr/internal/svm/svm_format.hpp | 17 +++++ src/odr/internal/svm/svm_to_svg.cpp | 111 ++++++++++++++++++++++++++++ test/data.cmake | 4 +- test/src/internal/svm/svm_test.cpp | 102 +++++++++++++++++++++++++ 7 files changed, 327 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 12a5cf3f2..be579919b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,10 @@ The release run heads these entries with the version and opens a fresh - A spreadsheet decodes in less memory: 626 MB peak instead of 914 MB on a 297 MB `content.xml`. Rendered output is unchanged. +- A StarView metafile clips what it says it clips: `CLIPREGION` and the two + that intersect one into it. A drawing that ran outside the clip used to be + shown in full. + - A StarView metafile draws its bitmaps (#194): `BMP`, `BMPEX` and their scaling and part variants, transparency mask included. Such a chart used to render as an empty frame. diff --git a/src/odr/internal/svm/PLAN.md b/src/odr/internal/svm/PLAN.md index a93372ec5..5d9a8694a 100644 --- a/src/odr/internal/svm/PLAN.md +++ b/src/odr/internal/svm/PLAN.md @@ -27,8 +27,8 @@ Each stage is one pull request, stacked on the one before it. nowhere in the corpus - and so is decoding a non-`UCS2` string instead of passing its bytes through: until then a latin-1 label emits invalid utf-8, which costs the image exactly as an unescaped `&` did. -4. **Clipping.** `CLIPREGION`, `ISECTRECTCLIPREGION`, - `ISECTREGIONCLIPREGION`, `MOVECLIPREGION`. +4. **Clipping** - done but for `MOVECLIPREGION`, which occurs nowhere and + would have to move path data that is already written out. 5. **Bitmaps** (#194) - done for the `BMP` and `BMPEX` families; `MASK`, which stencils one colour through a bitmap, and `ZCOMPRESS`, which needs inflating first, are still open. See the shortcut below. @@ -79,6 +79,25 @@ It is rare — 4 `MAPMODE` actions in 1125 files, one of them relative — and getting it right means following `vcl/source/outdev/map.cxx` rather than guessing, so it is its own stage. +## Clipping is nested groups + +An svg `clip-path` names one shape, and `ISECT…` intersects. Two clips +therefore become two groups, one inside the other, and the state's clip is a +*stack* of shapes rather than one: `ensure_clip` keeps the groups that the +next drawing action still wants, closes the ones it does not, and opens what +is missing. A `POP` that restores the clip is then nothing special - the next +action closes what it has to. + +A region streams as a band list, which is a union of rectangles, and from +version 2 also as the poly-polygon those were rasterised from. The polygons +are the better outline where they are there. Both go into one `` - +disjoint bands make union and even-odd the same thing, and a poly-polygon +wants even-odd anyway, so `clip-rule="evenodd"` covers both. + +vcl does *not* re-scale a clip when the map mode changes (`SetMapMode`: +"clip regions are not re-scaled"), so the shape is transformed once, when the +action sets it. + ## Text is where the corpus lives Two thirds of every action in the corpus is text, and three things about it diff --git a/src/odr/internal/svm/svm_format.cpp b/src/odr/internal/svm/svm_format.cpp index f0e2fe907..bf996aa50 100644 --- a/src/odr/internal/svm/svm_format.cpp +++ b/src/odr/internal/svm/svm_format.cpp @@ -10,6 +10,7 @@ #include #include #include +#include namespace odr::internal { @@ -23,6 +24,14 @@ std::string read_bytes(std::istream &in, const std::uint64_t size) { } } +/// `RegionType`, what shape the region was streamed as. +constexpr std::uint16_t region_null = 0; +constexpr std::uint16_t region_empty = 1; +/// `StreamEntryType`, what the band list holds. +constexpr std::uint16_t band_header = 0; +constexpr std::uint16_t band_separation = 1; +constexpr std::uint16_t band_end = 2; + /// A `.bmp` starts with `"BM"`; `"BA"`, an os/2 bitmap array, does not. constexpr std::uint16_t bmp_magic = 0x4d42; constexpr std::uint32_t bmp_file_header_size = 14; @@ -777,6 +786,67 @@ svm::BitmapAction svm::read_bitmap_action(std::istream &in, return result; } +svm::Region svm::read_region(std::istream &in) { + Region result; + + const VersionLength vl = read_version_length(in); + std::uint16_t content_version{}; + std::uint16_t type{}; + read_primitive(in, content_version); + read_primitive(in, type); + + if (type == region_null) { + result.null = true; + return result; + } + if (type == region_empty) { + return result; + } + + // the bands: a run of horizontal strips, each with the spans that are in + // the region, and together the union that covers it + std::int32_t top{}; + std::int32_t bottom{}; + while (true) { + std::uint16_t entry{}; + read_primitive(in, entry); + if (entry == band_end) { + break; + } + + std::int32_t first{}; + std::int32_t second{}; + read_primitive(in, first); + read_primitive(in, second); + + if (entry == band_header) { + top = first; + bottom = second; + } else if (entry == band_separation) { + result.rectangles.push_back({first, top, second, bottom}); + } else { + throw MalformedSvmFile(); + } + } + + if (vl.version >= 2) { + bool has_polygons{}; + read_primitive(in, has_polygons); + if (has_polygons) { + result.polygons = read_poly_polygon(in); + } + } + + return result; +} + +std::pair svm::read_clip_region_action(std::istream &in) { + Region region = read_region(in); + bool clip{}; + read_primitive(in, clip); + return {std::move(region), clip}; +} + std::uint16_t svm::read_text_align_action(std::istream &in) { std::uint16_t result; read_primitive(in, result); diff --git a/src/odr/internal/svm/svm_format.hpp b/src/odr/internal/svm/svm_format.hpp index f11f5cbbb..242699a6e 100644 --- a/src/odr/internal/svm/svm_format.hpp +++ b/src/odr/internal/svm/svm_format.hpp @@ -6,6 +6,7 @@ #include #include #include +#include #include // https://github.com/LibreOffice/core/blob/master/include/vcl/metaact.hxx @@ -267,6 +268,17 @@ struct TextRectangleAction final { std::uint16_t style{}; }; +/// A clip region. Its bands cover it as a union of rectangles; where the +/// file also kept the shape those were rasterised from, @ref polygons is it +/// and is the better outline. +struct Region final { + /// `REGION_NULL`: no clipping at all, as against a region that covers + /// nothing and clips everything away. + bool null{}; + std::vector rectangles; + std::vector> polygons; +}; + /// A dib as something a browser reads. A metafile stores a dib *with* its /// `BITMAPFILEHEADER`, so its bytes already are a `.bmp` file; they are only /// unpacked where a png would be smaller, which for a chart is by fifty. @@ -354,6 +366,11 @@ TextLineAction read_text_line_action(std::istream &in, const VersionLength &vl); std::uint16_t read_push_action(std::istream &in, const VersionLength &vl); /// The `TextAlign` of a `TEXTALIGN`. std::uint16_t read_text_align_action(std::istream &in); +/// A region, as `ReadRegion` reads one: a band list, and from version 2 the +/// poly-polygon it came from. +Region read_region(std::istream &in); +/// A `CLIPREGION`: the region, and whether it clips at all. +std::pair read_clip_region_action(std::istream &in); /// A dib with its file header, as `ReadDIB(…, bFileHeader=true)` reads one. /// @p limit is what the enclosing action declared, so a length field cannot /// ask for more than the file holds. diff --git a/src/odr/internal/svm/svm_to_svg.cpp b/src/odr/internal/svm/svm_to_svg.cpp index 47a788fe3..696cadba5 100644 --- a/src/odr/internal/svm/svm_to_svg.cpp +++ b/src/odr/internal/svm/svm_to_svg.cpp @@ -43,6 +43,9 @@ struct GraphicsState final { std::uint32_t over_line_rgb{}; /// vcl's default, and what a file that never says otherwise draws with. std::uint16_t text_align{ALIGN_TOP}; + /// What the drawing is clipped to, as path data, one entry per region the + /// file intersected in. Nesting a group per entry is what intersects them. + std::vector clip; }; struct SavedState final { @@ -61,6 +64,10 @@ struct Context final { /// Names the masks and clip paths apart. std::uint32_t element_count{}; bool inverter_written{}; + + /// The clip the open groups already apply - a prefix of the state's clip + /// once @ref ensure_clip has run. + std::vector written_clip; }; double scale(const IntPair fraction) { @@ -302,6 +309,76 @@ get_path_data_string(const std::span> polygons, return result; } +/// The rectangle as the four points that outline it. +std::vector get_rectangle_polygon(const Rectangle &rect) { + return {{rect.left, rect.top}, + {rect.right, rect.top}, + {rect.right, rect.bottom}, + {rect.left, rect.bottom}}; +} + +/// The region's outline: the shape it came from where the file kept one, and +/// the union its bands cover where it did not. +std::string get_region_path_data(const Region ®ion, const Context &context) { + if (!region.polygons.empty()) { + return get_path_data_string(region.polygons, true, context); + } + + std::vector> polygons; + polygons.reserve(region.rectangles.size()); + for (const Rectangle &rect : region.rectangles) { + polygons.push_back(get_rectangle_polygon(rect)); + } + return get_path_data_string(polygons, true, context); +} + +/// Intersecting a clip with the shape it already has is a group that clips +/// nothing, and a file does that: it sets the drawing area, then intersects +/// the region of the same rectangle. +void intersect_clip(std::string path_data, GraphicsState &state) { + if (!state.clip.empty() && state.clip.back() == path_data) { + return; + } + state.clip.push_back(std::move(path_data)); +} + +/// Opens the groups the state's clip asks for and closes the ones it no +/// longer does, keeping what the two have in common. Every drawing action +/// goes through here first, so what it writes lands inside them. +void ensure_clip(Context &context) { + svg::SvgWriter &out = *context.out; + const std::vector &clip = context.state.clip; + + std::size_t common = 0; + while (common < context.written_clip.size() && common < clip.size() && + context.written_clip[common] == clip[common]) { + ++common; + } + + while (context.written_clip.size() > common) { + out.write_element_end(); + context.written_clip.pop_back(); + } + + for (std::size_t i = common; i < clip.size(); ++i) { + const std::string id = + "odr-clip-" + std::to_string(++context.element_count); + + out.write_element_begin("clipPath"); + out.write_attribute("id", id); + out.write_element_begin("path"); + out.write_attribute("d", clip[i]); + // sub-polygons of one region are its holes, as they are in a shape + out.write_attribute("clip-rule", "evenodd"); + out.write_element_end(); + out.write_element_end(); + + out.write_element_begin("g"); + out.write_attribute("clip-path", "url(#" + id + ")"); + context.written_clip.push_back(clip[i]); + } +} + /// One path for all of them: the fill rule only cuts holes within a path. void write_path(const std::span> polygons, const bool fill, const LineInfo *line_info, @@ -560,6 +637,9 @@ void pop_state(Context &context) { state.text_fill_rgb = saved.state.text_fill_rgb; state.text_fill_rgb_set = saved.state.text_fill_rgb_set; } + if (saved.flags & PUSH_CLIPREGION) { + state.clip = saved.state.clip; + } if (saved.flags & PUSH_TEXTALIGN) { state.text_align = saved.state.text_align; } @@ -606,19 +686,23 @@ void translate_action(const ActionHeader &action_header, std::istream &in, break; case META_RECT_ACTION: { const Rectangle action = read_rectangle(in); + ensure_clip(context); write_rectangle(action, context); } break; case META_POLYLINE_ACTION: { const auto [points, line_info] = read_poly_line_action(in, action_header.vl); + ensure_clip(context); write_path({&points, 1}, false, &line_info, context); } break; case META_POLYGON_ACTION: { const auto [points] = read_polygon_action(in, action_header.vl); + ensure_clip(context); write_path({&points, 1}, true, nullptr, context); } break; case META_POLYPOLYGON_ACTION: { const auto [polygons] = read_poly_polygon_action(in, action_header.vl); + ensure_clip(context); write_path(polygons, true, nullptr, context); } break; case META_BMP_ACTION: @@ -629,24 +713,47 @@ void translate_action(const ActionHeader &action_header, std::istream &in, case META_BMPEXSCALEPART_ACTION: { const BitmapAction action = read_bitmap_action(in, action_header.type, action_header.vl); + ensure_clip(context); write_bitmap(action, context); } break; + case META_CLIPREGION_ACTION: { + auto [region, clip] = read_clip_region_action(in); + state.clip.clear(); + if (clip && !region.null) { + intersect_clip(get_region_path_data(region, context), state); + } + } break; + case META_ISECTRECTCLIPREGION_ACTION: { + const Rectangle action = read_rectangle(in); + intersect_clip( + get_path_data_string({get_rectangle_polygon(action)}, true, context), + state); + } break; + case META_ISECTREGIONCLIPREGION_ACTION: { + const Region region = read_region(in); + if (!region.null) { + intersect_clip(get_region_path_data(region, context), state); + } + } break; case META_TEXTALIGN_ACTION: state.text_align = read_text_align_action(in); break; case META_TEXT_ACTION: { const TextAction action = read_text_action(in, action_header.vl, state.encoding); + ensure_clip(context); write_text(action.point, action.text, {}, 0, context); } break; case META_TEXTARRAY_ACTION: { const TextArrayAction action = read_text_array_action(in, action_header.vl, state.encoding); + ensure_clip(context); write_text(action.point, action.text, action.dx_array, 0, context); } break; case META_STRETCHTEXT_ACTION: { const StretchTextAction action = read_stretch_text_action(in, action_header.vl, state.encoding); + ensure_clip(context); write_text(action.point, action.text, {}, action.width, context); } break; default: @@ -706,6 +813,10 @@ void svm::translate_to_svg(const SvmFile &file, std::ostream &out, ODR_WARNING(logger, context.stack.size() << " pushes were never popped"); } + // whatever the last clip left open + context.state.clip.clear(); + ensure_clip(context); + writer.write_element_end(); } diff --git a/test/data.cmake b/test/data.cmake index 4bc978b8e..d02c71ab5 100644 --- a/test/data.cmake +++ b/test/data.cmake @@ -17,9 +17,9 @@ odr_test_data( odr_test_data( PATH "reference-output/odr-public" URL "https://github.com/opendocument-app/OpenDocument.test.output.git" - REVISION "29c6270283527ff91c3ca125f67bd4260aa2acae") + REVISION "05441f24b4fcc9398b5115718a7d812ab4d63cea") odr_test_data( PATH "reference-output/odr-private" URL "https://github.com/opendocument-app/OpenDocument.test-private.output.git" - REVISION "898f1d823afdca5008ebb4a6579e0d4d9e412554") + REVISION "3ef61b792470da12cfd76e74afa1e0fad0812617") diff --git a/test/src/internal/svm/svm_test.cpp b/test/src/internal/svm/svm_test.cpp index da0bd208f..762078986 100644 --- a/test/src/internal/svm/svm_test.cpp +++ b/test/src/internal/svm/svm_test.cpp @@ -110,6 +110,23 @@ class SvmBuilder final { .end(); } + /// A region of one band, the shape `ReadRegion` reads a rectangle as. + SvmBuilder ®ion(const std::int32_t left, const std::int32_t top, + const std::int32_t right, const std::int32_t bottom) { + return begin(2) + .u16(1) // content version + .u16(2) // type: rectangle + .u16(0) // band header + .i32(top) + .i32(bottom) + .u16(1) // separation + .i32(left) + .i32(right) + .u16(2) // end + .u8(0) // no poly-polygon + .end(); + } + /// A 24-bit uncompressed dib with the `BITMAPFILEHEADER` a metafile stores /// it behind. @p pixels is @p width * @p height bgr triples, top row first; /// the rows go out bottom-up and padded, as a dib holds them. @@ -619,3 +636,88 @@ TEST(SvmToSvg, a_bitmap_mask_is_inverted) { EXPECT_NE(std::string::npos, svg.find("filter=\"url(#odr-invert)\"")); EXPECT_NE(std::string::npos, svg.find("mask=\"url(#odr-mask-1)\"")); } + +/// What a clip covers is a group around everything drawn after it. +TEST(SvmToSvg, a_clip_region_wraps_what_follows) { + const std::string svg = + translate(SvmBuilder() + .action(svm::META_ISECTRECTCLIPREGION_ACTION) + .rectangle(1, 2, 11, 22) + .end() + .action(svm::META_RECT_ACTION) + .rectangle(0, 0, 100, 100) + .end() + .file()); + + EXPECT_NE(std::string::npos, + svg.find("" + "")); +} + +/// One region intersected into another is one group inside the other, which +/// is how svg intersects two clips. +TEST(SvmToSvg, clips_intersect_by_nesting) { + const std::string svg = + translate(SvmBuilder() + .action(svm::META_ISECTRECTCLIPREGION_ACTION) + .rectangle(0, 0, 10, 10) + .end() + .action(svm::META_ISECTREGIONCLIPREGION_ACTION) + .region(2, 2, 8, 8) + .end() + .action(svm::META_RECT_ACTION) + .rectangle(0, 0, 100, 100) + .end() + .file()); + + EXPECT_NE(std::string::npos, svg.find("" + "")); + EXPECT_NE(std::string::npos, svg.find("M 2,2 L 8,2 8,8 2,8 Z")); + EXPECT_NE(std::string::npos, svg.find("")); +} + +/// A pop that restores the clip closes the group the push was drawn in. +TEST(SvmToSvg, a_pop_restores_the_clip) { + const std::string svg = + translate(SvmBuilder() + .action(svm::META_PUSH_ACTION) + .u16(svm::PUSH_CLIPREGION) + .end() + .action(svm::META_ISECTRECTCLIPREGION_ACTION) + .rectangle(1, 1, 2, 2) + .end() + .action(svm::META_RECT_ACTION) + .rectangle(0, 0, 100, 100) + .end() + .action(svm::META_POP_ACTION) + .end() + .action(svm::META_RECT_ACTION) + .rectangle(0, 0, 100, 100) + .end() + .file()); + + // the second rectangle is outside the group the first one is in + EXPECT_NE(std::string::npos, svg.find("/> Date: Sun, 30 Aug 2026 12:00:28 +0200 Subject: [PATCH 2/3] docs(svm): trim the comments to the house standard The clip comments that told the story rather than the point, and `get_rectangle_polygon`, which said what its own name does. The `clip-rule` comment now says why `evenodd` is right for both shapes a region arrives as: holes where the file kept the poly-polygon, and a union where it only kept bands, which never overlap. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XQpLmJpJ87qbKoG8B7kbLY --- src/odr/internal/svm/svm_format.cpp | 3 +-- src/odr/internal/svm/svm_format.hpp | 5 ++--- src/odr/internal/svm/svm_to_svg.cpp | 19 ++++++++----------- 3 files changed, 11 insertions(+), 16 deletions(-) diff --git a/src/odr/internal/svm/svm_format.cpp b/src/odr/internal/svm/svm_format.cpp index bf996aa50..b9ba2618d 100644 --- a/src/odr/internal/svm/svm_format.cpp +++ b/src/odr/internal/svm/svm_format.cpp @@ -803,8 +803,7 @@ svm::Region svm::read_region(std::istream &in) { return result; } - // the bands: a run of horizontal strips, each with the spans that are in - // the region, and together the union that covers it + // horizontal strips, each with the spans inside the region std::int32_t top{}; std::int32_t bottom{}; while (true) { diff --git a/src/odr/internal/svm/svm_format.hpp b/src/odr/internal/svm/svm_format.hpp index 242699a6e..09df69446 100644 --- a/src/odr/internal/svm/svm_format.hpp +++ b/src/odr/internal/svm/svm_format.hpp @@ -268,9 +268,8 @@ struct TextRectangleAction final { std::uint16_t style{}; }; -/// A clip region. Its bands cover it as a union of rectangles; where the -/// file also kept the shape those were rasterised from, @ref polygons is it -/// and is the better outline. +/// A clip region: bands covering it as a union of rectangles, and from +/// version 2 the poly-polygon those were rasterised from. struct Region final { /// `REGION_NULL`: no clipping at all, as against a region that covers /// nothing and clips everything away. diff --git a/src/odr/internal/svm/svm_to_svg.cpp b/src/odr/internal/svm/svm_to_svg.cpp index 696cadba5..f7e410c1d 100644 --- a/src/odr/internal/svm/svm_to_svg.cpp +++ b/src/odr/internal/svm/svm_to_svg.cpp @@ -309,7 +309,6 @@ get_path_data_string(const std::span> polygons, return result; } -/// The rectangle as the four points that outline it. std::vector get_rectangle_polygon(const Rectangle &rect) { return {{rect.left, rect.top}, {rect.right, rect.top}, @@ -332,9 +331,8 @@ std::string get_region_path_data(const Region ®ion, const Context &context) { return get_path_data_string(polygons, true, context); } -/// Intersecting a clip with the shape it already has is a group that clips -/// nothing, and a file does that: it sets the drawing area, then intersects -/// the region of the same rectangle. +/// A file that sets the drawing area and then intersects the same rectangle +/// asks for a group that clips nothing; that one is dropped. void intersect_clip(std::string path_data, GraphicsState &state) { if (!state.clip.empty() && state.clip.back() == path_data) { return; @@ -342,9 +340,8 @@ void intersect_clip(std::string path_data, GraphicsState &state) { state.clip.push_back(std::move(path_data)); } -/// Opens the groups the state's clip asks for and closes the ones it no -/// longer does, keeping what the two have in common. Every drawing action -/// goes through here first, so what it writes lands inside them. +/// Reconciles the open groups with the state's clip, keeping the prefix they +/// share. Every drawing action goes through here first. void ensure_clip(Context &context) { svg::SvgWriter &out = *context.out; const std::vector &clip = context.state.clip; @@ -368,7 +365,8 @@ void ensure_clip(Context &context) { out.write_attribute("id", id); out.write_element_begin("path"); out.write_attribute("d", clip[i]); - // sub-polygons of one region are its holes, as they are in a shape + // holes, where the region kept the shape it was rasterised from; its + // bands never overlap, so they union under the same rule out.write_attribute("clip-rule", "evenodd"); out.write_element_end(); out.write_element_end(); @@ -725,9 +723,8 @@ void translate_action(const ActionHeader &action_header, std::istream &in, } break; case META_ISECTRECTCLIPREGION_ACTION: { const Rectangle action = read_rectangle(in); - intersect_clip( - get_path_data_string({get_rectangle_polygon(action)}, true, context), - state); + const std::vector polygon = get_rectangle_polygon(action); + intersect_clip(get_path_data_string({&polygon, 1}, true, context), state); } break; case META_ISECTREGIONCLIPREGION_ACTION: { const Region region = read_region(in); From 6b5af685e2581852f6e6f85213432eddead1345d Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 30 Aug 2026 19:40:03 +0200 Subject: [PATCH 3/3] refactor(svm): a region that does not clip is no region `REGION_NULL` was a `bool null` beside the bands, which put "do not clip" and "clip everything away" one flag apart while they mean the opposite - and a null region whose flag goes unread yields empty path data, so it clips everything. `std::optional` makes the unwrap the only way in. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XQpLmJpJ87qbKoG8B7kbLY --- src/odr/internal/svm/svm_format.cpp | 15 ++++++++------- src/odr/internal/svm/svm_format.hpp | 15 ++++++++------- src/odr/internal/svm/svm_to_svg.cpp | 12 ++++++------ 3 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/odr/internal/svm/svm_format.cpp b/src/odr/internal/svm/svm_format.cpp index b9ba2618d..fe47e087c 100644 --- a/src/odr/internal/svm/svm_format.cpp +++ b/src/odr/internal/svm/svm_format.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -786,9 +787,7 @@ svm::BitmapAction svm::read_bitmap_action(std::istream &in, return result; } -svm::Region svm::read_region(std::istream &in) { - Region result; - +std::optional svm::read_region(std::istream &in) { const VersionLength vl = read_version_length(in); std::uint16_t content_version{}; std::uint16_t type{}; @@ -796,9 +795,10 @@ svm::Region svm::read_region(std::istream &in) { read_primitive(in, type); if (type == region_null) { - result.null = true; - return result; + return std::nullopt; } + + Region result; if (type == region_empty) { return result; } @@ -839,8 +839,9 @@ svm::Region svm::read_region(std::istream &in) { return result; } -std::pair svm::read_clip_region_action(std::istream &in) { - Region region = read_region(in); +std::pair, bool> +svm::read_clip_region_action(std::istream &in) { + std::optional region = read_region(in); bool clip{}; read_primitive(in, clip); return {std::move(region), clip}; diff --git a/src/odr/internal/svm/svm_format.hpp b/src/odr/internal/svm/svm_format.hpp index 09df69446..c60f199a3 100644 --- a/src/odr/internal/svm/svm_format.hpp +++ b/src/odr/internal/svm/svm_format.hpp @@ -4,6 +4,7 @@ #include #include +#include #include #include #include @@ -269,11 +270,10 @@ struct TextRectangleAction final { }; /// A clip region: bands covering it as a union of rectangles, and from -/// version 2 the poly-polygon those were rasterised from. +/// version 2 the poly-polygon those were rasterised from. An *empty* region +/// covers nothing and so clips everything away; `REGION_NULL`, which does not +/// clip at all, is no region and reads as `std::nullopt`. struct Region final { - /// `REGION_NULL`: no clipping at all, as against a region that covers - /// nothing and clips everything away. - bool null{}; std::vector rectangles; std::vector> polygons; }; @@ -366,10 +366,11 @@ std::uint16_t read_push_action(std::istream &in, const VersionLength &vl); /// The `TextAlign` of a `TEXTALIGN`. std::uint16_t read_text_align_action(std::istream &in); /// A region, as `ReadRegion` reads one: a band list, and from version 2 the -/// poly-polygon it came from. -Region read_region(std::istream &in); +/// poly-polygon it came from. `std::nullopt` where it does not clip at all. +std::optional read_region(std::istream &in); /// A `CLIPREGION`: the region, and whether it clips at all. -std::pair read_clip_region_action(std::istream &in); +std::pair, bool> +read_clip_region_action(std::istream &in); /// A dib with its file header, as `ReadDIB(…, bFileHeader=true)` reads one. /// @p limit is what the enclosing action declared, so a length field cannot /// ask for more than the file holds. diff --git a/src/odr/internal/svm/svm_to_svg.cpp b/src/odr/internal/svm/svm_to_svg.cpp index f7e410c1d..83ca315dc 100644 --- a/src/odr/internal/svm/svm_to_svg.cpp +++ b/src/odr/internal/svm/svm_to_svg.cpp @@ -10,6 +10,7 @@ #include #include +#include #include #include #include @@ -715,10 +716,10 @@ void translate_action(const ActionHeader &action_header, std::istream &in, write_bitmap(action, context); } break; case META_CLIPREGION_ACTION: { - auto [region, clip] = read_clip_region_action(in); + const auto [region, clip] = read_clip_region_action(in); state.clip.clear(); - if (clip && !region.null) { - intersect_clip(get_region_path_data(region, context), state); + if (clip && region) { + intersect_clip(get_region_path_data(*region, context), state); } } break; case META_ISECTRECTCLIPREGION_ACTION: { @@ -727,9 +728,8 @@ void translate_action(const ActionHeader &action_header, std::istream &in, intersect_clip(get_path_data_string({&polygon, 1}, true, context), state); } break; case META_ISECTREGIONCLIPREGION_ACTION: { - const Region region = read_region(in); - if (!region.null) { - intersect_clip(get_region_path_data(region, context), state); + if (const std::optional region = read_region(in)) { + intersect_clip(get_region_path_data(*region, context), state); } } break; case META_TEXTALIGN_ACTION: