Crop tile mapping - #45
Merged
Merged
Conversation
Fit was re-run from a hardcoded list naming only ResizeWidth and ResizeHeight, so every other edit that resizes the document left the view scaled for the previous one: editing a crop, dragging the crop overlay, disabling a resize, and removing one. Any new geometry-changing modifier would have joined them. Compare document_size across the edit instead. That is the actual condition, it cannot go stale when a modifier is added, and it leaves size-preserving edits alone so a deliberate zoom is not disturbed. Opening and closing the crop tool is the one refit this cannot see: it switches between the cropped and uncropped extents while the document keeps its size, so SelectTool keeps refitting explicitly. every_edit_that_changes_the_document_refits promised the general invariant but its cases were four Resize params, and it skipped any case whose document did not change, so a no-op case proved nothing. It now covers crop params too and asserts each case actually resizes. The two size-preserving params it held move to a test asserting the view is left alone.
to_doc derived the source region the document was made from as "source minus the crop's origin", which leaves the crop's own extent in the ratio handed to tile_out_rect. A crop at the origin therefore rescaled by doc/src: a 10000px crop of a 30000px source mapped source pixel 100 to document pixel 33, squeezing the texture across the quad by exactly the crop's fraction. Pass the kept region explicitly instead. chain_kept_extent walks the plan the way chain_doc_offset already does, narrowing at each crop and ignoring resizes, since a resize changes the document's size without changing which source pixels it stands for. This path only runs for per-tile ROIs, so a single-tile source never reached it. That is why it took a large tiled image to see, and why every small-image test stayed green. a_crop_translates_its_tiles_instead_of_scaling_them computed its own expectation with the same wrong formula, so it could only ever agree with the code. It now asserts the translation directly, alongside new cases for a crop at the origin, a crop then a resize, and a resize with no crop.
prepare_modifiers takes dirty out of pending_source_dirty at the top, then may defer to refresh_display_transforms and return. That return dropped the flag, so a frame that asked for a reprocess while the view was still interacting lost it: nothing marks it again once the view settles, and the pipeline keeps whatever it last rendered. Only chains with a non-pointwise modifier can defer, which is why a plain image is unaffected and one with a Resize is not. The decision is now defer_decision, a pure function, so the carry rule is testable. prepare_modifiers itself still needs a real device and a TiledSource, so the surrounding path remains untested -- the same gap that let the crop display bugs through.
place_tile measures each tile's quad from doc_region, which is in source pixels, while the view's scale is chosen against the document: fit is viewport/document. A resize keeps every source pixel, so doc_region stays the whole source while the document shrinks, and the two spaces differ by exactly the resize ratio. The quads were therefore that much too large. A 30000px source resized to 25% fitted in a 1600x900 viewport drew 3600px wide where fit had chosen 900px, which reads as the image refusing to shrink when you resize it down. ViewGeometry now carries doc_size and inv_tile_vp divides the ratio back out. Without a resize the ratio is 1, which is why a plain image and a crop-only chain were both unaffected: a crop narrows doc_region and the document together.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three display bugs on tiled and resized documents
All three were invisible to the suite and found by using the app. They share one shape: a value in one coordinate space used where another was meant. The spaces coincide in the common case, so the wrong code is correct until a chain has both a crop and a resize, or until the image is large enough to be tiled.
The spaces:
Crop stretched the texture on a tiled image
to_docderived the kept region assrc_w - offset.x, source minus the crop's origin, which leaves the crop's extent in the ratio handed totile_out_rect. A 10000px crop of a 30000px source mapped source pixel 100 to document pixel 33, squeezing the texture by exactly the crop's fraction. It now takes the kept region explicitly, withchain_kept_extentwalking the plan the waychain_doc_offsetalready does.Only reachable via
build_roi_display_bgswhenroi_active, which needs multiple tiles. A single-tile source never hits it, which is why it took a large image to see.A resized document drew at its old size
place_tilemeasures quads fromdoc_region, in source pixels, while the view's scale is chosen against the document (fitisviewport / document). A resize keeps every source pixel, so the two differ by exactly the resize ratio. A 30000px source resized to 25% in a 1600x900 viewport drew 3600px wide where fit had chosen 900px, so resizing down left the image large.ViewGeometrynow carriesdoc_sizeandinv_tile_vpdivides the ratio back out. Without a resize the ratio is 1.Editing a crop never refit the view
updaterefit from a list naming onlyResizeWidth/ResizeHeight, so crop edits, overlay drags, disabling a resize, and removing one all left the view scaled for the previous document. It now comparesdocument_size()across the edit, which is the condition itself and cannot go stale when a modifier is added. Size-preserving edits are still left alone so a deliberate zoom is not disturbed.Also here
Deferring in
prepare_modifiersconsumed thedirtyit postponed, losing the reprocess entirely. Found while investigating, and not the cause of either symptom above. The decision is nowdefer_decision, a pure function, so the carry rule is testable. Separately,doc_offsetanddoc_keptwere set after the all-tiles-culled early return, the same staleness the module header already documents fordoc_size.Tests
a_crop_translates_its_tiles_instead_of_scaling_themcomputed its expectation with the same wrong formula as the code, so it could only ever agree. It now asserts the translation directly, alongside cases for a crop at the origin, crop-then-resize, and resize-with-no-crop.every_edit_that_changes_the_document_refitspromised a general invariant but listed four Resize params and skipped any case whose document did not change, so two of its four were silent no-ops. It now covers crop params and asserts each case actually resizes.inscribe_transformhad no coverage despite positioning every processed texture on the ROI path. It was correct, and tests were added.414 tests pass with
--features heif,av. Each fix was verified by breaking it and watching the relevant test fail.Still not covered
prepare_modifiersneeds a real device and aTiledSource, andViewPipelineis only ever constructed by iced's renderer, so the deferral path has no test-reachable seam. That is the display-harness gap that let these through.