-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add custom font support by detecting fonts used in a composition and submitting them as job attachments #91
base: feature_dockable_submitter
Are you sure you want to change the base?
Conversation
…n and submitting them as job attachments
Quality Gate failedFailed conditions See analysis details on SonarQube Cloud Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE |
""" | ||
Install provided font to the worker machine | ||
|
||
:param src_path: path of font that needs to be installed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: missing :returns:
Same for some of the functions below
"script": { | ||
"actions": { | ||
"onEnter": { | ||
"command": "powershell", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q: is there a reason we cannot use call python font_manager.py --install
directly?
} | ||
// Else use the family name for the temp file that will be created | ||
else { | ||
if (dcUtil.getAEVersion() >= 24){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you help me understand why do we check layer instanceOf TextLayer before checking the version first?
If I understand right, line 159 font.font, this font is defined inside above if block so when the code runs this line, the font is not defined yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q: which version of AE we have tested the font solution?
// Else use the family name for the temp file that will be created | ||
else { | ||
if (dcUtil.getAEVersion() >= 24){ | ||
var fontName = font.font + ".otf"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we assume all fonts will be .otf extension? Will this cause a problem if the font is ttf or other extensions?
if (fontsInComp.length > 0){ | ||
// formatting collected fonts | ||
var fontReferences = generateFontReferences(fontsInComp); | ||
for (var j = 0; j < fontReferences.length; j++){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could we just use i instead of j since there is no i used before?
return { | ||
"invertObject": invertObject, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is duplicate to the next line.
else: | ||
# Check if the Fonts folder exists, create it if it doesn't | ||
if not os.path.exists(FONT_LOCATION_USER): | ||
print("Creating User Fonts folder: %s" % FONT_LOCATION_USER, flush=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed you were using print f-string other places. Could we make them consistent?
# Load the font in the current session, remove font when loading fails | ||
if not gdi32.AddFontResourceW(dst_path): | ||
os.remove(dst_path) | ||
raise WindowsError('AddFontResource failed to load "%s"' % src_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment to use fstring for consistency.
filename = os.path.basename(dst_path) | ||
fontname = os.path.splitext(filename)[0] | ||
|
||
# Try to get the font's real name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we have already had the real name from the submitter side? Could you help me understand this?
Could we have the following env tested? If you cannot test any one in above list, please help us understand the reason. Thanks. |
|
||
# Creates registry if it doesn't exist, opens when it does exist | ||
with winreg.CreateKeyEx(registry_scope, FONTS_REG_PATH, 0, access= winreg.KEY_SET_VALUE) as key: | ||
winreg.SetValueEx(key, fontname, 0, winreg.REG_SZ, filename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recall a prior conversation where we did not want to allow registry keys to be modifiable due to security risks. Has this concern been resolved? @eherozhao
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question! This looks fine to me, since this will be user-specific registry instead of admin registry.
@@ -144,12 +145,69 @@ function findJobAttachments(rootComp) { | |||
} | |||
} | |||
} | |||
if (layer instanceof TextLayer){ | |||
var font = layer.text.sourceText.value; | |||
var fontLocation = font.fontLocation; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naive question, I saw the popup handling we have in the previous if block for missing footage. In a similar vein, can there be case where there are missing fonts in the font location? Asking because I've sometimes loaded up AfterEffects and the application notifies me that some fonts are missing, so a similar warning might be warranted here before they end up submitting a job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I just pulled this PR and ran the build script python jsxbundler.py --source src/OpenAESubmitter.jsx --destination dist/DeadlineCloudSubmitter.jsx
and there was a minor space difference in this generated file on Line 728 and Line 731. Could you please re-run this python command and push up the updated format change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL:DR I tried submitting jobs using fonts from my local machine and fonts supported by Adobe and neither fonts were found and the job did not render with the fonts as expected. Some guidance on how to test this PR would be appreciated.
Long explanation:
I tried submitting an AE 25.1 job from my Mac laptop with a custom font called 8 Heavy from Adobe Creative Cloud found here: https://fonts.adobe.com/fonts/eight that I added directly from Creative Cloud.
Then I added that font to my assets and then submitted a job. Under Job Attachements, I see the following input file attached called /private/var/folders/r_/m1d3rb6s2nb559nj4hd77ptm0000gq/T/TemporaryItems/tempFonts/Users/viknith/Library/Application Support/Adobe/CoreSync/plugins/livetype/.w/.52471.otf
I submitted the job, and it succeed, but the rendered output did not use the font that I added to my job. Seems like this approach of adding a font is not covered.
Here were the logs from the font step:
2025/01/09 11:43:34-08:00
2025/01/09 11:43:34-08:00 ==============================================
2025/01/09 11:43:34-08:00 --------- Entering Environment: Install Fonts to Worker
2025/01/09 11:43:34-08:00 ==============================================
2025/01/09 11:43:34-08:00 ----------------------------------------------
2025/01/09 11:43:34-08:00 Phase: Setup
2025/01/09 11:43:34-08:00 ----------------------------------------------
2025/01/09 11:43:34-08:00 ----------------------------------------------
2025/01/09 11:43:34-08:00 Phase: Running action
2025/01/09 11:43:34-08:00 ----------------------------------------------
2025/01/09 11:43:34-08:00 Running command C:\Windows\System32\WindowsPowerShell\v1.0\powershell.EXE -File C:\ProgramData\Amazon\OpenJD\session-2d81d4ce86804bd6b571854e4e8dba72vm8fx4y9\assetroot-3751af5bd81011e6d21d\private\var\folders\r_\m1d3rb6s2nb559nj4hd77ptm0000gq\T\TemporaryItems\DeadlineCloudAESubmission\scripts/installfonts.ps1 C:\ProgramData\Amazon\OpenJD\session-2d81d4ce86804bd6b571854e4e8dba72vm8fx4y9\assetroot-3751af5bd81011e6d21d\private\var\folders\r_\m1d3rb6s2nb559nj4hd77ptm0000gq\T\TemporaryItems\DeadlineCloudAESubmission\scripts C:\ProgramData\Amazon\OpenJD\session-2d81d4ce86804bd6b571854e4e8dba72vm8fx4y9
2025/01/09 11:43:34-08:00 Command started as pid: 2380
2025/01/09 11:43:34-08:00 Output:
2025/01/09 11:43:34-08:00 Running python script at C:\ProgramData\Amazon\OpenJD\session-2d81d4ce86804bd6b571854e4e8dba72vm8fx4y9\assetroot-3751af5bd81011e6d21d\private\var\folders\r_\m1d3rb6s2nb559nj4hd77ptm0000gq\T\TemporaryItems\DeadlineCloudAESubmission\scripts/font_manager.py
2025/01/09 11:43:35-08:00 Running font script job: install
2025/01/09 11:43:35-08:00 Warning: couldn't recursively find tempFonts in subfolder: assetroot-3751af5bd81011e6d21d
2025/01/09 11:43:35-08:00 No custom fonts found, continuing task...
2025/01/09 11:43:35-08:00 Process pid 2380 exited with code: 0 (unsigned) / 0x0 (hex)
No newer logs at this moment.
There was also log saying 2025/01/09 11:44:04-08:00 WARNING:After Effects warning: Project has missing fonts.
I thought the problem might be due to the fact that I pulled in a custom font from Adobe, so then I decided to use a font that was locally available on my laptop called Amazon Ember Thin. The job attachment path was /private/var/folders/r_/m1d3rb6s2nb559nj4hd77ptm0000gq/T/TemporaryItems/tempFonts/Library/Fonts/AmazonEmber_Th.ttf
, but it resulted in the same issue with the same error messages.
Did I misconfigure something here? If so, where should the fonts be located so that I know that they will be pulled correctly? Some clearer notes on how to test this would be much appreciated!
What was the problem/requirement? (What/Why)
After Effects requires all of a project's needed fonts to be installed system-wide onto the machine, or into the profile of the user running the software at render time. There is no font embedding available, nor does After Effects do any path searching for fonts.
What was the solution? (How)
Pull request #46 includes font files as job attachments in the bundle. We integrated it along with some fixes into the dockable submitter.
What is the impact of this change?
Users can use any custom fonts they'd like in their comp, and all chosen fonts will render correctly on the farm.
How was this change tested?
Testing was performed with SMF hosts running After Effects aerender to verify correct output of a project containing custom fonts. The farm render output was compared to local renders from a machine with those custom fonts installed, and the farm output is as expected.
Was this change documented?
Yes, there are changes to the project README.md describing the feature.
Is this a breaking change?
No.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.