Skip to content

Commit

Permalink
fix(ui): non-default filters can erase layer
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
psychedelicious committed Nov 4, 2024
1 parent c3cef2c commit e4e78c3
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,10 @@ export class CanvasEntityFilterer extends CanvasModuleBase {
* Processes the filter, updating the module's state and rendering the filtered image.
*/
processImmediate = async () => {
if (!this.$isFiltering.get()) {
this.log.warn('Cannot process filter when not initialized');
return;
}
const config = this.$filterConfig.get();
const filterData = IMAGE_FILTERS[config.type];

Expand Down Expand Up @@ -342,7 +346,6 @@ export class CanvasEntityFilterer extends CanvasModuleBase {
});

// Final cleanup and teardown, returning user to main canvas UI
this.resetEphemeralState();
this.teardown();
};

Expand Down Expand Up @@ -409,9 +412,11 @@ export class CanvasEntityFilterer extends CanvasModuleBase {
};

teardown = () => {
this.$initialFilterConfig.set(null);
this.konva.group.remove();
this.unsubscribe();
this.konva.group.remove();
// The reset must be done _after_ unsubscribing from listeners, in case the listeners would otherwise react to
// the reset. For example, if auto-processing is enabled and we reset the state, it may trigger processing.
this.resetEphemeralState();
this.$isFiltering.set(false);
this.manager.stateApi.$filteringAdapter.set(null);
};
Expand All @@ -428,7 +433,6 @@ export class CanvasEntityFilterer extends CanvasModuleBase {

cancel = () => {
this.log.trace('Canceling');
this.resetEphemeralState();
this.teardown();
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -535,6 +535,11 @@ export class CanvasSegmentAnythingModule extends CanvasModuleBase {
* Processes the SAM points to segment the entity, updating the module's state and rendering the mask.
*/
processImmediate = async () => {
if (!this.$isSegmenting.get()) {
this.log.warn('Cannot process segmentation when not initialized');
return;
}

if (this.$isProcessing.get()) {
this.log.warn('Already processing');
return;
Expand Down Expand Up @@ -689,7 +694,6 @@ export class CanvasSegmentAnythingModule extends CanvasModuleBase {
});

// Final cleanup and teardown, returning user to main canvas UI
this.resetEphemeralState();
this.teardown();
};

Expand Down Expand Up @@ -758,7 +762,6 @@ export class CanvasSegmentAnythingModule extends CanvasModuleBase {
cancel = () => {
this.log.trace('Canceling');
// Reset the module's state and tear down, returning user to main canvas UI
this.resetEphemeralState();
this.teardown();
};

Expand All @@ -773,8 +776,11 @@ export class CanvasSegmentAnythingModule extends CanvasModuleBase {
* - Resets the global segmenting adapter
*/
teardown = () => {
this.konva.group.remove();
this.unsubscribe();
this.konva.group.remove();
// The reset must be done _after_ unsubscribing from listeners, in case the listeners would otherwise react to
// the reset. For example, if auto-processing is enabled and we reset the state, it may trigger processing.
this.resetEphemeralState();
this.$isSegmenting.set(false);
this.manager.stateApi.$segmentingAdapter.set(null);
};
Expand Down

0 comments on commit e4e78c3

Please sign in to comment.