Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Empty Pipeline lowers image quality #262

Open
erik-koynov opened this issue Apr 3, 2023 · 12 comments
Open

Empty Pipeline lowers image quality #262

erik-koynov opened this issue Apr 3, 2023 · 12 comments
Assignees
Labels
bug Something isn't working enhancement New feature or request

Comments

@erik-koynov
Copy link

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
Copy link
Collaborator

kwcckw commented Apr 3, 2023

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
Copy link
Author

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
Copy link
Collaborator

kwcckw commented Apr 3, 2023

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 jboarman added bug Something isn't working enhancement New feature or request labels Apr 11, 2023
@jboarman
Copy link
Member

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

@kwcckw
Copy link
Collaborator

kwcckw commented Apr 24, 2023

@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
Copy link
Collaborator

kwcckw commented May 15, 2023

@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
Copy link
Member

jboarman commented May 15, 2023

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
Copy link
Collaborator

kwcckw commented May 15, 2023

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

@kwcckw
Copy link
Collaborator

kwcckw commented May 16, 2023

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
Copy link
Contributor

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
Copy link
Collaborator

kwcckw commented May 16, 2023

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
Copy link
Collaborator

kwcckw commented May 19, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
Status: Todo
Development

No branches or pull requests

4 participants