2
\$\begingroup\$

I have written the beginning of a map engine which runs off PIL and some JavaScript client side. Ignoring the rest of the code that is in the same file as this function, can you suggest any improvements to this code here?

def convert_colour(region_number, incolour, outcolour):
    #print(region_list.regions_d[region_number][0].split('\\')[7], incolour, outcolour)
    img = Image.open(region_list.regions_d[region_number][0])
    img = img.convert("RGBA")
    newData = []
    newData = [outcolour if item==incolour else item for item in img.getdata()]
    img.putdata(newData)
    img.save(region_list.regions_d[region_number][0], "PNG")
    colour_change_single(region_number, outcolour)

Further to a suggestion I have tried to include numpy but recieve the following error:

Traceback (most recent call last):
  File "D:\Anaconda3\Lib\idlelib\image-transparency.py", line 227, in <module>
    run_function_tests()
  File "D:\Anaconda3\Lib\idlelib\image-transparency.py", line 219, in run_function_tests
   run_engine_test(change_list)
  File "D:\Anaconda3\Lib\idlelib\image-transparency.py", line 161, in run_engine_test
    convert_colour(x[0], region_list.regions_d[x[0]][1], x[1])
  File "D:\Anaconda3\Lib\idlelib\image-transparency.py", line 115, in convert_colour
    new_img = _convert_colour(img, incolour, outcolour)
  File "D:\Anaconda3\Lib\idlelib\image-transparency.py", line 108, in _convert_colour
    img[_colour_mask(img, incolour)] = outcolour
ValueError: shape mismatch: value array of shape (3,) could not be broadcast to indexing result of shape (216,4)

I realised there needs maybe some clarity about this traceback. run_function_tests() this has code to run a short test with my old code.

def run_engine_test(change_list):
for x in change_list:
    convert_colour(x[0], region_list.regions_d[x[0]][1], x[1])

change_list = [(n,random.choice(colour_list.colour_list)) for n in region_list.regions_d]

We also have a function called colour detect which will check for patterns in the colour

def colour_detect():
#Detect if colours in the image match the colours on the palette and re-maps the dictionary if a region colour is changed
    detected_colours = []
        for x in region_list.regions_d:
            region_colour = print_img_data(region_list.regions_d[x][0])
            region_list.regions_d[x] = [region_list.regions_d[x][0], region_colour]
        if region_colour not in detected_colours:
            detected_colours.append(region_colour)

This is intended to be run at the beginnig of a "game or round" and then in between the engine ticks when actions like wars are comitted. Hence why it checks all of the images to ensure that the dictionary has been updated correctly.

After that the traceback comes from your functions.

[Edit] The error was related to a typo in my code. The suggestion provided reduced the time to do the functions by 2/3rds!!!!

This is the image i am editing (c) Image that I am editing, note, all sections have their own file.

\$\endgroup\$

3 Answers 3

3
\$\begingroup\$

The easiest way is to use the numpy interface for this, since it allows you to do operations on the whole image:

from PIL import Image
import numpy as np

def _colour_mask(img, colour):
    """Finds all indices of a single colour in a PIL.Image"""
    if len(img.shape) == 3:
        return (img == colour).all(axis=2).nonzero()
    elif len(image.shape) == 2:
        return (img == colour).nonzero()
    else:
        raise ValueError("Invalid image shape {}".format(img.shape))


def _convert_colour(img, incolour, outcolour):
    """Replaces incolour with outcolour in a PIL.Image.

    Returns a new PIL.Image.
    Assumes that img has as many channels as len(incolour) and len(outcolour).
    """ 
    img = np.array(img)
    img[_colour_mask(img, incolour)] = outcolour
    return Image.fromarray(img)


def convert_colour(region_number, incolour, outcolour):
    file_name = region_list.regions_d[region_number][0]
    img = Image.open(file_name)
    new_img = _convert_colour(img, incolour, outcolour)
    new_img.save(file_name, "PNG")
    colour_change_single(region_number, outcolour)
\$\endgroup\$
8
  • \$\begingroup\$ I have amended my accepted answer as you have perfectly answered my question. I didn't think about using npy for this, but considering your experience, I will certainly take your advice. Thanks for your time :) \$\endgroup\$
    – Swift
    Commented Aug 22, 2018 at 13:58
  • 1
    \$\begingroup\$ @Swift: It does work with color images for me (you can even call image.convert("RGBA") on it without anything changing). Not sure if this would work with grayscale images, though. \$\endgroup\$
    – Graipher
    Commented Aug 23, 2018 at 9:28
  • \$\begingroup\$ It's just that I get an error about mis-shaped or something like that. Let me reproduce it, perhaps you can explain the error I get. I really want to convert the colour conversion to np as it seems everyone is shouting about how much faster it can process this data for me. \$\endgroup\$
    – Swift
    Commented Aug 23, 2018 at 9:32
  • \$\begingroup\$ @Swift: Without seeing the error I cannot know what the reason is... \$\endgroup\$
    – Graipher
    Commented Aug 23, 2018 at 9:33
  • 1
    \$\begingroup\$ Dont worry, I found the mishaped "Mustard_Yellow" value, like you said, it was missing the aplha value. Im so sorry, what a derp I can be at times haha! \$\endgroup\$
    – Swift
    Commented Aug 23, 2018 at 9:49
3
\$\begingroup\$

global

this uses global variable region_list, but only region_list.regions_d[region_number][0], which is probably a filename. Better would be to just pass the filename to this function, instead of using this global variable

other tasks

what does colour_change_single(region_number, outcolour) do? It looks unrelated to changing the image, so wouldn't it make more sense to have the one invoking convert_colour to also take care of this?

\$\endgroup\$
1
  • \$\begingroup\$ colour_change_single(region_number, outcolour) is used to amend the region_list dictionary class, region number defines the dict index and outcolour is the colour the region has been replaced with. Region list isnt a file, its just a dictionary that I want to change as the game plays out but resets each "round" \$\endgroup\$
    – Swift
    Commented Aug 22, 2018 at 13:57
1
\$\begingroup\$

Add a """docstring""" describing what the function does, and what the arguments do.

Remove newData = []

Perhaps rename incolour and outcolour to more descriptive names.

\$\endgroup\$
2
  • \$\begingroup\$ Sorry to remove my accepted answer, yours certainly had a cadence improvement to the code, best practices shall we say, however, another answer gave an improvement to the code itself, which is exactly what I wanted. It worked but I had no idea how to improve it by adding more. I hope you can understand. Thanks. \$\endgroup\$
    – Swift
    Commented Aug 22, 2018 at 18:54
  • \$\begingroup\$ @Swift No problem. The currently accepted answer is a definite improvement. I will, however, recommend in the future waiting before pulling the trigger on which answer is "accepted" for at least a few days. Unlike Stack Overflow, Code Review is not a "oh this answer solves my problem"; it is more of a "many of these answers all improve the code" forum. \$\endgroup\$
    – AJNeufeld
    Commented Aug 22, 2018 at 18:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.