Page MenuHome
Differential D9974

UI: Sanitize threaded preview creation with undo
ClosedPublic
Actions

Authored by Julian Eisel (Severin) on Jan 2 2021, 8:00 PM.

Details

Reviewers
Brecht Van Lommel (brecht)
Bastien Montagne (mont29)
Commits
rB3850fdd5b957: Assets: Sanitize threaded preview creation with undo
rBcd818fd081f5: Assets: Sanitize threaded preview creation with undo
Summary

With undo/redo after creating assets, previews of the "Current File"
asset library would often go missing, e.g. see T83884.
What I thought was just a simple error with flags, turned out to be a
bigger problem:
The preview generation in a background job will eventually modify ID
data, but the undo push was done prior to that. So obviously, an undo
then would mean the preview is lost.

This patch makes it so undo/redo will regenerate the preview, if the preview
rendering was invoked but not finished in the undone/redone state.

The preview flag PRV_UNFINISHED wasn't entirely what we needed. So I had to
change it to a slightly different flag.

Diff Detail

Repository
rB Blender
Branch
arcpatch-D9974 (branched from master)
Build Status
Buildable 18365
Build 18365: arc lint + arc unit

Event Timeline

Julian Eisel (Severin) created this revision.Jan 2 2021, 8:00 PM
Harbormaster completed remote builds in B11947: Diff 32378.Jan 2 2021, 8:00 PM
Julian Eisel (Severin) requested review of this revision.Jan 2 2021, 8:00 PM
Julian Eisel (Severin) mentioned this in D9886: (Unsatisfying) attempt to sanitize threaded preview creation with undo.Jan 2 2021, 8:09 PM
Julian Eisel (Severin) updated this revision to Diff 32418.Jan 4 2021, 2:18 PM
  • Avoid need for versioning, reuse old flag bit
Harbormaster completed remote builds in B11973: Diff 32418.Jan 4 2021, 2:18 PM
Bastien Montagne (mont29) requested changes to this revision.Jan 8 2021, 12:35 PM

Generally looks good, besides picky notes below.

My main issue with this patch is that it will restart ID previews job on each memfile undo step... Not sure if there would be a way to deffer that to after all undo/redo steps needed have been performed?

source/blender/editors/undo/memfile_undo.c
156

We try to put action (verb) at the end of function names usually: memfile_undosys_unfinished_id_previews_restart ?

304–308

Needs some comment similar to above I think?

This revision now requires changes to proceed.Jan 8 2021, 12:35 PM
Julian Eisel (Severin) updated this revision to Diff 32555.Jan 8 2021, 5:16 PM
Julian Eisel (Severin) marked an inline comment as done.
  • Address review requests
Harbormaster completed remote builds in B12057: Diff 32555.Jan 8 2021, 5:16 PM
Julian Eisel (Severin) mentioned this in T83751: After renaming, Asset Browser shows old data-block name.Jan 21 2021, 10:36 PM
Julian Eisel (Severin) added a comment.Jan 22 2021, 12:55 PM
In D9974#250487, @Bastien Montagne (mont29) wrote:

My main issue with this patch is that it will restart ID previews job on each memfile undo step... Not sure if there would be a way to deffer that to after all undo/redo steps needed have been performed?

It could be done in ed_undo_step_impl() I guess, but that may either mean iterating over all IDs to check if their job needs to be restarted, or passing some info from the undo decoding to it, e.g. which data-blocks were modified.
That's not really great either.
Not sure if it's worth it, it's not like the job stuff is very expensive and it only happens in few cases (undoing after a preview was created).

Julian Eisel (Severin) updated this revision to Diff 35946.Apr 7 2021, 5:56 PM
  • Update to latest master
Harbormaster completed remote builds in B13922: Diff 35946.Apr 7 2021, 5:56 PM
Julian Eisel (Severin) added a project: Asset Browser.Aug 27 2021, 11:53 AM
Julian Eisel (Severin) mentioned this in rB7a5216497cc3: Fix possible crash displaying asset preview from current file.Sep 10 2021, 2:44 PM
Bastien Montagne (mont29) requested changes to this revision.Sep 29 2021, 4:37 PM
Bastien Montagne (mont29) added inline comments.
source/blender/editors/undo/memfile_undo.c
171

