3
\$\begingroup\$

This is my first 'proper' script since learning python and I'd love it if I could get some feedback on things I've done wrong / things I can improve on.

The script takes clipboard data from after effects, containing information on position, rotation and scale.
It then gets out the numbers, converts them to keyframe data for The Foundry's Nuke and then writes that data to a text file ready for Nuke to import.

As this is my first script I'm sure there is plenty I can streamline / refine.

from itertools import chain, zip_longest from PySide import QtGui import os import sys # Fetch data from clipboard app = QtGui.QApplication(sys.argv) cb = QtGui.QApplication.clipboard() # Needs changing to user input / fetch from cb srcWidth = 1920 srcHeight = 1080 # create temporary file for clipboard data with open('AEClipboardTemp', 'w+') as tempFile: tempFile.write(cb.text()) rotationData = [] scaleData = [] positionData = [] """ Data Extraction Functions """ def extractRotationData(): keepDataR = False with open('AEClipboardTemp', 'r') as aeRead: for line in aeRead: if line.strip() == "Frame\tdegrees": print("rotation data extracted") keepDataR = True elif line.strip() == "": keepDataR = False elif keepDataR: rotationData.append(line) def extractScaleData(): keepDataS = False with open('AEClipboardTemp', 'r') as aeRead: for line in aeRead: if line.strip() == "Frame\tX percent\tY percent\tZ percent" or \ line.strip() == "Frame\tX percent\tY percent": print("scale data extracted") keepDataS = True elif line.strip() == "": keepDataS = False elif keepDataS: scaleData.append(line) def extractPositionData(): keepDataP = False with open('AEClipboardTemp', 'r') as aeRead: for line in aeRead: if line.strip() == "Frame\tX pixels\tY pixels\tZ pixels" or \ line.strip() == "Frame\tX pixels\tY pixels": print("transform data extracted") keepDataP = True elif line.strip() == "": keepDataP = False elif keepDataP: positionData.append(line) # Column count for nuke keyframe file nukeCol = 0 # Detect if information exists with open('AEClipboardTemp', 'r') as aeRead: if "degrees" in aeRead.read().strip().split(): print("rotation data present") nukeCol += 1 extractRotationData() with open('AEClipboardTemp', 'r') as aeRead: if "percent" in aeRead.read().strip().split(): print("scale data present") nukeCol += 2 extractScaleData() with open('AEClipboardTemp', 'r') as aeRead: if "pixels" in aeRead.read().strip().split(): print("transform data present") nukeCol += 2 extractPositionData() print("Nuke columns = ", nukeCol) """ Remove redundant data """ AERData = [] AESData = [] AEPData = [] # remove frame number for line in rotationData: org = line.strip().split() org = org[1::] AERData.append(org) # remove frame number and z coord for line in scaleData: org = line.strip().split() with open('AEClipboardTemp', 'r') as aeRead: for line in aeRead: if line.strip() == "Frame\tX percent\tY percent\tZ percent": org = org[1:len(org)-1:] AESData.append(org) elif line.strip() == "Frame\tX percent\tY percent": org = org[1::] AESData.append(org) # remove frame number and z coord for line in positionData: org = line.strip().split() with open('AEClipboardTemp', 'r') as aeRead: for line in aeRead: if line.strip() == "Frame\tX pixels\tY pixels\tZ pixels": org = org[1:len(org)-1:] AEPData.append(org) elif line.strip() == "Frame\tX pixels\tY pixels": org = org[1::] AEPData.append(org) """ Conversions from AE to Nuke data """ # Rotation Data Convert AERData = [(float(x)*(-1)) for line in AERData for x in line] AERData = [str(line) for line in AERData] nukeRData = [list(x) for x in zip(AERData)] # Scale Data Convert AESData = [(float(x)/100) for line in AESData for x in line] AESData = [str(line) for line in AESData] nukeSData = [list(x) for x in zip(AESData[0::2], AESData[1::2])] # Position Data Convert # X data AEPDataX = [] AEPDataX.append(list(coord[0::2] for coord in AEPData)) AEPDataX = [(float(x)-(srcWidth/2)) for item in AEPDataX for y in item for x in y] AEPDataX = [str(line) for line in AEPDataX] # Y data AEPDataY = [] AEPDataY.append(list(coord[1::2] for coord in AEPData)) AEPDataY = [((srcHeight-(float(x)))-(srcHeight/2)) for item in AEPDataY for y in item for x in y] AEPDataY = [str(line) for line in AEPDataY] # Combine X+Y data nukePData = [list(x) for x in zip(AEPDataX, AEPDataY)] # Combine position, rotation and scale data translationData = [filter(None, col) for col in zip_longest(nukeRData, nukePData, nukeSData)] """ Format data for nuke ASCII import """ tData = [] for i in translationData: for item in i: for x in item: tData.append(x) nCB = [tData[x:x+nukeCol] for x in range(0, len(tData), nukeCol)] nCBFormat = [] for line in nCB: nCBFormat.append(line) nCBFormat.append("\n") nCBChain = list(chain.from_iterable(nCBFormat)) nCBChain = " ".join(nCBChain) """ Nuke keyframe ASCII File creation functions """ def writeToTemp(): with open("nukeKeysTemp.txt", 'w') as keyFile: keyFile.write(str(nCBChain)) def nukeKeysCreate(): with open("nukeKeysOut_v01.txt", 'w') as writeOut: with open("nukeKeysTemp.txt", 'r') as wSFix: for line in wSFix: cleanLine = line.lstrip() writeOut.write(cleanLine) def nukeKeysTempDel(): os.remove("nukeKeysTemp.txt") def cleanUp(): try: os.remove("AEClipboardTemp") except OSError: pass writeToTemp() nukeKeysCreate() nukeKeysTempDel() cleanUp() 

