-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Added progress callback when save_all is used #7435
Open
radarhere
wants to merge
19
commits into
python-pillow:main
Choose a base branch
from
radarhere:progress
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
6850465
Added progress callback when save_all is used
radarhere 3465a59
Pass back index of image in sequence, instead of filename
radarhere b5f3228
Restored filename in progress callback
radarhere fe7c9d3
Moved progress code into common private method
radarhere fae0653
Merge branch 'main' into progress
radarhere 19c2721
Merge branch 'main' into progress
radarhere 4a0a011
Merge branch 'main' into progress
radarhere 69680db
Merge branch 'main' into progress
radarhere 344c300
Merge branch 'main' into progress
radarhere 3c80ec0
Merge branch 'main' into progress
radarhere b7a5b59
Updated tests for os.path.realpath
radarhere 14560e1
Merge branch 'main' into progress
radarhere d52b3a2
Merge branch 'main' into progress
radarhere 682a9ae
Merge branch 'main' into progress
radarhere 2f27173
Merge branch 'main' into progress
radarhere 26e2a9f
Merge branch 'main' into progress
radarhere 8b4b7ce
Merge branch 'main' into progress
radarhere ebbdc6e
Merge branch 'main' into progress
radarhere fc5c096
Merge branch 'main' into progress
radarhere File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Say... if the frames are counted from
1
, why count the images from0
?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'm triggering these callbacks when frames are completed, not when they are started.
My intention is for
(0, 1, 7)
to communicate "We're in the first image. We've completed the first frame, there are seven frames in total".Then
(2, 7, 7)
communicates "We're in the third image. We've completed seven frames, there are seven frames in total, so we're done"If I started counting the frames from 0, the last value would be
(2, 6, 7)
. That looks less to me like save_all is finished.I recognise this could be a subject of debate.
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 question was “Why count the images from
0
?”I.e. why are the images numbered starting from
0
? You're numbering frames starting from1
– it would be consistent to use the same numbering scheme for images.(And yes, I know that due to how pointer arithmetic works, arrays in C are indexed starting from
0
, but that's an implementation detail and it's kind of ridiculous to drag it into higher-level languages just because; counting things is meant to be started from1
– that's what the number means in the first place.)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 trying to explain that my goal is not to number the frames from 1, but rather to clearly communicate when progress is completed, because the frame count will be compared with the total number of frames. The image count will not.
Python arrays are also indexed from 0.
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.
And yet counting from
0
would be inconsistent with the other counting used within same data set.Which makes no sense since Python is a high-level language (and there's some consistency issues caused by this arbitrary choice to imitate C array indexing, but that's neither here nor there).
Besides, the images aren't coming from a list as far as the user is concerned (well, there's
append_images
, but going by that logic you'd have to start from-1
since the first input image is not in this list in the first place… except that-1
is a valid index in Python, so it probably would beNone
instead).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've pushed a commit naming each piece of information within the returned data. It is now "image_index" and "completed_frames". Hopefully that clarifies that one is the Pythonic zero-based index of the image in progress, and the other is the number of frames that have been finished.