Async Aggregators #1
Conversation
Added the css (qss) for the new UI elements
Main.py is now adjusted to work with new UI elements
Hopefully passes lint
aggregators misspelled
# Conflicts: # main.py
Generate ASCII art with hidden message from input image (Issue #10)
Fix merge conflict
Rename all modules to lower case for consistency
Remove docker from readme
Motion initial values
# Conflicts: # src/images/snake.jpg # src/level.py
add image to readme
Update README.md
Added level selection to the readme
There was a problem hiding this comment.
Ahoy Async Aggregators, congrats on making the top 10! I'm glad I pulled your project to review because I particularly enjoyed it whlie judging.
I only made comments where I had something to bring up, so don't think I didn't like anything about your code. I actually thought the code quality was pretty high overall.
README
Installation was very smooth (makes a good first impression when judging a bunch of projects ;p), and the commit history GIF was a nice touch.
Teamwork
It looks like everyone contributed. 62 PRs is quite a lot! Commit messages were mostly good; a few were a little subpar but that's not unusual. If anyone wants a refresher check out How to Write a Git Commit Message.
Code Style
Some of the things I commented on are nitpicky or simply matters of opinion. In those cases I'm just alerting you to alternative options. Overall, like I said, the code quality was pretty high. There were a couple places that needed more descriptive variable names, and there were some whitespace choices that looked like they might have been made by an overzealous autoformatter.
I think the biggest issue that stood out to me was the use of dictionaries instead of keyword arguments in some places. Passing arguments like that makes it hard to trace how data is moving through your program and what arguments functions expect.
Code Organization
The big problem here was the Level class and (what I'm guessing are) the downstream design decisions. I made comments in several places regarding those, but let me try to summarize and unify them here.
The Level class is a singleton that represents the gamestate rather than a particular level. As such it has to keep track of the data for each level and mutate itself when the user goes on to the next level.
Similarly, the GUI does not have a strong separation of logic for the different levels. I can't be too specific here because I'm not super familiar with pyqt6, but ideally the level would provide the GUI with callbacks to interpret the current state of the puzzle. Instead there is one apply_filter function that handles this logic by parsing strings. I'm guessing this led to the usage of opaque "args" dicts for the filter functions.
There was some unused code scattered around, and in a real project I would expect some logging, but these things can be chalked up to the time pressure in a code jam.
Summary
I thought you did a great job, both when I was sweating on some of the puzzles and when I was reading your code.
If you have any questions or objections feel free to leave comments and I'll try to reply in a reasonably timely manner.
| input_img, coordinates = prepare_input(Path(str(args.get("image_to_edit")))) | ||
| output_img_path = Path(image_dir_path, "ascii_output.png") | ||
| ascii_file_path = Path(image_dir_path, "ascii.txt") | ||
| generate_ascii_file(input_img, ascii_file_path, 2) |
There was a problem hiding this comment.
Is there a purpose to saving the text to a file before immediately reopening it in seed_secret and ascii_to_image? It would probably be better to keep the text in memory and save the four file IO operations.
It looks like PIL has a way to convert to a QImage, which QPixmap can take.
There was a problem hiding this comment.
I would expect a function named ascii_to_image to just return an image object. If its main purpose is a side-effect like writing a file, I'd prefer that to be reflected in the name, e.g. write_ascii_png or something.
| def prepare_input(img_path: Path) -> Tuple[Image.Image, List[int]]: | ||
| """ | ||
| Prepare the input image by center cropping into a square | ||
|
|
||
| :param img_path: input image file path | ||
| :return: Tuple of cropped input Image and the coordinates of the cropped area | ||
| """ | ||
| img = Image.open(img_path) | ||
|
|
||
| # Calculate the square size | ||
| width, height = img.size | ||
| size = min(width, height) | ||
|
|
||
| # Calculate the coordinates for cropping | ||
| left = (width - size) // 2 | ||
| top = (height - size) // 2 | ||
| right = (width + size) // 2 | ||
| bottom = (height + size) // 2 | ||
|
|
||
| # Crop the image to a square | ||
| return img.crop((left, top, right, bottom)), [left, top, right, bottom] |
There was a problem hiding this comment.
This function's job confuses me a little bit. As far as I can tell those coordinates are only used to calculate the width and height of the cropped section, but those are properties of the Image object it's returning anyway.
Clarifying the purpose/type of the function would also allow giving it a more descriptive name, e.g. square_crop.
| return np.average(np_img.reshape(w * h)) | ||
|
|
||
|
|
||
| def img_to_ascii(img: Image.Image, dens: int) -> List[str]: |
There was a problem hiding this comment.
Since 3.9, you can use the built-in types here (note the capitalization of the typehint).
| def img_to_ascii(img: Image.Image, dens: int) -> List[str]: | |
| def img_to_ascii(img: Image.Image, dens: int) -> list[str]: |
| rows = int(gs_img_h / h) # Number of rows of text | ||
|
|
||
| # Generate ascii art | ||
| ascii_ = [] |
There was a problem hiding this comment.
When initializing an empty list like this, it's good to typehint the values that will be added to it later.
| ascii_ = [] | |
| ascii_: list[str] = [] |
|
|
||
| # Convert to greyscale (L stands for luminance) | ||
| gs_img = img.convert("L") | ||
| gs_img_w, gs_img_h = gs_img.size[0], gs_img.size[1] |
There was a problem hiding this comment.
| gs_img_w, gs_img_h = gs_img.size[0], gs_img.size[1] | |
| gs_img_w, gs_img_h = gs_img.size |
| # Convert the numpy array (OpenCV image) to a QImage | ||
| height, width, channel = unmasked_image_rgb.shape | ||
| bytes_per_line = 3 * width | ||
| q_image = QImage(unmasked_image_rgb.data, width, height, bytes_per_line, QImage.Format.Format_RGB888) | ||
|
|
||
| # Convert the QImage to QPixmap | ||
| pixmap = QPixmap.fromImage(q_image) | ||
| resized_pixmap = pixmap.scaled(args["image_label_w"], args["image_label_h"]) |
There was a problem hiding this comment.
This is a good candidate to be abstracted into a function.
| def __init__(self, minimum: int, maximum: int, interval: int = 1, | ||
| orientation: Qt.Orientation = Qt.Orientation.Horizontal, | ||
| labels: list | None = None, p0: typing.Any = 0, parent: typing.Any = None, | ||
| suppress_mouse_move: bool = False) -> None: |
There was a problem hiding this comment.
Personally I think this is easier to read, but opinions vary.
| def __init__(self, minimum: int, maximum: int, interval: int = 1, | |
| orientation: Qt.Orientation = Qt.Orientation.Horizontal, | |
| labels: list | None = None, p0: typing.Any = 0, parent: typing.Any = None, | |
| suppress_mouse_move: bool = False) -> None: | |
| def __init__(self, | |
| minimum: int, maximum: int, | |
| interval: int = 1, | |
| orientation: Qt.Orientation = Qt.Orientation.Horizontal, | |
| labels: list | None = None, | |
| p0: typing.Any = 0, | |
| parent: typing.Any = None, | |
| suppress_mouse_move: bool = False, | |
| ) -> None: |
| if labels is not None: | ||
| if not isinstance(labels, (tuple, list)): | ||
| raise Exception("<labels> is a list or tuple.") | ||
| if len(labels) != len(levels): | ||
| raise Exception("Size of <labels> doesn't match levels.") | ||
| self.levels = list(zip(levels, labels)) | ||
| else: | ||
| self.levels = list(zip(levels, map(str, levels))) |
There was a problem hiding this comment.
This version is more lenient on what types it will accept for labels, which could be good or bad. In general though, it's best to embrace the duck-typing and let the user pass whatever they want as long as it works. This version also has fewer branches, making it easier to read.
| if labels is not None: | |
| if not isinstance(labels, (tuple, list)): | |
| raise Exception("<labels> is a list or tuple.") | |
| if len(labels) != len(levels): | |
| raise Exception("Size of <labels> doesn't match levels.") | |
| self.levels = list(zip(levels, labels)) | |
| else: | |
| self.levels = list(zip(levels, map(str, levels))) | |
| if labels is None: | |
| labels = map(str, levels) | |
| labels = list(labels) | |
| if len(labels) != len(levels): | |
| raise ValueError("Size of <labels> doesn't match levels.") | |
| try: | |
| self.levels = list(zip(levels, labels)) | |
| except TypeError: | |
| raise TypeError("<labels> is not iterable.") |
| length = style.pixelMetric(QStyle.PixelMetric.PM_SliderLength, st_slider, self.sl) | ||
| available = style.pixelMetric(QStyle.PixelMetric.PM_SliderSpaceAvailable, st_slider, self.sl) | ||
|
|
||
| for v, v_str in self.levels: |
There was a problem hiding this comment.
Should probably be more descriptive names, e.g. slider_value, slider_label.
|
|
||
| text_img = Image.new('1', (text_width, text_height), color='white') | ||
| text_draw = ImageDraw.Draw(text_img) | ||
| text_draw.text((-left, -top), message, fill='black', font=font) # Notice the negative offsets |
There was a problem hiding this comment.
These comments sound like they're for a tutorial. Make sure to take your expected audience into account when writing your code and comments.
There was a problem hiding this comment.
Ahoy Async Aggregators, congrats on making the top 10! I'm glad I pulled your project to review because I particularly enjoyed it whlie judging.
I only made comments where I had something to bring up, so don't think I didn't like anything about your code. I actually thought the code quality was pretty high overall.
README
Installation was very smooth (makes a good first impression when judging a bunch of projects ;p), and the commit history GIF was a nice touch.
Teamwork
It looks like everyone contributed. 62 PRs is quite a lot! Commit messages were mostly good; a few were a little subpar but that's not unusual. If anyone wants a refresher check out How to Write a Git Commit Message.
Code Style
Some of the things I commented on are nitpicky or simply matters of opinion. In those cases I'm just alerting you to alternative options. Overall, like I said, the code quality was pretty high. There were a couple places that needed more descriptive variable names, and there were some whitespace choices that looked like they might have been made by an overzealous autoformatter.
I think the biggest issue that stood out to me was the use of dictionaries instead of keyword arguments in some places. Passing arguments like that makes it hard to trace how data is moving through your program and what arguments functions expect.
Code Organization
The big problem here was the Level class and (what I'm guessing are) the downstream design decisions. I made comments in several places regarding those, but let me try to summarize and unify them here.
The Level class is a singleton that represents the gamestate rather than a particular level. As such it has to keep track of the data for each level and mutate itself when the user goes on to the next level.
Similarly, the GUI does not have a strong separation of logic for the different levels. I can't be too specific here because I'm not super familiar with pyqt6, but ideally the level would provide the GUI with callbacks to interpret the current state of the puzzle. Instead there is one apply_filter function that handles this logic by parsing strings. I'm guessing this led to the usage of opaque "args" dicts for the filter functions.
There was some unused code scattered around, and in a real project I would expect some logging, but these things can be chalked up to the time pressure in a code jam.
Summary
I thought you did a great job, both when I was sweating on some of the puzzles and when I was reading your code.
If you have any questions or objections feel free to leave comments and I'll try to reply in a reasonably timely manner.
No description provided.