Looking for the particles that are emitted when a fish is detected in Terraria

Question

The code run perfectly fine, with no errors. However, the entirety of the code is in 1 function and is kind of hard to read. As well as that I'm not sure if and how the code could be more optimised/improved. Thank you for your suggestions and help!

def ScreenShot():
    pydirectinput.click(x=483, y=559 )
    time.sleep(1)
    win32api.mouse_event(win32con.MOUSEEVENTF_LEFTDOWN,0,0)
    time.sleep(.1)
    win32api.mouse_event(win32con.MOUSEEVENTF_LEFTUP,0,0)
    time.sleep(2)
    x = True
    while x == True:
        if x == True:
            if keyboard.is_pressed('q'):  # if key 'q' is pressed 
                print('Stopped Fishing')
                break  # finishing the loop

            screenie = py.screenshot()
            screenie.save("is_fish_there.png")
            print("taking a screenshot")
            Fishisthere = cv2.imread("is_fish_there.png")
            img = Fishisthere[590:614,400:690]
            hsv = cv2.cvtColor(img, cv2.COLOR_BGR2HSV)
            lowerrange = np.array([106, 147, 0])
            upperrange = np.array([179, 255, 245])
            mask = cv2.inRange(hsv, lowerrange, upperrange)
            blue = np.sum(mask)
            print (blue)
        if blue > 15000:
            print("fish has been found")
            win32api.mouse_event(win32con.MOUSEEVENTF_LEFTDOWN,0,0)
            time.sleep(.1)
            win32api.mouse_event(win32con.MOUSEEVENTF_LEFTUP,0,0)
            time.sleep(.5)
            win32api.mouse_event(win32con.MOUSEEVENTF_LEFTDOWN,0,0)
            time.sleep(.1)
            win32api.mouse_event(win32con.MOUSEEVENTF_LEFTUP,0,0)
            time.sleep(1.5)
              



#check()
ScreenShot()

Here is an example of is_fish_there.png:

Answer 1

Any group of things that are doing something can be broken down into smaller functions that encapsulate that particular task. The first few lines of your function are doing some setup so I have created a function that brings out that setup. You might have a better understanding of the semantics of the setup so could improve on the name instead saying something about why the mouse is moved where it is etc.

def setup_fish_detection():
    pydirectinput.click(x=483, y=559 )
    time.sleep(1)
    win32api.mouse_event(win32con.MOUSEEVENTF_LEFTDOWN,0,0)
    time.sleep(.1)
    win32api.mouse_event(win32con.MOUSEEVENTF_LEFTUP,0,0)
    time.sleep(2)

So with this next function, I've tried to take what could conceptually be aggregated into simply taking the screenshot from the two actions and a log that it was originally. This function is then abstracting how we take the screenshot.

def take_screenshot(file_name):
     screenie = py.screenshot()
     screenie.save(file_name)
     print("taking a screenshot")

The detection of the fish in said screenshot is another thing, this could perhaps be broken down further into gathering the metric and comparing it.

def fish_in_screenshot(screenshot):
    img = screenshot[590:614,400:690]
    hsv = cv2.cvtColor(img, cv2.COLOR_BGR2HSV)
    lowerrange = np.array([106, 147, 0])
    upperrange = np.array([179, 255, 245])
    mask = cv2.inRange(hsv, lowerrange, upperrange)
    blue = np.sum(mask)
    print (blue)
    return blue > 15000

Again with the post-detection mechanics, I'm sure you'll be able to better name what this action represents.

def post_detection_handling():
    win32api.mouse_event(win32con.MOUSEEVENTF_LEFTDOWN,0,0)
    time.sleep(.1)
    win32api.mouse_event(win32con.MOUSEEVENTF_LEFTUP,0,0)
    time.sleep(.5)
    win32api.mouse_event(win32con.MOUSEEVENTF_LEFTDOWN,0,0)
    time.sleep(.1)
    win32api.mouse_event(win32con.MOUSEEVENTF_LEFTUP,0,0)
    time.sleep(1.5)

The main function is then left to almost read as pseudocode.

def main():
    setup_fish_detection()
    screenshot_file_name = "is_fish_there.png"
    while not keyboard.is_pressed('q'):
        take_screenshot(screen_shot_file_name)
        screenshot = cv2.imread(screen_shot_file_name)
        if fish_in_screenshot(screenshot):
            print("fish has been found")
            post_detection_handling()
    print('Stopped Fishing')


#check()
if __name__=="__main__":
    main()

In the main function, I've also simplified the while loop which will behave the same as the original code but not if some other places in the while loop were edited.

