Skip to content
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

[Feature] Image Diff Preview #748

Merged
merged 34 commits into from Oct 12, 2021
Merged

Conversation

Stengo
Copy link
Contributor

@Stengo Stengo commented Feb 21, 2021

Partially implements #640

This PR introduces a fairly basic image diffing cell. For now it only supports the usual 'swipe' type comparison, but more options could easily be added in the future.

I AGREE TO THE GITUP CONTRIBUTOR LICENSE AGREEMENT

This view will be used very similarly to GIDiffView, as both are
variably sized diffing cell contents.
Again, similarly to the approach used for GIDiffView, we are attaching
this content view to the diff data, so it can later be integrated into
a cell.
Once again, this is mimicking GIDiffView and the associated
GITextDiffCellView. For now we merely use the previously introduced
imageDiffView as a way to identify whether an image cell should be
shown.
This will later be necessary to request enough height to display images
at their full resolution (or a fraction of that depending on width
constraints).
Following the example of GIDiffView, we attach (and detach) the content
view to a designated cell.
To be able to export blobs of images later on, we'll need access to
the repository. This commit lays the necessary foundation for that step.
Adds another necessary resource to be able to display images later on.
This preliminary solution only covers showing the absolute newest
version of the file and is therefore usually wrong.
It does however establish the connection between delta, repository and
the final image.
This code is very similar to the viewDeltasInDiffTool utility method, as
we are fundamentally doing the same thing (exporting the image file
referenced by the SHA1 value and then displaying it).
The most noteworthy detail is that we are defaulting back to the
canonical path for untracked files.
This works analogous to the previously added updateNewImage method.
The major difference is that we are looking at the oldFile and there
is no fallback if it does not exist.
The goal is to make the GIImageDiffView a slide style view image
comparison tool, in which the two images are displayed in the same
area, but hidden or revealed by sliding a divider across them.
This method allows us to calculate how big this combined area needs
to be.
This method is designed to work within a given frame and will return
a rectangle that will provide the correct dimensions to display
both the old and new image in a shared area.
Currently this will overlay the old image over the new one. However,
this is only setting up for the ability to partially hide both images.
Instead of asking for a somewhat random, large height to fit the image,
we can now ask for exactly the required height.
To do this, we try to fit the image at full resolution, but scale down
according to width constraints if necessary.
This property will be used to figure out how much to show of the old and
new image. A value of 0 will represent showing just the new image, while
a value of 1 will mean only the old image is shown.
Everything in between will move the divider in between accordingly.
By using masking layers, we can easily hide parts of the old and new
images. Originally, we considered drawing the images directly, but the
draw calls turned out to be very slow. This mask layer solution seems to
be about four times as fast.
By adding gesture recognizers to the view, we can use the entire image
area as a large input field for the user to select the split between
the old and new image.
Whenever we change the percentage value we need to trigger a redraw.
By default, changing a CALayer's frame causes it to animate to the new
position. This prevents that behavior, as we need the change to happen
as fast as possible.
This adds the commonly used checkerboard transparency indicator.
The pattern had already been used by the GIMapViewController, so no new
assets were necessary.
In order to reliably obey dark mode, it seems to be necessary to
reassign the colors constantly.
We tried to handle it via viewDidChangeEffectiveAppearance, but the
behavior was unreliable.
Since we couldn't notice any performance downsides to reassigning the
colors with every draw call, we went with this solution.
Another common design element in slide style image comparison tools are
red and green borders to separate old and new images.
This is a simple implementation of that, which once again reassigns
the colors on every draw call to handle dark mode properly.
The final common design element of slider style image comparison tools.
To further indicate where the old and new images meet, we add a thin
line right between them.
@lucasderraugh
Copy link
Collaborator

I haven't tried it out but it certainly looks cool. Thanks for working on this!

@gelosi
Copy link

gelosi commented Feb 25, 2021

wow! would be really cool to have it in master 🖤

@lucasderraugh
Copy link
Collaborator

Tried the branch out and it seems pretty solid. I need to make some other changes for the 1.2.1 release before we'd but this in. This might land in 1.2.2 or 1.3 if I think it warrants the minor bump.

@soberman
Copy link

soberman commented May 3, 2021

Would love to see it in the app!

@MrSkwiggs
Copy link

This is 🔥 . WANT

Copy link
Collaborator

@lucasderraugh lucasderraugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I can permit this at the moment as one of the main things we try to maintain with this app is performance. I'll test this out with large image assets today, but ultimately I think in its current implementation it would be a bit too slow. I'll try to make some suggestions as to what to change so we can get this feature in though.

An alternative would be to disable this by default and allow an opt in in preferences, but I feel that's more of a crutch than anything.