Am not particularly happy that this gets called here.

Clearing/tagging preview to be updated in undo code is fine. But think the actual call for update should happen from UI user (in that case, the asset browser/file browser draw code)?

This revision now requires changes to proceed.Sep 29 2021, 4:37 PM
Julian Eisel (Severin) updated this revision to Diff 44201.Nov 1 2021, 2:38 PM
  • Merge branch 'master' into arcpatch-D9974
Harbormaster completed remote builds in B18365: Diff 44201.Nov 1 2021, 2:38 PM
Julian Eisel (Severin) planned changes to this revision.Nov 1 2021, 2:38 PM
Julian Eisel (Severin) mentioned this in T92477: Asset preview lost after edit & reload.Nov 1 2021, 2:59 PM
Julian Eisel (Severin) added inline comments.Nov 1 2021, 3:18 PM
source/blender/editors/undo/memfile_undo.c
171

Since the preview is ID data not UI data, that wouldn't work. You wouldn't want your asset previews to be lost just because you didn't happen to display that particular asset in the Asset Browser after an undo.
But what we could do instead is somehow schedule an update, that is then performed immediately after the undo.

So for example, in wm_event_do_notifiers(), we could listen to NC_WM | ND_UNDO and call some ED_preview_restart_render_pending() there. We'd have to do a FOREACH_MAIN_ID_BEGIN() iteration there which isn't nice. We could avoid it with some global flag set here in the undo system, indicating that some preview needs to be restarted. Is that worth it?

Julian Eisel (Severin) updated this revision to Diff 44762.Nov 15 2021, 4:35 PM
  • Only schedule preview regeneration on undo/redo, don't execute it
Harbormaster completed remote builds in B18664: Diff 44762.Nov 15 2021, 4:36 PM
Bastien Montagne (mont29) requested changes to this revision.Nov 23 2021, 4:33 PM
Bastien Montagne (mont29) added inline comments.
source/blender/editors/undo/memfile_undo.c
257–260

Since you kill the preview job, you have no guarantee that existing (re-used) IDs already have a valid preview. So this code should be executed over the whole bmain, unconditionally.

This revision now requires changes to proceed.Nov 23 2021, 4:33 PM
Julian Eisel (Severin) updated this revision to Diff 45175.Nov 23 2021, 5:50 PM
Julian Eisel (Severin) marked an inline comment as done.
  • Address error found in review
  • Remove byte order mark changes done by IDE
Harbormaster completed remote builds in B18916: Diff 45175.Nov 23 2021, 5:50 PM
Bastien Montagne (mont29) accepted this revision.Nov 24 2021, 8:56 AM

LGTM

This revision is now accepted and ready to land.Nov 24 2021, 8:56 AM
Julian Eisel (Severin) added a commit: rBcd818fd081f5: Assets: Sanitize threaded preview creation with undo.Nov 24 2021, 11:27 AM
Julian Eisel (Severin) added a commit: rB3850fdd5b957: Assets: Sanitize threaded preview creation with undo.
Julian Eisel (Severin) closed this revision.Nov 24 2021, 11:39 AM

Thanks! Committed now.

Revision Contents
Changeset List

PathSize
source/
blender/
blenkernel/
intern/
31 lines
editors/
interface/
3 lines
undo/
45 lines
util/
2 lines
makesdna/
2 lines

Diff 44201

View Options

source/blender/blenkernel/intern/icons.cc

Loading...
View Options

source/blender/editors/interface/interface_icons.c

Loading...
View Options

source/blender/editors/undo/memfile_undo.c

Loading...
View Options

source/blender/editors/util/ed_util_ops.cc

Loading...
View Options

source/blender/makesdna/DNA_ID.h

Loading...