sparkfish / augraphy

Augmentation pipeline for rendering synthetic paper printing, faxing, scanning and copy machine processes
https://github.com/sparkfish/augraphy
MIT License
339 stars 44 forks source link

Empty Pipeline lowers image quality #262

Open erik-koynov opened 1 year ago

erik-koynov commented 1 year ago

Hi all, I am working with a custom randomized version of the AugraphyPipeline and in the course of trials I noticed that even if no agumentations were passed to the AugraphyPipeline - i.e. three empty lists- it still distorts the image.

kwcckw commented 1 year ago

Could you show us an example of input image and output image?

Right now there's an overlay process to print ink layer to paper layer even though there isn't any augmentation in ink layer. The default overlay method is using cv2.addWeighted so the output image may looks faded.

You may insert a different overlay method in order to use a different overlay method, for example:

default method:

pipeline = AugraphyPipeline(ink, paper, post, overlay_type="ink_to_paper")

Or something like:

pipeline = AugraphyPipeline(ink, paper, post, overlay_type="darken")

Here's a list of supported overlay_type:

    "ink_to_paper"
    "min"
    "max"
    "mix"
    "normal"
    "lighten"
    "darken"
    "addition"
    "subtract"
    "difference"
    "screen"
    "dodge"
    "multiply"
    "divide"
    "hard_light"
    "grain_extract"
    "grain_merge"
    "overlay"

Most of the overlay methods are adapted from here: https://github.com/flrs/blend_modes

And visualization of their overlay methods: https://blend-modes.readthedocs.io/en/latest/visual_examples.html

erik-koynov commented 1 year ago

Ok. I can propose having the overlay types in a Enum-like structure - as class variables in a separate OverlayType class such that it is easier for IDEs to display the different options. I will submit a pull request at some later point.

kwcckw commented 1 year ago

Ok. I can propose having the overlay types in a Enum-like structure - as class variables in a separate OverlayType class such that it is easier for IDEs to display the different options. I will submit a pull request at some later point.

Yes, please submit a pull request, thanks.

jboarman commented 1 year ago

@kwcckw Do you think you have enough information for us to add this issue to the current sprint?

kwcckw commented 1 year ago

@kwcckw Do you think you have enough information for us to add this issue to the current sprint?

I think this is important but not urgent yet, because later we will need to restructure the whole repo so that it can display the options in a similar way.

kwcckw commented 1 year ago

@proofconstruction Regarding this proposed method - Enum-like structure - as class variables in a separate OverlayType class such that it is easier for IDEs to display the different options, do you see a better way to do so?

Actually it maybe too messy if we want to create a subclass for each method in a class.

jboarman commented 1 year ago

Do we really need to subclass each method? Would something like this work?

from enum import Enum

class Overlay(str, Enum):
    INK_TO_PAPER = "ink_to_paper"
    MIN = "min"
    MAX = "max"
    MIX = "mix"
    NORMAL = "normal"
    LIGHTEN = "lighten"
    DARKEN = "darken"
    ADDITION = "addition"
    SUBTRACT = "subtract"
    DIFFERENCE = "difference"
    SCREEN = "screen"
    DODGE = "dodge"
    MULTIPLY = "multiply"
    DIVIDE = "divide"
    HARD_LIGHT = "hard_light"
    GRAIN_EXTRACT = "grain_extract"
    GRAIN_MERGE = "grain_merge"
    OVERLAY = "overlay"

# prints value, offers comparison testing
print(f"{Overlay.INK_TO_PAPER}")
print(f"{(Overlay.INK_TO_PAPER == 'ink_to_paper')}")

Then overlay values could be passed as an enum, while internally treated as a string where code is not yet using the Enum:

pipeline = AugraphyPipeline(ink, paper, post, overlay_type=Overlay.INK_TO_PAPER)
kwcckw commented 1 year ago

Thanks, i didn't think of this. I guess this should work, i will try it later.

kwcckw commented 1 year ago

Do we really need to subclass each method? Would something like this work?

from enum import Enum

class Overlay(str, Enum):
    INK_TO_PAPER = "ink_to_paper"
    MIN = "min"
    MAX = "max"
    MIX = "mix"
    NORMAL = "normal"
    LIGHTEN = "lighten"
    DARKEN = "darken"
    ADDITION = "addition"
    SUBTRACT = "subtract"
    DIFFERENCE = "difference"
    SCREEN = "screen"
    DODGE = "dodge"
    MULTIPLY = "multiply"
    DIVIDE = "divide"
    HARD_LIGHT = "hard_light"
    GRAIN_EXTRACT = "grain_extract"
    GRAIN_MERGE = "grain_merge"
    OVERLAY = "overlay"

# prints value, offers comparison testing
print(f"{Overlay.INK_TO_PAPER}")
print(f"{(Overlay.INK_TO_PAPER == 'ink_to_paper')}")

Then overlay values could be passed as an enum, while internally treated as a string where code is not yet using the Enum:

pipeline = AugraphyPipeline(ink, paper, post, overlay_type=Overlay.INK_TO_PAPER)

So i tried this but i don't see how this will be clearer for the users to select the options? The input to overlay method is still str and we are just changing the input to class instance's parameter.

proofconstruction commented 1 year ago

The different values defined in the enum type can be displayed as hints by some IDEs.

Using custom enums for parameters with a low-finite number of possible values (like curling_direction from BookBinding) is a good idea in general, because it helps prevent users from calling functions with values which are invalid but still have the correct type, like if someone put curling_direction = 99. In this case, the user would get a ValueError: invalid enum value.

kwcckw commented 1 year ago

The different values defined in the enum type can be displayed as hints by some IDEs.

Using custom enums for parameters with a low-finite number of possible values (like curling_direction from BookBinding) is a good idea in general, because it helps prevent users from calling functions with values which are invalid but still have the correct type, like if someone put curling_direction = 99. In this case, the user would get a ValueError: invalid enum value.

Thanks, but to relate it into current case, we have overlay_type = str, so if we define the class like this:

  class OverlayTypes(str, Enum):
      """Contains methods to overlay ink into paper"""

      ink_to_paper = "ink_to_paper"
      min = "min"
      max = "max"
      mix = "mix"
      normal = "normal"
      lighten = "lighten"
      darken = "darken"
      addition = "addition"
      subtract = "subtract"
      difference = "difference"
      screen = "screen"
      dodge = "dodge"
      multiply = "multiply"
      divide = "divide"
      hard_light = "hard_light"
      grain_extract = "grain_extract"
      grain_merge = "grain_merge"
      overlay = "overlay"  

The pipeline will be initialized with this:

pipeline = AugraphyPipeline(ink, paper, post, overlay_type=getattr(OverlayTypes, overlay_type))

Where overlay_type is the user input in str.

If user uses a wrong input, actually it won't show all the possible options too, for example:

 getattr(OverlayTypes, "1234")

The error is

       raise AttributeError(name) from None

  AttributeError: 1234

And it gives hints on types only. Using getattr(OverlayTypes, 1234) gives this error:

 getattr(): attribute name must be string

Or did I missed out anything and it should be done in a different way?

kwcckw commented 1 year ago

I will leave this open first unless it is proven the Enum method is able to show all the available options.