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

temp-preview-undo-sanitize (branched from master)

Build Status

Buildable 12057
Build 12057: 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

Avoid need for versioning, reuse old flag bit

Harbormaster completed remote builds in B11973: Diff 32418.Jan 4 2021, 2:18 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` ?
294–298	Needs some comment similar to above I think?

This revision now requires changes to proceed.Jan 8 2021, 12:35 PM

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

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).

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

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?

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
259–262	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

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

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.

Thanks! Committed now.

UI: Sanitize threaded preview creation with undo
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 32555

source/blender/blenkernel/intern/icons.cc

source/blender/editors/interface/interface_icons.c

source/blender/editors/undo/memfile_undo.c

source/blender/editors/util/ed_util_ops.c

source/blender/makesdna/DNA_ID.h

UI: Sanitize threaded preview creation with undoClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 32555

source/blender/blenkernel/intern/icons.cc

source/blender/editors/interface/interface_icons.c

source/blender/editors/undo/memfile_undo.c

source/blender/editors/util/ed_util_ops.c

source/blender/makesdna/DNA_ID.h

UI: Sanitize threaded preview creation with undo
ClosedPublic
Actions

Revision Contents
Changeset List