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

modifying colored_img_generator to loop over known colors. #100

Merged
merged 8 commits into from
Jun 23, 2023

Conversation

KaceCM
Copy link
Contributor

@KaceCM KaceCM commented Jun 22, 2023

Before this correction, the function raised "ValueError("Not enough default colors! Pass additional " 'colors to "colors" parameter')", but we couldn't pass additional colors.

To avoid this problem and to make it easier, the correction takes the 9 known colors (Blue, Red, Green, Yellow, Magenta, Pink, Gray, Brown, Orange), and loop over them if needed.

@lukasalexanderweber
Copy link
Member

Thanks! Can we keep the error message anyway?

@lukasalexanderweber
Copy link
Member

Or what exactly is idx%len(colors) doing? Just repeating the first color after the last one is reached?

@KaceCM
Copy link
Contributor Author

KaceCM commented Jun 22, 2023

Yes of course we can keep the message, I'm going to add a couple of lines to make it even better.

idx%len(colors) means that, for a given tuple "colors", if the number of images is longer than this tuple length, it will repeat from the first color.
I had a problem using 20 images, so I did this correction and thought it would be useful for people !

By the way, thank you for this insane package !

@KaceCM
Copy link
Contributor Author

KaceCM commented Jun 22, 2023

I have added a warning message, with instructions for those who wish to add their own colours. Otherwise, it loops the default colors.

Let me know if anything is missing, or if you need anything else !

Copy link
Member

@lukasalexanderweber lukasalexanderweber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for reaching out to fix this for many images. Clever idea with the use of Modulo for repeating colors. Sorry for the bad formatting, im on the Smartphone

corners,
sizes,
colors=(
(255, 000, 000),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we leave the comments which code is which color?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But yes I think its a good idea having this as a parameter

), # Orange
):
def colored_img_generator(sizes, colors):
if type(colors) is not tuple:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to remove this check. I understand why one might check the contract but I dont do it somewhere else so I want to stay constant

)

if len(sizes) + 1 > len(colors):
print(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use StitchingWarning from https://github.com/OpenStitching/stitching/blob/main/stitching/stitching_error.py instead of print. I would like to remove this part: ```
Please add additional colors in a tuple.\n
Example of use : colors=((255, 000, 000), (000, 255, 255)).\n

I would like to change to 'Without additional colors, there will be seam masks with identical colors'



def create_img_by_size(size, color=(0, 0, 0)):
def create_img_by_size(size, color):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please leave the default since it would be a breaking change

@KaceCM
Copy link
Contributor Author

KaceCM commented Jun 23, 2023

I have now corrected all your requests, tell me if everything is correct now !

@lukasalexanderweber lukasalexanderweber merged commit 017dfca into OpenStitching:main Jun 23, 2023
2 checks passed
@lukasalexanderweber
Copy link
Member

Thanks a lot! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants