Skip to content

Replay timing and Executable#2

Open
GalDotan wants to merge 18 commits intomainfrom
Replay-Timing-
Open

Replay timing and Executable#2
GalDotan wants to merge 18 commits intomainfrom
Replay-Timing-

Conversation

@GalDotan
Copy link
Collaborator

Fixed timing, Added Executable

Copy link
Member

@AsafMeizner AsafMeizner left a comment

Choose a reason for hiding this comment

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

Couldent test the code so il take your word it works but some cleanup is necessery as the code is very hard to read and will be impossible to maintain and upgrade in the future

Copy link
Member

Choose a reason for hiding this comment

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

add pycache to gitignore (both files and folder)

Copy link
Member

Choose a reason for hiding this comment

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

rename to backend_client.py no need to state py in name the extension is enogh

Copy link
Member

Choose a reason for hiding this comment

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

Dont have space in folder names, and call its something other than app data like source or just app

Copy link
Member

Choose a reason for hiding this comment

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

place all icons in a single icons folder

Copy link
Member

Choose a reason for hiding this comment

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

Split to diffrent files components and helpers! dont have everything in one main file.

Comment on lines +4 to +7
try:
from ntcore import NetworkTableInstance
except ImportError:
NetworkTableInstance = None
Copy link
Member

Choose a reason for hiding this comment

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

why try catch on imports

Comment on lines +24 to +25
if NetworkTableInstance is None:
print("ERR ntcore not installed", flush=True); return
Copy link
Member

Choose a reason for hiding this comment

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

just throw an exeption on init if not installed

Comment on lines +201 to +224
if cmd == "SET_SERVER":
host, port = rest[0].split()
pub.set_server(host, int(port)); print("OK", flush=True)
elif cmd == "LOAD_CSV":
path = rest[0]
if (path.startswith('"') and path.endswith('"')) or (path.startswith("'") and path.endswith("'")):
path = path[1:-1]
pub.load_csv(path) # prints OK/ERR
elif cmd == "SEEK":
pub.seek(float(rest[0])); print("OK", flush=True)
elif cmd == "PLAY":
pub.play(); print("OK", flush=True)
elif cmd == "PAUSE":
pub.pause(); print("OK", flush=True)
elif cmd == "STOP":
pub.stop(); print("OK", flush=True)
elif cmd == "PUBLISH_ON":
pub.set_publish(True); print("OK", flush=True)
elif cmd == "PUBLISH_OFF":
pub.set_publish(False); print("OK", flush=True)
elif cmd == "QUIT":
pub.exit = True; print("BYE", flush=True); break
else:
print("ERR", flush=True)
Copy link
Member

Choose a reason for hiding this comment

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

What am i seeing?! use a switch or dict with an enum, similar to the robot state machine. or you can simply use a dict with string to function

Copy link
Member

Choose a reason for hiding this comment

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

would appreciate build instructions

Comment on lines +53 to +61
def set_server(self, host: str, port: int): return self._send(f"SET_SERVER {host} {port}")
def load_csv(self, path: str): return self._send(f"LOAD_CSV {path}")
def seek(self, t: float): return self._send(f"SEEK {t}")
def play(self): return self._send("PLAY")
def pause(self): return self._send("PAUSE")
def stop(self): return self._send("STOP")
def pub_on(self): return self._send("PUBLISH_ON")
def pub_off(self): return self._send("PUBLISH_OFF")
def quit(self):
Copy link
Member

Choose a reason for hiding this comment

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

this is a repetitive and hard to read way to write this, use partial methods to clean this up

Copy link
Member

Choose a reason for hiding this comment

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

your logo is invisible in dark mode

@AsafMeizner
Copy link
Member

Create a new release on github with the updated executable

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.

2 participants