-
Notifications
You must be signed in to change notification settings - Fork 286
Prevent MP4 export with invalid resolution #1833
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
Prevent MP4 export with invalid resolution #1833
Conversation
7187494
to
113f92f
Compare
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.
The code looks good. I've added a fix for the layout issue. My main concern is that there isn't a similar warning in the camera properties dialog. By the time users get to this and realize their mistake, they have probably already finished their entire animation. Adjusting the export resolution will allow the export to succeed, but it will also scale the animation and potentially cause blurring. In many cases, adjusting the camera layer size is preferable to adjusting the export size. I'm not sure what would be the best way to convey that information to the user though.
<item> | ||
<widget class="QLabel" name="unevenWidthLabel"> | ||
<property name="toolTip"> | ||
<string>The MP4 format does not support odd width. Please specify an even width or use a different file format.</string> |
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.
Technically MP4 is a container format and does support odd width/height, it is the specific codec we choose to use for exporting with the MP4 container that does not support this. While I don't think users need to know the technical details like this, I think maybe "MP4 exporter" instead of "MP4 format" may be better. It could avoid confusion in the future if we add more options to MP4 export, which could potentially allow videos with an odd width/height to work.
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.
As long as Pencil2D doesn’t allow the selection of alternative codecs, I don’t think the wording matters that much. We can worry about that when we do add such options. Until then, whatever you prefer is fine by me.
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.
Thanks for fixing the layout issue, I didn’t even realise Qt had an option like that!
My main concern is that there isn't a similar warning in the camera properties dialog. By the time users get to this and realize their mistake, they have probably already finished their entire animation. Adjusting the export resolution will allow the export to succeed, but it will also scale the animation and potentially cause blurring. In many cases, adjusting the camera layer size is preferable to adjusting the export size. I'm not sure what would be the best way to convey that information to the user though.
I don’t know what to say to that except I second every word of it, including that last sentence.
<item> | ||
<widget class="QLabel" name="unevenWidthLabel"> | ||
<property name="toolTip"> | ||
<string>The MP4 format does not support odd width. Please specify an even width or use a different file format.</string> |
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.
As long as Pencil2D doesn’t allow the selection of alternative codecs, I don’t think the wording matters that much. We can worry about that when we do add such options. Until then, whatever you prefer is fine by me.
Confirmed the issue is resolved. |
Fixes #1491. Has some small layout jumps when the warning is shown or hidden that I’m not quite sure how to properly deal with, but that’s still much better than the confusing wall of text that we have now.