-
Notifications
You must be signed in to change notification settings - Fork 28
draft: fix: Ensure filenames are encoded in UTF-8. #130
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
Conversation
df76f3d to
223ad96
Compare
| """ | ||
| scene_file = data.get("scene_file", "") | ||
|
|
||
| if locale.getpreferredencoding().lower() not in ("utf8", "utf-8"): |
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 we always encode and decode and avoid the branch here?
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 was considering not modifying filenames for UTF-8 default encoding file systems, as it usually wouldn't change anything. Is there any reason to do it other than avoiding the branch?
But I do see your point that as most of our rendering is in Windows, we could always encode/decode. Rev 2 coming up.
We can override merge even with this check failing. We should plan on refactoring all the tests in this folder so that we re-use the code amongst the tests instead of rewriting. But it is out of scope for this PR. |
223ad96 to
56506c7
Compare
Signed-off-by: Karthik Bekal Pattathana <[email protected]>
56506c7 to
8bbe53f
Compare
|
|
|
||
| def _convert_filename_to_utf8_encoding(self, scene_file: str) -> str: | ||
| try: | ||
| return scene_file.encode(locale.getpreferredencoding()).decode("utf-8") |
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 doesn't look right to me, because the Python 3 str type is already unicode. If this fixes something, it means that there was an earlier processing step that was wrong. We should track down that earlier step instead of patching it after it's already wrong.
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.
because the Python 3 str type is already unicode
Yup, that tripped me as well when I was trying to root cause this issue. Turns out python uses Unicode but it does not use UTF-8 encoding by default. The default encoding is platform dependent and in Windows it is CP-1252. This is what I found out while investigating this issue in the official docs. (Its for the open() library function but I think it uses the same philosophy here)
We should track down that earlier step instead of patching it after it's already wrong.
I traced the issue back to adaptor.py while investigating, which showed the problem even before Cinema 4D started running.
...
2024/12/18 09:52:41-06:00 ADAPTOR_OUTPUT: INFO: Loading 'init_data' schema from C:\Program Files\Python312\Lib\site-packages\deadline\cinema4d_adaptor\Cinema4DAdaptor\schemas\init_data.schema.json
2024/12/18 09:52:41-06:00 ADAPTOR_OUTPUT: INFO: Loading 'run_data' schema from C:\Program Files\Python312\Lib\site-packages\deadline\cinema4d_adaptor\Cinema4DAdaptor\schemas\run_data.schema.json
2024/12/18 09:52:42-06:00 ADAPTOR_OUTPUT: INFO: In Adaptor: Scene file is C:\ProgramData\Amazon\OpenJD\session-xxx\assetroot-yyyy\scene\redshift_textured_β.c4d
2024/12/18 09:52:42-06:00 ADAPTOR_OUTPUT: INFO: In adaptor: Scene file is file? False
...
Patching it in the adaptor unfortunately causes the adaptor to freeze while running. It does not happen often but only at times and I tried root causing the issue but couldn't find it. I patched it in handler as it was the most straightforward fix and quick fix to unblock customers.
2024/12/18 10:14:37-06:00 ADAPTOR_OUTPUT: INFO: In Adaptor: Scene file is C:\ProgramData\Amazon\OpenJD\session-xxx\assetroot-yyyy\scene\redshift_textured-_₿_ę_ñ_β_Б_ت.c4d
2024/12/18 10:14:37-06:00 ADAPTOR_OUTPUT: INFO: In adaptor: Scene file is file? False
...
It was stuck and I had to fail the job manually.
...
2024/12/18 10:18:44-06:00 Canceling subprocess 3868 via notify then terminate method at 2024-12-18T16:15:44Z.
Initially, this seemed to be a Cinema 4D-specific issue, not occurring in other DCCs like Unreal and Keyshot on Windows. Hence, I ruled out checking the dependencies of Cinema 4D.
However, while reviewing logs yesterday evening, I discovered that Keyshot jobs, despite being successful, had a similar error in the adaptor logs. This suggests the issue could affect all DCCs that run in Windows.
2024/12/17 12:49:39-06:00 ADAPTOR_OUTPUT: STDOUT: self._perform_action(action)
2024/12/17 12:49:39-06:00 ADAPTOR_OUTPUT: STDOUT: File "C:\Users\job-user\.conda\envs\Lib\openjd\adaptor_runtime_client\base_client_interface.py", line 233, in _perform_action
2024/12/17 12:49:39-06:00 ADAPTOR_OUTPUT: STDOUT: action_func(a.args)
2024/12/17 12:49:39-06:00 ADAPTOR_OUTPUT: STDOUT: File "C:\Users\job-user\.conda\envs\hashname_xxx\Lib\deadline\keyshot_adaptor\KeyShotClient\keyshot_handler.py", line 110, in set_scene_file
2024/12/17 12:49:39-06:00 ADAPTOR_OUTPUT: STDOUT: raise FileNotFoundError(f"The scene file '{scene_file}' does not exist")
2024/12/17 12:49:39-06:00 ADAPTOR_OUTPUT: STDOUT: FileNotFoundError: The scene file 'C:\ProgramData\Amazon\OpenJD\session-xxx\assetroot-yyy\AppData\Local\Temp\tmpd3n517x9\unpack\Keyframe-Robot-Scene Sharing Version ñot Angry.bip' does not exist
...
Since Cinema 4D customers were currently impacted and are blocked for working, I prioritized fixing it for Cinema 4D first. After that, I plan to investigate the issue further.
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.
Have you looked into creating a minimal reproducer example as a small job template? That could help isolate the issue from the environment and provide faster iteration to debug it.
|
|
||
| import os | ||
| from typing import Any, Callable, Dict | ||
| import locale |
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: can you run hatch fmt on this? I would expect the imports to be in a different order.
|
Closing this PR in favor for OpenJobDescription/openjd-adaptor-runtime-for-python#171 |


What was the problem/requirement? (What/Why)
Cinema 4D submissions fail to run if filenames contain non-ASCII values. For example, a scene file named
TestSceneñ.c4dwould not render.What was the solution? (How)
In our handler, we check if a file exists before loading it into Cinema 4D.
However, Windows systems use cp1252 encoding by default, not UTF8. This causes incorrect filename encoding and failed file existence checks, preventing renders. Encoding to UTF8 fixes most renders.
While renders now work for characters like
They still fail for emojis and some mathematical operators. Cinema 4D's load document function returns None without details on why it fails.
Regardless, this is a partial fix that should help most of our customers.
I will also investigate further if its possible to have general solution that can fix it for all DCCs that have this issue.
What is the impact of this change?
Customers with scene files containing non-ASCII characters in filenames can now use them. This excludes filenames with emojis or mathematical symbols.
How was this change tested?
Yes
Did you run the "Job Bundle Output Tests"? If not, why not? If so, paste the test results here.
Yes, I also added a new test to check for this condition specifically.
Was this change documented?
No
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.