A sample of the clipboard data from After Effects can be found at AE Keyframe Data Example - gist

And the associated output would be Nuke keyframe file

\$\endgroup\$
3
  • \$\begingroup\$Welcome to Code Review. One of our rules for on-topic question requires that you embed your code in the question itself. Be not afraid to provide lengthy codes, codereview has an extended limit of 64k characters per post.\$\endgroup\$CommentedJan 28, 2016 at 11:29
  • \$\begingroup\$Looking at your input file, there is only one Transform Rotation, Transform Scale, or Transform Position. Is it always the case or can there be multiple of them?\$\endgroup\$CommentedJan 28, 2016 at 12:12
  • \$\begingroup\$@MathiasEttinger There is a maximum of one of each. Example 1: Rotation, Scale and Position Example 2: Rotation and Position Example 3: Position\$\endgroup\$CommentedJan 28, 2016 at 12:22

2 Answers 2

1
\$\begingroup\$

Temporary files

There is an entire package dedicated to dealing with temporary files. You could improve parts of your code using it.

However, it seems mostly unnecessary. The two temporary files you use are:

  1. To be able to re-read again and again the same data: use a list of lines instead;
  2. To format a single string with newlines in it into a list of lines: do not join using '\n' beforehand.

Repeating yourself

At first sight, your code looks like:

  • Some initialization;
  • 3 functions that look alike;
  • 3 blocks of code that look alike;
  • 2 for-loops that look alike;
  • 2 blocks of code that look alike;
  • 2 blocks of code that look alike;
  • Some teardown logic.

This is a lot of things that look alike. You should try to remove the number of things that look alike to:

  1. Not bore potential readers (including yourself);
  2. Extract logical blocks that look alike¹ and allow for re-usability of your code;
  3. Reduce the burden of maintainability of your code.

For that, you will need to parametrize your functions to allow for the same kind of code to be run on different inputs/outputs.

For instance, the extractXXXData() could be rewritten:

def extractData(output_list, start_condition, data_type_name): keepData = False with open('AEClipboardTemp', 'r') as file: for line in file: line = line.strip() if start_condition(line): print(data_type_name, "data extracted") keepData = True elif line == "": keepData = False elif keepData: output_list.append(line) 

and used like:

nukeCol = 0 extractData(rotationData, lambda line: line == "Frame\tdegrees", "rotation") if rotationData: nukeCol += 1 extractData(scaleData, lambda line: line == "Frame\tX percent\tY percent\tZ percent" or line == "Frame\tX percent\tY percent", "scale") if scaleData: nukeCol += 2 extractData(positionData, lambda line: line == "Frame\tX pixels\tY pixels\tZ pixels" or line == "Frame\tX pixels\tY pixels", "transform") if positionData: nukeCol += 2 print("Nuke columns = ", nukeCol) 

a lot less code to manage.

Your repetitions also lead to having…

Most of the code at top-level

All your variables are global, and all your functions access them globaly. Again, you need to create functions that encapsulate similar behaviours and parametrize them to achieve the specific behaviour you need for a particular dataset.

Removing redundant data can be done with:

""" Remove redundant data """ def strip_Z_axis_and_Frame(data_in, data_out): for line in data_in: # line already stripped in extractData data_out.append(line.split()[1:3]) AERData = [] AESData = [] AEPData = [] strip_Z_axis_and_Frame(rotationData, AERData) strip_Z_axis_and_Frame(scaleData, AESData) strip_Z_axis_and_Frame(positionData, AEPData) 

Note how you don't even need to know the type of data beforehand here. The slice operator [1:3] will happily drop column 0, take column 1 and take column 2 if it exists. So it will work for rotations too.

Same to convert between AE and Nuke data:

""" Conversions from AE to Nuke data """ def convert_column(column_data, converting_function): return [str(converting_function(line)) for line in column_data] AERData = chain.from_iterable(AERData) AESData = chain.from_iterable(AESData) AESDataX = AESData[0::2] AESDataY = AESData[1::2] AEPData = chain.from_iterable(AEPData) AEPDataX = AEPData[0::2] AEPDataY = AEPData[1::2] AERData = convert_column(AERData, lambda x: float(x) * (-1)) AESDataX = convert_column(AESDataX, lambda x: float(x) / 100) AESDataY = convert_column(AESDataY, lambda x: float(x) / 100) AEPDataX = convert_column(AEPDataX, lambda x: float(x)-(srcWidth/2)) AEPDataY = convert_column(AEPDataY, lambda x: (srcHeight-(float(x)))-(srcHeight/2)) 

Do things one step at a time: extract columns and then convert the data in those columns.

Computing the final string is then only a matter of

""" Format data for nuke ASCII import """ nCB = zip_longest(AERData, AESDataX, AESDataY, AEPDataX, AEPDataY) nCBChain = [] for line in nCB: nCBChain.append(' '.join(filter(None, line))) nCBChain = '\n'.join(nCBChain) 

Note how I used filter instead of nukeCol to strip out empty columns.

Doing that, you can move most of your logic into functions. Most of what is left could be put into a main function that you would call as the (almost) only top-level instruction.

About pythonic constructs

You are aware of list-comprehensions, but somehow you mainly use for loops to append data to a list.

You are aware of iteration through a list of strings, but somehow you rely on files reading to retrieve the same kind of information several times.

You know how to strip data from a line, but somehow you need to use a file to store intermediate string instead of stripping lines in the list before joining them into a single string.

While I’m at it, let me introduce you to two other functions found in the itertools package:

  • dropwhile: will let you skip data at the beginning of an iterable;
  • takewhile: will let you skip data at the end of an iterable.

Combining both, you can simplify the way you search the transformations in your clipboard:

def extractData(lines_of_text, searched_text): searched_text = searched_text.lower() data_with_headers = takewhile(lambda line: line != '', dropwhile(lambda line: searched_text in line, lines_of_text)) return list(data_with_headers)[2:] # dropping headers 

This will require that you store your clipboard in a list of lines (all lowercase); and can be called like:

rotationData = extractData(lines_of_text, "rotation") scaleData = extractData(lines_of_text, "scale") positionData = extractData(lines_of_text, "transform") 

