-
Notifications
You must be signed in to change notification settings - Fork 551
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
Fix passing of integer arguments in Python 3.10+ #4527
Conversation
self.topLeftHandle = QRectF(-cs/sx/2.0, -cs/sy/2.0, cs/sx, cs/sy) | ||
self.topRightHandle = QRectF(source_width - (cs/sx) + cs/sx/2.0, -cs/sy/2.0, cs/sx, cs/sy) | ||
self.bottomLeftHandle = QRectF(-cs/sx/2.0, source_height - (cs/sy) + cs/sy/2.0, cs/sx, cs/sy) | ||
self.bottomRightHandle = QRectF(source_width - (cs/sx) + cs/sx/2.0, source_height - (cs/sy) + cs/sy/2.0, cs/sx, cs/sy) | ||
self.topLeftHandle = QRectF( | ||
-csx / 2.0, -csy / 2.0, csx, csy) | ||
self.topRightHandle = QRectF( | ||
source_width - csx / 2.0, -csy / 2.0, csx, csy) | ||
self.bottomLeftHandle = QRectF( | ||
-csx / 2.0, source_height - csy / 2.0, csx, csy) | ||
self.bottomRightHandle = QRectF( | ||
source_width - csx / 2.0, source_height - csy / 2.0, csx, csy) |
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.
Here's a perfect example of that last point in my intro. I noticed early on that cs/sx
and cs/sy
were being computed dozens of times over and over again, so I added two variables csx
and csy
that held those values, respectively. Then I replaced every instance of cs/sx
with csx
, and the same for cs/sy
=> csy
.
Once I'd made those changes, it was obvious that source_width - (csx) + (csx / 2.0)
is the same thing as source_width - csx / 2.0
, so I made that substitution. And the same for a lot of other computations.
# There's one a bit farther down that uses
((x1*source_width+x2*source_width) / 2.0) - (cs/sx/2.0)
# That reduces to
((x1 + x2) * source_width - csx) / 2.0
# so that's what I replaced it with.
@jonoomph Once this is merged I think there should probably be a new OpenShot 2.6.2 release for Python 3.10 compatibility. Maybe, if I manage to get the JUCE update together quickly enough, including that as well. |
@ferdnyc This looks good to me, and it makes sense! However, it's a very difficult one to review (as a lot of subtle changes are in here). But assuming you have tested this one, and there are no obvious breakages, let's go ahead and merge away. We'll get more testing eyeballs after merging it. |
40d7a2b
to
c186bb6
Compare
@jonoomph Yeah, makes sense. I was wrong about the builds auto-enabling warnings, anyway, that'd require a launch script because doing it from inside Python code is too late. So, it takes a fair amount of effort to test this, unfortunately. Meanwhile, I've updated to Fedora 35 which includes Python 3.10, so I keep having to merge this branch into all of my others just so I can test them locally! I fixed a couple last things and removed the attempts to set |
Replacement for getTransformMode with less repetitious code
9c9c674
to
33cf68c
Compare
Hopefully this'll solve Python 3.10 fully. Time will tell... |
@ferdnyc Regression found: the "Tracker" effect is broken by this PR. When you drop the Tracker effect, and attempt to draw a rectangle in the video_widget, it fails to draw. I'm looking at this now, but some of the changes to video_widget are a bit tricky to see clearly. |
Looks like zooming into the video_widget is also broken by this PR. Oops. |
I'm leaning towards reverting this entire PR, as it's so complex, I'll need to go back over every single line of it, and possibly need to refactor portions of it. Unless @ferdnyc can see a quick fix, lol. |
Found the issues, will be merging a revert of just the tiny sections causing the bugs: #4583 |
Update: Downloadable packages
Hot off the project build servers, here for testing purposes (see below) are links to OpenShot packages built off this branch. They're stored in my personal Google Drive space. When you initially follow the link, you may get a spinny icon for a while until Google decides it can't preview the file and shows you a download button. You don't have to wait for that, though — there's already a download icon at the top of the page. Whenever/however you click Download, after that you'll likely get a warning that Google can't scan the file for viruses, you'll have to click past that to actually download the file.
Linux: OpenShot-v2.6.1-dev-daily-8590-aee3a44b-73c519f0-x86_64.AppImage
Mac: OpenShot-v2.6.1-dev-daily-8590-aee3a44b-73c519f0-x86_64.dmg
I'm not actually sure Windows users will be able to easily test this (they'd need to run
openshot-qt-cli.exe
to see console output, and even then I'm not sure the environment setting will work correctly), but here are links anyway...Win64: OpenShot-v2.6.1-dev-daily-8590-aee3a44b-73c519f0-x86_64.exe
Win32: OpenShot-v2.6.1-dev-daily-8590-aee3a44b-73c519f0-x86.exe
Oh, yes. Fixes #4358.
Original PR message
This is a PR I've put off submitting for too long already. Hopefully it's complete, but one of the things we really need to do is try to verify that.
The integer problem
Python 3.10 removes automatic conversion of floats to integers, when passing arguments to functions that require integer arguments. The Python developers had been warning of their plans to do that for quite some time, and we weren't really listening. But, now they've pulled that trigger, and as a result OpenShot (even 2.6.1) fails to run under Python 3.10.
That's because it turns out we unknowingly (ab)use integer promotion a lot, when calling PyQt5 functions for integer Qt classes like
QSize
,QPoint
,QRect
,QColor
, andQTimer
(among others). All of those require integer arguments, and under Python 3.10 throw an exception if you pass them floating-point arguments.The (developing) solution
This PR attempts to fix the problem, by wrapping all values being passed as integer arguments to functions that require them with
int()
. These places were discovered by reading the code, by searching the code, and by running OpenShot under Python 3.9 with the environment variablePYTHONWARNINGS=always
set.Setting that environment variable causes Python to display a message whenever a function that takes an integer argument is passed one that isn't an integer. (Which, under 3.9, does not yet throw an exception.) Fixing those warnings gets the code ready for Python 3.10. Hopefully I've found all of them, but the only way to be sure is to use every single piece of code so that it can be confirmed not to emit any warnings. There's no way I've managed to do that.
How YOU can help
This branch has been created in the project repo, so it will shortly result in packages being built for it. When those packages are available, running them under the included Python 3.x will automatically set
PYTHONWARNINGS=always
in the process environment, meaning OpenShot will output messages on the console if there are any remaining uses of integer promotion. (That means OpenShot must be launched from the command line, so that it has a terminal to output to. I don't believe the warnings will make it into our logs, but perhaps they will.) If any of those messages are displayed, PLEASE report them ASAP so that they can be fixed, because under Python 3.10 they will cause program crashes.Other fixes
This PR also fixes a few other issues:
origin_x
/origin_y
) would lead to exceptions, now it should be able to handle creating any properties it needs to.getTransformMode()
in the VideoWidget code is replaced with a rewrittencheckTransformMode()
that has much less repetitious code because it loops over the handle regions instead of checking each one in a hand-coded chain ofelseif
s.