Having the filename of the screenshot as a separate variable I find helps, it's one of those things that needs to match in the two locations its used but is something that will probably change quite a bit over time.

Answer 2

However, the entirety of the code is in 1 function and is kind of hard to read.

Agreed. And while this is not a major problem for short pieces of code like this, it will turn into a major maintainability nightmare for larger programs.

Splitting a program into multiple functions has several benefits.

• Scope (can't accidentally modify variables you're not supposed to).
• Readability (if the function has a proper name, it becomes clear what is happening without requiring comments).
• Re-usability (you don't have to copy multiple lines of code if you can simply make another call to an earlier defined function).

However, if your question asks whether it's bad to have everything behind a single function call, that's not a problem. It's actually quite common and we usually name this function main.

It's a good practice to start using include guards around main, especially if you ever want to re-use parts of your code. Now your main only gets executed if the script is called directly, not when it's called by import from an external program.

Once we start doing this, we can actually take apart your ScreenShot function. You already had a check function (currently commented out), but all such functions on the same level would go into your new main.

def main():
    #check()
    ScreenShot()

if __name__ == '__main__':
    main()

Ordinarily I don't re-write code that doesn't have its imports and all other relevant requirements listed, since I like being able to test what I write. However, the following re-write should get you started in splitting up your functions further:

def screenshot():
    pydirectinput.click(x=483, y=559 )
    time.sleep(1)
    win32api.mouse_event(win32con.MOUSEEVENTF_LEFTDOWN,0,0)
    time.sleep(.1)
    win32api.mouse_event(win32con.MOUSEEVENTF_LEFTUP,0,0)
    time.sleep(2)
    while True
        if keyboard.is_pressed('q'):  # if key 'q' is pressed 
            print('Stopped Fishing')
            break  # finishing the loop

        screenie = py.screenshot()
        screenie.save("is_fish_there.png")
        print("taking a screenshot")
        Fishisthere = cv2.imread("is_fish_there.png")
        img = Fishisthere[590:614,400:690]
        hsv = cv2.cvtColor(img, cv2.COLOR_BGR2HSV)
        lowerrange = np.array([106, 147, 0])
        upperrange = np.array([179, 255, 245])
        mask = cv2.inRange(hsv, lowerrange, upperrange)
        return np.sum(mask)

def is_fish_found(value):
    if value > 15000:
        print("fish has been found")
        return True
             
def do_clicks():
    win32api.mouse_event(win32con.MOUSEEVENTF_LEFTDOWN,0,0)
    time.sleep(.1)
    win32api.mouse_event(win32con.MOUSEEVENTF_LEFTUP,0,0)
    time.sleep(.5)
    win32api.mouse_event(win32con.MOUSEEVENTF_LEFTDOWN,0,0)
    time.sleep(.1)
    win32api.mouse_event(win32con.MOUSEEVENTF_LEFTUP,0,0)
    time.sleep(1.5) 

def main():
    #check()
    blue = screenshot()
    if is_fish_found(blue):
        do_clicks()

if __name__ == '__main__':
    main()

Answer 3

Statements like this are unnecessary:

x == True

if x is True, this becomes True. If x is False, this becomes False. Therefore, x == True is identical to just saying x. Comparing a boolean value to a boolean doesn't make sense.

Additionally, you have:

while x == True:
    if x == True:

Between the while statement and the if statement, x could not have changed. Therefore, your code is actually doing this:

while x:
    if keyboard.is_pressed('q'):  # if key 'q' is pressed 
        print('Stopped Fishing')
        break  # finishing the loop

    screenie = py.screenshot()
    screenie.save("is_fish_there.png")
    print("taking a screenshot")
    Fishisthere = cv2.imread("is_fish_there.png")
    img = Fishisthere[590:614,400:690]
    hsv = cv2.cvtColor(img, cv2.COLOR_BGR2HSV)
    lowerrange = np.array([106, 147, 0])
    upperrange = np.array([179, 255, 245])
    mask = cv2.inRange(hsv, lowerrange, upperrange)
    blue = np.sum(mask)
    print (blue)
    if blue > 15000:
        print("fish has been found")
        win32api.mouse_event(win32con.MOUSEEVENTF_LEFTDOWN,0,0)
        time.sleep(.1)
        win32api.mouse_event(win32con.MOUSEEVENTF_LEFTUP,0,0)
        time.sleep(.5)
        win32api.mouse_event(win32con.MOUSEEVENTF_LEFTDOWN,0,0)
        time.sleep(.1)
        win32api.mouse_event(win32con.MOUSEEVENTF_LEFTUP,0,0)
        time.sleep(1.5)