Differential D14615

Fix T94559: Copying a geometry node group does not copy animation data
ClosedPublic
Actions

Authored by Angus Stanton (abstanton) on Apr 11 2022, 3:35 AM.

Details

Reviewers

Hans Goudey (HooglyBoogly)

Maniphest Tasks

T94559: Geometry nodes: keyframes being deleted if you delete the same node in a node tree duplicate

Commits

rBccd2e89d37fb: Fix T94559: Copying geometry node group does not copy animation data

Summary

Reimplement copy geo nodes in c.

This is done because the copy() function called in the python version
does not copy the animation data. This causes duplicate node trees to affect
the animation data of originals. Therefore, it has been reimplemented in C.

Diff Detail

Repository

rB Blender

Branch

T94559-geo-nodes-copy (branched from master)

Build Status

Buildable 21608
Build 21608: arc lint + arc unit

Event Timeline

Angus Stanton (abstanton) requested review of this revision.Apr 11 2022, 3:35 AM

Angus Stanton (abstanton) created this revision.

Harbormaster completed remote builds in B21572: Diff 50316.Apr 11 2022, 3:35 AM

Angus Stanton (abstanton) added inline comments.Apr 11 2022, 1:23 PM

source/blender/editors/object/object_modifier.c
3343	Oops, forgot to remove this after copying, will do that

Not sure whether this code should be in object_modifier.c or a node operator in say node_add.cc. The only thing stopping me put it as a node operator is that 1. It assigns the node group to the object, and 2. It's specific to geo nodes, not generic for all node trees

Angus Stanton (abstanton) added a reviewer: Hans Goudey (HooglyBoogly).Apr 11 2022, 4:41 PM

Remove unnecessary update flags and RNA props

Harbormaster completed remote builds in B21586: Diff 50336.Apr 11 2022, 5:18 PM

Behe eddine (Behija) added a subscriber: Behe eddine (Behija).Apr 11 2022, 7:30 PM

WELL I can approve that it's working well

BTW thanks for helping me understand how blender works

Is this WIP for a specific reason? It looks pretty good to me. I just added a few comments about formatting inline.

release/scripts/startup/bl_operators/geometry_nodes.py
83	Keep two lines of white-space here.
source/blender/editors/object/object_intern.h
204	Keep an empty line after the last declaration.
source/blender/editors/object/object_modifier.c
3298	Use title-case in these section names.
3313	I think avoiding newlines before the null checks makes it a bit easier to read, and more clear that the null checks are associated with the previous declaration.

Address Comments

Harbormaster completed remote builds in B21608: Diff 50377.Apr 12 2022, 8:41 PM

In D14615#393060, @Hans Goudey (HooglyBoogly) wrote:

Is this WIP for a specific reason? It looks pretty good to me. I just added a few comments about formatting inline.

Just forgot to remove the WIP tbh. So do you think it's fine to have it as an object operator? If so then I hope it's done now.

Angus Stanton (abstanton) edited the summary of this revision. (Show Details)Apr 12 2022, 8:42 PM

Great, thanks.

So do you think it's fine to have it as an object operator? If so then I hope it's done now.

I don't see why not, there are plenty of other object operators relating to modifiers in this file.

This revision is now accepted and ready to land.Apr 12 2022, 8:57 PM

Do you mind updating the description to describe the change and the reasoning for moving the operator to C? I'll use that as the commit message then.

Hans Goudey (HooglyBoogly) retitled this revision from T94559 - Reimplement copy geo nodes in c to Fix T94559: Copying a geometry node group does not copy animation data.Apr 12 2022, 8:59 PM

In D14615#393096, @Hans Goudey (HooglyBoogly) wrote:

Do you mind updating the description to describe the change and the reasoning for moving the operator to C? I'll use that as the commit message then.

All done

Thought I'd just ping @Hans Goudey (HooglyBoogly) , is this good to go?

Closed by commit rBccd2e89d37fb: Fix T94559: Copying geometry node group does not copy animation data (authored by Angus Stanton (abstanton), committed by Hans Goudey (HooglyBoogly)). · Explain WhyApr 18 2022, 5:22 PM

This revision was automatically updated to reflect the committed changes.

Hans Goudey (HooglyBoogly) added a commit: rBccd2e89d37fb: Fix T94559: Copying geometry node group does not copy animation data.