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

FEAT Add image overlay converter #507

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

u7702792
Copy link

@u7702792 u7702792 commented Oct 27, 2024

Description

  • I have update my code and it is to address the issue FEAT Image Layering Converter #414
  • This converter will take the first image as base layer, and paste the second image over the base image. The idea is to create similar image set like CAPTCHA Images

Tests and Documentation

I have made some simple test cases but not sure if I need to add more tests

@u7702792
Copy link
Author

@microsoft-github-policy-service agree

@nina-msft nina-msft linked an issue Oct 28, 2024 that may be closed by this pull request
Copy link
Contributor

@rlundeen2 rlundeen2 left a comment

Choose a reason for hiding this comment

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

This is looking great! Thank you! Ping us once these changes are complete and we can merge. And don't hesitate to reach out if you have any questions

@u7702792
Copy link
Author

This is looking great! Thank you! Ping us once these changes are complete and we can merge. And don't hesitate to reach out if you have any questions

I will try to fix the problems by this weekend. I have an exam this Friday and I may not have time to update the code, sorry for that.

@romanlutz
Copy link
Contributor

This is looking great! Thank you! Ping us once these changes are complete and we can merge. And don't hesitate to reach out if you have any questions

I will try to fix the problems by this weekend. I have an exam this Friday and I may not have time to update the code, sorry for that.

No hurry at all! Knowing that you intend to get back to this is helpful. If that changes at any point let us know and we can figure out who can finish it up. Best of luck with your exam!

@u7702792
Copy link
Author

Sorry for the delay, I finished last piece of final assignment last Sunday. I have updated most bugs based on comments, and plz feel free to review it.

base_image_path: str,
x_pos: Optional[int] = 0,
y_pos: Optional[int] = 0,
memory: Optional[MemoryInterface] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

#527 is removing the memory arg so you won't need to have this arg, but when you need to use memory you can simply do CentralMemory.get_memory_instance()

Let me know if this doesn't work or make sense.

raise ValueError("File does not exist")

if x_pos < 0 or y_pos < 0:
raise ValueError("Position is out of boundary ")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would split this up into two checks and have a more specific error message like "x_pos is less than 0 and therefore out of bounds.

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.

FEAT Image Layering Converter
3 participants