Miscellaneous

  • Random strings in the middle of the program are weird. It’s like dropping a random number in here. It does nothing to the execution flow, but it’s pure noise. Note that multi-line strings are not multi-line comments. If you want multi-line comments, use # at the beginning of multiple line.
  • Multi-line strings can, however, be found as docstrings, which your code lacks a lot.
  • Constants are better written using UPPER_SNAKE_CASE and kept at the beginning of the file.
  • Functions and variable names are better written using snake_case.
  • It is recommended to put imports from the standard library before other kind of imports.

Putting it all together

import os import sys from itertools import chain, zip_longest, dropwhile, takewhile from PySide import QtGui # Needs changing to user input / fetch from cb WIDTH = 1920 HEIGHT = 1080 def get_clipboard_lines(): """Retrieve the content of the clipboard. Lowercase and strip it to ease further computation. Create the QApplication beforehand to be able to get that content. """ app = QtGui.QApplication(sys.argv) return [line.strip().lower() for line in QtGui.QApplication.clipboard().splitlines()] def create_nuke(filename, data): """Output computed data to the specified file""" with open(filename, 'w') as output: output.writelines(data) #### #Data Extraction Functions #### def extract_data(lines_of_text, searched_text): """Extract data for frames relevant to a certain type of transformation""" searched_text = searched_text.lower() data_with_headers = takewhile(lambda line: bool(line), dropwhile(lambda line: searched_text in line, lines_of_text)) return list(data_with_headers)[2:] # dropping headers def strip_frame(data): """Remove the frame number and Z axis from a transformation dataset. Also flatten the whole data into a single list. """ return list(chain.from_iterable(line.split()[1:3] for line in data)) def convert_column(column_data, converting_function): """Convert data from AE to Nuke using the supplied function""" return [str(converting_function(line)) for line in column_data] def main(lines_of_text, nuke_filename): """Process the content of AE clipboard contained in lines_of_text and output a Nuke file. """ # Column count for nuke keyframe file rotation = extract_data(lines_of_text, "rotation") scale = extract_data(lines_of_text, "scale") position = extract_data(lines_of_text, "transform") #### #Remove redundant data #### rotation = strip_frame(rotation) scale = strip_frame(scale) scale_X = scale[0::2] scale_Y = scale[1::2] position = strip_frame(position) position_X = position[0::2] position_Y = position[1::2] #### #Conversions from AE to Nuke data #### frames = zip_longest( convert_column(rotation, lambda x: float(x) * (-1)), convert_column(scale_X, lambda x: float(x) / 100), convert_column(scale_Y, lambda x: float(x) / 100), convert_column(position_X, lambda x: float(x)-(WIDTH/2)), convert_column(position_Y, lambda x: (HEIGHT-(float(x)))-(HEIGHT/2)) ) frame_lines = (' '.join(filter(None, line)) for line in frames) create_nuke(nuke_filename, frame_lines) # Fetch data from clipboard main(get_clipboard_lines(), "nukeKeysOut_v01.txt") 

Note that I have no idea about the kind of data returned from QtGui.QApplication.clipboard(); I assumed it is a single string. In case it is something else, you can still get it like you did, write it to a temporary file like you did, and return [line.strip().lower() for line in file]; or figure out a proper way to convert it to a list of lines.


¹Ok, enough of that

\$\endgroup\$
1
  • \$\begingroup\$Thank you VERY much for such a detailed breakdown! I'll be going through everything you've written to try and re-create you're final product.\$\endgroup\$CommentedJan 28, 2016 at 15:19
1
\$\begingroup\$

From a first look it looks quite good, for your first "real" script I'd say it's well done.

  • First thing, there should be a main function, by convention something like if __name__ == "__main__": main() and then put top-level statements into that function instead, c.f. this post.
  • Python names should be lower_case or UPPER_CASE, though since you use it consistently, it's still at least readable.
  • All the extract... functions should share some common code - at the moment there's a lot of duplication, same for many of the other blocks, basically everything that looks the same is probably better off by using a common function instead.
  • The repeated list comprehensions in the lower part aren't particularly performant.
  • Initialising to [] and then calling append is redundant.
  • nCBChain is already a string, no need for str.
\$\endgroup\$

    Start asking to get answers

    Find the answer to your question by asking.

    Ask question

    Explore related questions

    See similar questions with these tags.