fix(ui): misc fixes #7252
Merged
fix(ui): misc fixes #7252
Conversation
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
Previously, we cleared the canvas progress image when the canvas had no active generations. This allowed for a brief flash of canvas state between the last progress image for a given generation, and when the output image for that generation rendered. Here's the sequence: - Progress images are received and rendered - Generation completes - no active canvas generations - Clear the progress image -> canvas layers visible unexpectedly, creating an awkward jarring change - Generation output image is rendered -> output image overlaid on canvas layers In 83538c4 I attempted to fix this by only clearing the progress image while we were not staging. This isn't quite right, though. We are often staging with no active generations - for example, you have a few images completed and are waiting to choose one. In this situation, if you cancel a pending generation, the logic to clear the progress image doesn't fire because it sees staging is in progress. What we really need is: - Staging area module clears the progress image once it has rendered an output image. - Progress image module clears the progress image when a generation is canceled or failed, in which case there will be no output image. To do this, we can add an event listener to the progress image module to listen for queue item status changes, and when we get a cancelation or failure, clear the progress image.
The root issue is the compositing cache. When we save the canvas to gallery, we need to first composite raster layers together and then upload the image. The compositor makes extensive use of caching to reduce the number of images created and improve performance. There are two "layers" of caching: 1. Caching the composite canvas element, which is used both for uploading the canvas and for generation mode analysis. 2. Caching the uploaded composite canvas element as an image. The combination of these caches allows for the various processes that require composite canvases to do minimal work. But this causes a problem in this situation, because the user expects a new image to be uploaded when they click save to gallery. For example, suppose we have already composited and uploaded the raster layer state for use in a generation. Then, we ask the compositor to save the canvas to gallery. The compositor sees that we are requesting an image for the current canvas state, and instead of recompositing and uploading the image again, it just returns the cached image. In this case, no image is uploaded and it the button does nothing. We need to be able to opt out of the caching at some level, for certain actions. A `forceUpload` arg is added to the compositor's high-level `getCompositeImageDTO` method to do this. When true, we ignore the uppermost caching layer (the uploaded image layer), but still use the lower caching layer (the canvas element layer). So we don't recompute the canvas element, but we do upload it as a new image to the server.
When filtering, we use a listener to trigger processing the image whenever a filter setting changes. For example, if the user changes from canny to depth, and auto-process is enabled, we re-process the layer with new filter settings. The filterer has a method to reset its ephemeral state. This includes the filter settings, so resetting the ephemeral state is expected to trigger processing of the filter. When we exit filtering, we reset the ephemeral state before resetting everything else, like the listeners. This can cause problem when we exit filtering. The sequence: - Start filtering a layer. - Auto-process the filter in response to starting the filter process. - Change the filter settings. - Auto-process the filter in response to the changed settings. - Apply the filter. - Exit filtering, first by resetting the ephemeral state. - Auto-process the filter in response to the reset settings.* - Finish exiting, including unsubscribing from listeners. *Whoops! That last auto-process has now borked the layer's rendering by processing a filter when we shouldn't be processing a filter. We need to first unsubscribe from listeners, so we don't react to that change to the filter settings and erroneously process the layer. Also, add a check to the `processImmediate` method to prevent processing if that method is accidentally called without first starting the filterer. The same issue could affect the segmenyanything module - same fixes are implemented there.
psychedelicious
requested review from
blessedcoolant,
maryhipp and
hipsterusername
as code owners
hipsterusername
approved these changes
Nov 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Fixes:
Related Issues / Discussions
Linked inline.
QA Instructions
Should fix the listed issues.
Merge Plan
Probably should do a bugfix release with these fixes.
Checklist