Performance issue
The callback you are using for the b button forces your application to redraw the sea and sand rectangles as many times as the user clicks on the button. You are not supposed to redraw any widget or graphical elements within the canvas when you call draw_optimal_route() except the line which is the only useful information the user expects from your application. This is a real performance issue you must think about.
UX issue
On my laptop, I hardly am able to run your program because the vertical length of the window does not allow me to see the GO button. The best user experience I can get from the GUI is this one:
Of course, this is not an issue if the resolution of your laptop is better than mine.
Design issue
OOP and GUIs are good friends. No wonder that, historically speaking, graphical user interfaces coincided with the birth of object-oriented concepts (source):
Sketchpad can be considered as the first Graphical User Interface
(GUI) ... while the Object-Oriented term itself is formed 4 years
later (in 1966). My guess is that Alan Kay is looking back at those
days and he made Sketchpad as aspiration of what will become of OOP)
This means, at your place, I would start by object-orient that program.
I know there are many Gods out there who say OOP is not good. But I can assure you that OOP is at least your best friend when you deal with graphical user interfaces. If you do not believe me, look how many engineering principles we respect and problem we solve by re-designing your program using the OOP approach:
Use as less parameters as possible
My solution below renders most of your functions niladic (having no argument) and, at worst, few of them are monadic (having one argument). This is an extremely good design principle for 2 main reasons:
1. Parameters belong to a level of abstraction which is different from that of functions.
2. Less parameters makes your tests (such as unit tests) much easier.
Getting rid of the performance issue
The use of OOP principles, allowed me to get rid of the performance issue mentioned in the beginning. This is achieved almost "by default".
Fixing the UX issue
I suggest you to redesign your GUI as shown in the below screenshot. This may be interesting for users with a machine like mine:
Random notes in brief
- Let us comply with PEP8:
Wildcard imports (from import *) should be avoided, as they
make it unclear which names are present in the namespace, confusing
both readers and many automated tools. There is one defensible use
case for a wildcard import, which is to republish an internal
interface as part of a public API (for example, overwriting a pure
Python implementation of an interface with the definitions from an
optional accelerator module and exactly which definitions will be
overwritten isn't known in advance).
This means you could change from tkinter import * to import tkinter as tk.
- You can improve the names of different instances and variables you used. For a small program like that, you can allow yourself, maybe, to name a variable "w" but you can not provide this luxury for yourself in larger programs as you can not remember what they are referring to. Each variable or function name must be well chose to serve its purpose and reflect it as good as possible.
- I think it is a little bit confusing that "Sand Speed / Sea Speed ratio". Actually you commented that the "sea speed" is one, and you never referred to this notion any where in your program. So I suggest you to re-write the label text to refer only to the sand speed.
- I noticed in the same scope, you use the same name for several variables. This is the case, for instance, when you named all the labels by "
l". You are lucky in this context you did not suffer from the consequences of such coding choice, but that is not an excuse you keep that attitude for your further applications.
- Misleading named parameter:
In both definitions of
find_optimal_midpoint_y() and total_time() functions, you set a defined parameter sand_speed=SAND_SPEED, but later when you inject sand_speed_w.get() as the actual argument when you call them through this line of code: midpoint_y = find_optimal_midpoint_y(... sand_speed=sand_speed_w.get()). Of course, you have the right to change the value of named parameter to whatever you want, but I just do not see the point of using it in this context because of two main reasons: first when I tried to redesign your program, I have been thinking to get rid of that named parameter and use directly SAND_SPEED within the body of the two functions instead. But then, I found you never used that constant at all, so simply get rid of it. The second thing about it that instead of coding midpoint_y = find_optimal_midpoint_y(... sand_speed=sand_speed_w.get()) you can simply write midpoint_y = find_optimal_midpoint_y(... sand_speed_w.get()) because you can use named parameters just as you use mandatory ones.
- Misuse of inner functions
The pythonic purpose of inner functions is rather to devise decorators. Using inner functions notion in your context leads to the performance issue mentioned above.
- In addition to SEA_SPEED, you should get rid of LIFEGUARD_Y and CHILD_Y constants which seem to be useless for your program.
Solution
Given the points I mentioned above and some other minor things I could not write right now (I'm a little sick), I came up with this solution. You can refer to tkinter best practices article to understand the logic and, more importantly, please do not hesitate to comment anything you do not understand in my approach below. Note that I ended up with more code, but a cleaner and scalable one:
import tkinter as tk
WIDTH = 500
HEIGHT = 500
class FeymannLifeguard(tk.Frame):
def __init__(self, master):
self.master = master
tk.Frame.__init__(self, self.master)
self.configure_gui()
self.create_widgets()
def configure_gui(self):
self.master.title('Feymann lifeguard route problem')
self.master.geometry('500x600')
self.master.resizable(False, False)
def create_widgets(self):
self.create_sea_sand_area()
self.create_control_area()
def create_sea_sand_area(self):
self.sea_sand_area = tk.Canvas(self.master, width=WIDTH, height=HEIGHT)
self.sea_sand_area.pack()
self.sand_area = self.sea_sand_area.create_rectangle(0, 0, WIDTH//2, HEIGHT, fill='yellow')
self.sea_area = self.sea_sand_area.create_rectangle(WIDTH//2, HEIGHT, WIDTH, 0, fill='blue')
def create_control_area(self):
self.control_area = tk.Canvas(self.master, width=WIDTH, height=100)
self.control_area.pack()
self.create_control_widgets()
def create_control_widgets(self):
self.lifeguard_label = tk.Label(self.control_area, text='Lifeguard')
self.lifeguard_scale = tk.Scale(self.control_area, from_=0, to=HEIGHT, orient=tk.HORIZONTAL)
self.child_label = tk.Label(self.control_area, text='Child')
self.child_scale = tk.Scale(self.control_area, from_=0, to=HEIGHT, orient=tk.HORIZONTAL)
self.sand_speed_label = tk.Label(self.control_area, text="Sand Speed")
self.sand_speed_scale = tk.Scale(self.control_area, from_=1, to=3, resolution=0.1, orient=tk.HORIZONTAL)
self.go = tk.Button(self.control_area, text='GO', command=self.draw_optimal_route)
self.position_control_widgets()
def position_control_widgets(self):
self.lifeguard_label.grid(row=0, column=0, padx=0, sticky=tk.W)
self.lifeguard_scale.grid(row=1, column=0, sticky=tk.W)
self.child_label.grid(row=0, column=1, padx=20, sticky=tk.W)
self.child_scale.grid(row=1, column=1, padx=20, sticky=tk.W)
self.sand_speed_label.grid(row=0, column=2, padx=20, sticky=tk.W)
self.sand_speed_scale.grid(row=1, column=2, padx=20, sticky=tk.W)
self.go.grid(row=1, column=3, sticky=tk.S, padx=20)
def distance(self, a, b):
return ((a[0] - b[0])**2 + (a[1]-b[1])**2)**0.5
def total_time(self, midpoint):
sand_len = self.distance((self.lifeguard_scale.get(), 0), (midpoint, WIDTH//2))
sea_len = self.distance((midpoint, WIDTH//2), (self.child_scale.get(), WIDTH))
sand_time = sand_len/self.sand_speed_scale.get()
sea_time = sea_len / 1
return sand_time + sea_time
def find_optimal_midpoint(self):
return min(range(0, 500), key=lambda y: self.total_time(y))
def draw_optimal_route(self):
self.clear_sea_sand_area()
self.midpoint_y = self.find_optimal_midpoint()
self.sea_sand_area.create_line(0, self.lifeguard_scale.get(), WIDTH//2, self.midpoint_y, tag='sea_line')
self.sea_sand_area.create_line(WIDTH//2, self.midpoint_y, WIDTH, self.child_scale.get(), tag='sand_line')
def clear_sea_sand_area(self):
self.sea_sand_area.delete('sea_line')
self.sea_sand_area.delete('sand_line')
if __name__ == '__main__':
root = tk.Tk()
FeymannLifeguard(root)
root.mainloop()