@@ -332,7 +340,13 @@ - (void)_reloadDeltas {
GCDiffPatch* patch = [self.repository makePatchForDiffDelta:delta isBinary:&isBinary error:&error];
if (patch) {
XLOG_DEBUG_CHECK(!isBinary || patch.empty);
if (patch.empty) {

BOOL isImage = [[NSImage alloc] initWithContentsOfFile:[self.repository absolutePathForFile:delta.canonicalPath]] != nil;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a lighter weight approach for determining if a path may be an image. If we have a lot of images and large images then we're going to load them all into memory up front and that could be very slow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point!
I switched it over to using UTIs, which should be very fast and still plenty reliable 🙂 (c4f52a4)

I'm not super familiar with memory management bridging for core foundation entities, though, so that part could definitely use a thorough look 😅 (the same goes for my other fix).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that should mostly be right, but to verify NSImageView can load it we probably should use NSImage.imageTypes to get the array of valid UTIs and then check conformance against those to see if we match any. I think kUTTypeImage type might be too generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorted! bae1fc2

} else {
newPath = [self.repository absolutePathForFile:_delta.canonicalPath];
}
_currentImageView.image = [[NSImage alloc] initWithContentsOfFile:newPath];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit concerned about loading in the file this way in cases where the asset is rather large. For most cases we're probably not dealing with giant assets, but if you do this is going to grind to a halt. We should probably generate a thumbnail of a max size so we cap the load time. Another optimization we should probably have is to separate out the image loading and size calculations. The table view needs to know size (because we don't use automatic height adjustments yet, unfortunately) and the size calculation should be as fast as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, limiting the size in memory definitely sounds advisable!
I took care of that in 991f889. For now I simply used a somewhat arbitrary maximum image dimension of 4000 pixels, because it was easy to do and seemed like a reasonable limit.
If you think we should change this value or even make it scale dynamically by screen size or something like that let me know 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With regards to separating out image loading and size calculations:
Does that mean you would want to make the cells a static height?

Right now we load the new images on setDelta and then query their dimensions during the sizing calculations to make sure they aren't scaled improperly and the aspect ratios make sense.
So the only way I could think of to avoid this reliance on in-memory images would be to set an arbitrary height and then cram the images into that space whenever they are ready 🤔

@richardtop
Copy link

Looking forward to this feature! Looks great!

The previous approach of initializing an NSImage to figure out whether a
file is an image was questionable when taking large binaries into account.
This new solution uses the file ending to determine the uniform type
identifier of a file, which can be used to check for image files.
To ensure fast performance and a reasonable memory impact even for very
large image files, we now create thumbnails of a limited size.
@Stengo
Copy link
Contributor Author

Stengo commented Sep 20, 2021

Hey @lucasderraugh!
Is there anything else I should be doing to help this PR along?
With the UTI identification in place (c4f52a4 / bae1fc2) and the thumbnails being generated (991f889) only the separation of image loading and size calculation would be left, but as mentioned in #748 (comment) I'm not sure how to go about that 🤔

@lucasderraugh
Copy link
Collaborator

Hey @lucasderraugh!
Is there anything else I should be doing to help this PR along?
With the UTI identification in place (c4f52a4 / bae1fc2) and the thumbnails being generated (991f889) only the separation of image loading and size calculation would be left, but as mentioned in #748 (comment) I'm not sure how to go about that 🤔

Perhaps this StackOverflow post would answer this for you: https://stackoverflow.com/questions/1551300/get-size-of-image-without-loading-in-to-memory

I wish we didn't require size calculation up front, but that's a bigger API transition that is a bit beyond scope for this PR at least. If you can test with committing 30 or so large different images and see what the performance is like that would be great. I'll gladly test it out when the size logic is in.

To ensure maximum performance we now offload loading the image into
memory to a separate thread.
As we still need the image size to lay out cells properly, we query
just this information before starting to load the full image.
@Stengo
Copy link
Contributor Author

Stengo commented Sep 26, 2021

Hello again @lucasderraugh!
I separated the image loading from the cell sizing as you suggested in febb995.
It essentially offloads the loading onto a different thread, but queries the file for it's size information right before doing so. It keeps the UI much more responsive for very large image files, but introduces a moment where a blank cell is shown. To mitigate that I introduced a spinner to indicate that things are still being processes 🙂

Very large, compressed RAW camera files (like the ARW files created by Sony mirrorless cameras) still cause slight hiccups, but those are also present in Finder, so I doubt there's a way to work around them. Very large uncompressed RAW files (like big TIFF images) don't present this behavior.

I didn't experience any performance issues when dealing with a large number of high quality images (~40) beyond what the current live version of GitUp experiences (e.g. staging all files takes quite a few seconds).

Let me know what you think 🙂

@lucasderraugh
Copy link
Collaborator

@Stengo Fantastic. I'll take a look when I get home to a laptop. We'll try to bundle up some changes for Monterey release.

Copy link
Collaborator

@lucasderraugh lucasderraugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks amazing and performant! One last question before we can get this in for me.

GitUpKit/Interface/GIImageDiffView.m Show resolved Hide resolved
@lucasderraugh lucasderraugh merged commit 90a5943 into git-up:master Oct 12, 2021
@Stengo Stengo deleted the image-diffing branch October 21, 2021 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants