Repository navigation
feat(image-edit): tone and sharpen edits (Adjust Image) - #535
Conversation
48eef55 to
01302c6
Compare
|
Thank you for the PR! Unfortunately this seems to introduce a duplicate copy of the dither algorithms previously also introduced with #534 - much of which is now in Raygeo. I suspect that to remove the Dither algorithm-specific code from this PR, additional Raygeo extensions may be required, such as exposing the Dither matrices through the public API. So this may be a larger change that is needed to avoid the duplication. On the plus side, it would likely make this PR smaller... |
|
I went looking for the dither duplication you flagged so I could move it to Raygeo, but I cannot find it in this PR — asking for a pointer before I start moving code, since the change you describe sounds large. What I checked:
On the dither matrices specifically: the error-diffusion kernels are already delegated to Raygeo via So: could you point at the specific code you saw as a duplicate? If you were looking at #535 stacked on #534 rather than #535 against main, that would explain it — and in that case the right move is to do the Raygeo extraction on #534 (or after it lands) and rebase this one on top, rather than shrinking this PR. Separately, this PR has the same release blocker as #532: it needs |
|
@switch87 Thanks for digging into this, and sorry for not pointing at specific lines right away. Two things conspired to make this hard to spot: the command as pasted ( The duplication I meant is on the current head (
The only spelling difference is the kernel name: raygeo calls it The good news is that your proposed resolution is exactly what #534 did, and it is merged: main's Two of your points I'll happily concede:
One genuine follow-up — not a blocker, and not duplication since it is brand-new code: |
|
Correction to the release point in my comment above, which I've now fixed there as well: I claimed the dither algorithms were unreleased — that was wrong. raygeo 1.62.0 is already on PyPI, the |
01302c6 to
7bd227b
Compare
|
Rebased onto current main (7199db3) — and the duplication you pointed at disappeared without a single line of mine having to go: Green after the rebase, in the pixi environment:
On the follow-up you asked for: agreed, the tone LUT and the unsharp mask belong in |
|
Honestly I think these image editing tools are not raster engraving specific, and it would be more natural to show the edited image on the canvas directly. Image editing is a generally useful tool - tracing as done by the contour step could benefit too - so hiding this in the raster step seems like the wrong location in the architecture. I am thinking a dedicated image editing tools in the Tool menu - and possibly even a completely new image editing addon for proper encapsulation + an image editing tool registry. As it is, the pipeline has to be aware of every single image editing mechanism, which is a clear architectural layer violation. The pipeline should be generic in this regard. It could have a generic "image preprocessing" step without knowing the pre-processing type - but I am not sure that is even needed here. The key question is: Do we want non-destructive image editing? If so, then the workpiece should probably carry a declarative description of the image editing steps, and could perhaps provide a cached convenience method to get the processed image. The alternative: destructive editing. This would mean baking the processed image into the workpiece. I tend towards non-destructive editing, as it is future proof and likely more in line with long term user expectations; the pipeline could then ask for the processed image. So in short, I think this needs a design / plan that we should agree on first. |
|
Agreed on all three points — the raster step is the wrong home for this, the 1. Where the state lives
# rayforge/core/workpiece.py
# An optional override for the workpiece geometry. If set, this takes
# precedence over the source_segment's geometry. It allows for
# non-destructive editing (like splitting) without modifying the
# shared source segment.
self._edited_boundaries: Geometry | None = NoneSo the image edit stack becomes its sibling: # rayforge/image/edit.py
@dataclass(frozen=True)
class ImageEditStep:
"""One non-destructive image editing operation."""
op: str # registry key, stable across versions
params: dict[str, Any] # JSON-serializable, op-specific
enabled: bool = True
On the workpiece rather than the source asset, because one source can be 2. The op registrySame shape as the registries in class ImageEditRegistry:
def register(self, op_cls, *, addon_name=None): ...
def unregister_all_from_addon(self, addon_name) -> int: ...
def get(self, op_name) -> type[ImageEditOp] | None: ...
class ImageEditOp(ABC):
NAME: ClassVar[str] # serialisation key, never renamed
LABEL: ClassVar[str] # gettext-wrapped, for the UI
@staticmethod
def apply(image, params, *, pixels_per_mm, alpha=None): ...Two properties this buys us:
3. Execution — this is the layer-violation fixToday the pipeline learns about image editing in two places:
both fed by Instead the stack runs inside the one method every render already funnels Putting the stack there means all of them see the edited image automatically. Proposed order, appended to what Cache key becomes
Colour semantics. The built-in ops convert to luminance internally. Format. 4. UIThe The five controls move off the Engrave page ( 5. What happens to this PRNothing in #535 has shipped:
The raygeo split you asked for earlier fits here as layer 1: the pure Decisions I'd like your call on
|
|
I don't understand the "both fed by RasterStep.image_adjustments" part. Why does the raster step still play a role here? And about this: I think it would be cleaner to start with a separate refactor PR first, one that introduces the image editing tool registry, and turns the invert into such a generic tool - stripping the invert setting out of the raster step. This would allow to refine the architecture before committing to the full implementation of the additional image editing tools - which should be straight forward once the overall architecture is in place. About your questions:
|
7bd227b to
455a424
Compare
|
On your question about "both fed by The refactor you asked for is up as #552: the image edit registry, One note: #552 is a draft only because GitHub would not let me open it, or mark it ready, as a regular PR ( |
|
Thanks again. Now that main has the image editing addon, I think this PR can be rebased to add the new tools. |
get_processed_image() tags the render with xres/yres from the workpiece size before running the edit stack, so an edit with a physical size (such as a sharpen radius in mm) gives the same result on the canvas and in the pipeline. The processed cache key includes the size, so resizing reapplies the edits. Refs barebaric#515
Brightness, contrast and gamma become the "tone" edit (one lookup table) and the unsharp mask the "sharpen" edit, with its radius in mm. Both run on the workpiece's image via the image edit registry, in pyvips, so the Engrave step and the pipeline need no knowledge of them. Sharpening blurs premultiplied colour, so transparent areas do not darken the edges. Tools > Adjust Image... edits both for the selected workpieces with a live canvas preview; Apply is one undo step and closing discards. Refs barebaric#515
455a424 to
60a6b0d
Compare
switch87
changed the title
|
Rebuilt on current main, on top of the image edit registry. The old step-based commits are gone. Brightness, contrast, gamma and sharpen are now two edits in the Image Editing addon, The only core change: To keep this one reviewable I split off two parts:
One question: is a single "Adjust Image" dialog that edits both ops what you had in mind, or would you prefer one menu entry per tool? |
|
Thanks for updating the PR. I am currently traveling so cannot test, but reading the code this now looks fine, architecture wise. Just one point: I worry about memory ballooning, as I did not see a cache retention policy on the workpiece. On large images this could lead to too much cache. Regarding your suggestion for a follow up PR for a preview dialog - I am not sure why this would help, as I would have assumed this to be obsolete due to the on canvas editing. |
The render and processed-image caches on a workpiece grew by one image per zoom level and per resize, and the source asset's base-image cache by up to ten images regardless of their size. On an 8000x6000 image ten zoom steps and ten resizes kept about 2.9 GB of pixels cached. Add a small thread-safe LruCache with an entry limit and a byte budget, and use it for all three caches: at most 4 workpiece renders and 4 processed images (10 base images per source), each cache limited to 128 MiB of pixels, least recently used first; the newest image is always kept. Edit changes still replace the processed cache instead of adding to it, so slider moves in the Adjust Image dialog keep a single cached result. Refs barebaric#515
|
Thanks, good catch. Measured on an 8000x6000 RGBA image: slider moves were already fine (changing the edits replaces the processed cache), but every zoom level and every resize added an entry to the workpiece's render and processed caches, and the source asset kept up to 10 base images of any size. Ten zoom steps plus ten resizes ended with ~2.9 GB of pixels cached (RSS ~1.8 GB). Pushed 6dcfdf2 with a retention policy:
Tests that pin it:
About the preview dialog: you're right. The on-canvas editing already shows the result live, so a separate preview dialog isn't needed. I won't follow that up. |
The dialog opens non-modal and adopts the step settings dialog's structure: a toolbar view with the page in a scrolled window, 600 px wide, so the preferences margins match it exactly. Brightness, contrast, gamma and sharpen become sliders with percent entries like the laser page's power row, with the 250 ms preview debounce moving into the dialog. Refs barebaric#515
A new screenshot target stages the example photo, opens the dialog with the brightness, contrast and sharpen sliders set, and captures it as addon-image-editing-adjust.webp. The docs embed it after the Adjust Image settings, in every language.
|
Thank you for the PR. I made some refinements because testing it manually I discovered some issues, but I left the general approach untouched. Thanks again! |
This branch was successfully deployed
2 participants
Adds the photo corrections from #515 as image edits in the Image Editing addon, on top of the registry from #552. The edits live on the workpiece; the Engrave step and the pipeline do not know about them.
Added
Design
invert:tone(brightness,contrast,gamma): one lookup table viamaplut, for 8- and 16-bit images; alpha is kept.sharpen(amountin %,radius_mm): an unsharp mask in pyvips. The blur runs on premultiplied colour, so transparent areas do not darken the edges.WorkPiece.get_processed_image()tags the render withxres/yres(pixels per mm, from the workpiece size) before running the stack, so a radius in mm gives the same result on the canvas and in the pipeline. The processed cache key includes the size, so resizing reapplies the edits.ImageEditOp.apply()documents this.raygeo.image.adjustfollow-up discussed earlier..po/.potchanges; the new strings are wrapped in_().Not in this PR (split off)
Tests
rayforge/builtin_addons/rayforge-addon-image-editing/tests(ops, adjustments model, commands, raster equivalence): 70 passed together withtests/core/test_workpiece_image_edits.py,tests/core/test_doc_image_edits.pyandtests/core/image_edit.../tests/ui_gtk/test_adjust_dialog.py(-m ui): 5 passedtests/core tests/image tests/pipeline: 1946 passedsharpenandtone, and one undo restores the originalRefs #515