Details

Reviewers

Bastien Montagne (mont29)
Alexander Gavrilov (angavrilov)

Commits

rBd49fa504fdf0: Fix T72861: Viewport Render Keyframes ignores Grease Pencil and Shape Keys

Summary

The Viewport Render Keyframes operator didn't include keyframes from grease pencil animation or from mesh shape key animation (see T72861).

To find all related datablocks that could be considered as part of the selected object, BKE_library_foreach_ID_link(..., IDWALK_RECURSE) is used. To prevent relations like constraint targets from being visited, the recursion is stopped when it finds a different object.

An alternative approach would be to explcitly follow certain pointers, for example mesh->key. However, this would require more code, and is likely to miss some pointer as well (if not now, then in a future version). This approach seems more future-proof to me.

Since this is a fix for a new feature introduced in 2.82, I see this change as a bugfix.

This file allows you to test the feature.

D6558-demo.blend646 KBDownload

Open the blend file.
When you open the blend file, the four objects that have a text below them should be selected.
In the 3D viewport 'View' menu, choose Viewport Render Keyframes.
PNG files for all frames should have been rendered, but the burnt-in frame number should show that only frames 1, 3, 5, 8, 10, 12, 14, 16, 18, and 19 have been rendered. These are the only frames for which the selected objects have keys.

Diff Detail

Repository: rB Blender

Event Timeline

Sybren A. Stüvel (sybren) created this revision.Jan 10 2020, 5:54 PM

Harbormaster completed remote builds in B6213: Diff 20714.Jan 10 2020, 5:54 PM

Sybren A. Stüvel (sybren) edited the summary of this revision. (Show Details)Jan 10 2020, 5:58 PM

Sybren A. Stüvel (sybren) added a project: BF Blender (2.82).

Sybren A. Stüvel (sybren) added a subscriber: Sebastian Parborg (zeddb).

Besides technical notes below, am still skeptical about the idea of using libquery for that… among other potentially annoying (and completely unmanageable) cases, you have:

IDs stored in ID props (aka custom properties) by users or scripts - those can basically point to any random ID, and would be followed by the recursive libquery call.
A bit similar, ID's that might be used by random modifiers (textures…), but also nodetrees (from materials mainly).

If we accept those relatively unlikely, but potentially 'exploding' cases in that feature, it should at least be clearly documented in a comment I think.

source/blender/editors/render/render_opengl.c
597	This should also return in case `id == id_self`, you are supposed to have already handled that ID's animation earlier. So should be `if (GS(id->name) == ID_OB \|\| id == id_self)` ?
597–602	I would rather have a white list of IDTypes allowed to be used here, than a blacklist… This would be much more clear (and future-proof). E.g., does it mean that you do want to use material or texture keyframes here as well?
610	This comment is a bit misleading… By reading it I had the impression that GP datablocks did not return their animdata! Should be rephrased imho, maybe something like: `In addition to regular ID's animdata, GreasePencil uses a specific frame-based animation system that requires specific handling here` …or something like that?
613–615	This should rather be handled by checking for `IDWALK_CB_LOOPBACK`. Would be much safer/generic. Although… this should not be needed at all in fact, code in libquery uses a gset to keep track of IDs already handled, so that kind of infinite loop should never be possible anyway. Did you actually have that issue? Or did you add that on a theoretical basis?

This revision now requires changes to proceed.Jan 10 2020, 9:58 PM

In D6558#154171, @Bastien Montagne (mont29) wrote:

Besides technical notes below, am still skeptical about the idea of using libquery for that… among other potentially annoying (and completely unmanageable) cases, you have:

IDs stored in ID props (aka custom properties) by users or scripts - those can basically point to any random ID, and would be followed by the recursive libquery call.

True.

A bit similar, ID's that might be used by random modifiers (textures…), but also nodetrees (from materials mainly).

I don't think this is much of an issue, as those keys also show up in the dopesheet under the selected object.

If we accept those relatively unlikely, but potentially 'exploding' cases in that feature, it should at least be clearly documented in a comment I think.

That, and in the manual as well.

source/blender/editors/render/render_opengl.c
597	`gather_frames_to_render_for_id(oglrender, id, &id, 0);` was called for every selected object (where `id` is the object), so this check is necessary to allow that call but to disallow following pointers to a different object. So, the check is necessary as-is, but I can see that it can be confusing to have an explicit call to a function that's also a library query callback function. As solution I have duplicated this code AnimData *adt = BKE_animdata_from_id(id); gather_frames_to_render_for_adt(oglrender, adt); into the `FOREACH` over all selected objects. This removes the entire handling of `id_self` from the callback; by not having `ID_OB` in the whitelist these pointers are now automatically skipped.
597–602	I would rather have a white list of IDTypes allowed to be used here 👍 E.g., does it mean that you do want to use material or texture keyframes here as well? Yes. Keyframes on those are shown in the dope sheet under the object itself, so as a user I would expect those to count as well.

Incorporated feedback from @Bastien Montagne (mont29)

Harbormaster completed remote builds in B6224: Diff 20745.Jan 13 2020, 11:49 AM

Used explicit base revision for the patch, so that it doesn't include other commits on the release branch.

Harbormaster completed remote builds in B6225: Diff 20746.Jan 13 2020, 11:50 AM

Sybren A. Stüvel (sybren) marked 2 inline comments as done.Jan 13 2020, 11:50 AM

Bastien Montagne (mont29) added inline comments.Jan 13 2020, 4:45 PM

source/blender/editors/render/render_opengl.c
597	You still need to check for `id_self`, and return/skip in case of equality. Other IDtypes may end up referencing themselves one way or the other, and those (the `id_self` ones) have always already been processed, so you do not want to waste time processing them again.
603	Explicitly use a const `ID_Type` enum variable as 'parameter' of the `switch`. Together with listing all known IDtypes (and not using `default:` clause), this ensure the compiler will complain if/when someone adds a new IDtype and forgets to update that switch properly. ;)
630	I would rather explicitly list IDTypes we do not want to care about here, and have no `default:` clause at all. See also previous comment about using `ID_Type` enum.