Page MenuHome

FCurves: Offset modifier
Needs RevisionPublic

Authored by Takain (Takain) on Sep 16 2020, 12:36 PM.

Details

Summary

This is a small feature that allows to offset the fcurve in either x or y axis.
I couldn't find a way to modify the curves without modifying the keyframes themselves, if this is already possible then this is not needed.

It can be useful in many cases:

  • when having too many curves, offsetting manually can be time consuming
  • allows to adjust the curves non destructively
  • helps to adjust the output of the modifier stack, as some other modifiers wont allow you to edit the keyframes anymore
  • plays well with other modifiers, such as cycles, can help break down the repetition
  • when exporting fbx to game engines, it helps achieving looping easier by not changing the exported keyframe range (if you had to move the keyframes themselves)

However:

  • it's useful when dealing with assets/actions but not so much when using for continuous animation like a scene or the NLA
  • right now you have adjust curve by curve if you want different random offsets (if this one gets accepted, I have an idea to improve this, but in another commit because affects other modifiers)

Randomize property is a random amount from 0 to offset it will subtract from the chosen offset
If you choose to randomize 50%, and the offset value to 80, it will evaluate the value to be 40 to 80.
And is applied to the whole curve
Its applied to both X and Y because you can simply stack if you want different random for each

Diff Detail

Event Timeline

Takain (Takain) requested review of this revision.Sep 16 2020, 12:36 PM
Takain (Takain) created this revision.
  • Fix randomize only applying to frame
Ankit Meel (ankitm) edited the summary of this revision. (Show Details)Sep 19 2020, 1:15 PM
Christoph Lendenfeld (ChrisLend) requested changes to this revision.Oct 5 2020, 1:16 PM

Hey there,

This is a really cool patch and it's working well.
There are just some minor code style fixes you'd need to do (refer to this page: https://wiki.blender.org/wiki/Style_Guide/C_Cpp)

Major changes are comments:
Start with upper case and end with a dot.
Use C style comments consistently (/* */)
Cut the comments that don't explain anything. That's especially the one word comments.

Also for value literals here is how they should be done

float/double (f only for floats):
use: 0.3f, 1.0f
not: .3, 1.f

Again, great work and with a few tweaks we can have that modifier in Blender

This revision now requires changes to proceed.Oct 5 2020, 1:16 PM
  • Fix code style

Thanks for the review.

I think this looks good to me as well.

The only thing I have to add it the "Randomize" percentage change nitpick.

source/blender/makesrna/intern/rna_fcurve.c
1619

I think we should change this to be percentage then instead?

So instead of PROP_FACTOR it is PROP_PERCENTAGE instead.

Then you will not have to explain that 0.2 == 20% and it is clear in the UI that this is percentage just at a glance.

Sybren A. Stüvel (sybren) requested changes to this revision.Oct 6 2020, 2:33 PM

I think the feature is nice to have. Some clarification would be welcome, though. To me, at least from the patch description and screenshot, it's unclear what "Randomize" would do.

  • Randomize the keyframe X or Y coordinates? Maybe both?
  • Would it apply a different random value to each keyframe, or would it pick a single random offset and apply that to all keyframes in the curve?
  • What is the percentage taken over? It is described as "a maximum of (percentage) variation", but what variation?
source/blender/blenkernel/intern/fmodifier.c
1052

Mark evaltime as const, and return evaltime - offset.

1054

const FMod_Offset *

1057

const float

1057

Just name the variable xoffset, so that it's clear which offset is being referred to without having to go back to this line.

1059–1060

Run the auto-formatter (make format) before sending in a patch.

This revision now requires changes to proceed.Oct 6 2020, 2:33 PM
Takain (Takain) edited the summary of this revision. (Show Details)
  • Fix constness and format
  • Clarify Randomize in UI and patch description
  • Change Randomize to percentage
Sybren A. Stüvel (sybren) requested changes to this revision.Oct 7 2020, 5:39 PM

@Takain (Takain) Please mark notes as "Done" when you've handled them. That makes it possible to hide them (hamburger menu in the top right corner), making it easier to see the code again.

I don't really see the use of creating a new RNG for every "random" value, then initialising it with the same seed every time. This will just produce the same "random" value each time. How is this feature intended to work?

This revision now requires changes to proceed.Oct 7 2020, 5:39 PM
Takain (Takain) marked 6 inline comments as done.EditedOct 7 2020, 6:49 PM

I don't really see the use of creating a new RNG for every "random" value, then initialising it with the same seed every time. This will just produce the same "random" value each time. How is this feature intended to work?

Its for example if you need some curves with different offsets, but some without, im using it by pasting the offset modifiers it on the curves i want and then changing the seeds
one way to use that is on animations, where rotating bones with different offsets in each curve looks ugly, but moving them with an offset in each curve looks nice, so its intended that same seed will have same value for the whole curve

I though on having a checkbox to reinitialize seeds on pasting or adding fmodifiers, to easily change the offsets, but i didnt include in this patch because it would affect all modifiers that have a seed such as noise

Now for the technical part i couldnt place the random generation on new_data, because you may want to change the seed later, maybe theres a better place to put other than fcm_offset_evaluate, should i move it elsewhere? or maybe just remove randomize for now?