Page MenuHome

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

Authored by Sybren A. Stüvel (sybren) on Fri, Jan 10, 5:54 PM.

Details

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.

  • 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

Bastien Montagne (mont29) requested changes to this revision.Fri, Jan 10, 9:58 PM

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.Fri, Jan 10, 9:58 PM
Sybren A. Stüvel (sybren) marked 2 inline comments as done.Mon, Jan 13, 11:48 AM

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.

Sybren A. Stüvel (sybren) edited the summary of this revision. (Show Details)

Incorporated feedback from @Bastien Montagne (mont29)

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

Sybren A. Stüvel (sybren) marked 2 inline comments as done.Mon, Jan 13, 11:50 AM
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.

Sybren A. Stüvel (sybren) marked 3 inline comments as done.
This revision is now accepted and ready to land.Tue, Jan 14, 1:26 PM