We already had the ability to bake fcurves but no way to convert the baked result back without using python.
This patch adds and operator that is available now next to the bake operator in the drop down menu,
Details
Details
- Reviewers
Sybren A. Stüvel (sybren) - Commits
- rBfb88d4eda8a8: Add a Un-Bake FCurves operator
Diff Detail
Diff Detail
- Repository
- rB Blender
Event Timeline
Comment Actions
I basically copied the "bake" operator with a few modifications. I left the comments from the bake operator code mostly untouched.
Comment Actions
One thing I don't understand from a conceptual point of view, is why there are start and end parameters. Why not convert all the baked data there is?
As the code was moved from a place that already had those start and end parameters, keeping them around makes sense, of course. When it comes to the new un-bake operator, do you have any strong feelings about the default values ? Would it work to have them initialised to INT_MIN and INT_MAX so that all baked data is converted?
| source/blender/blenkernel/intern/fcurve.c | ||
|---|---|---|
| 1126 | 'leading' and 'heading' mean the same thing. Maybe 'leading' and 'trailing'? I know it's just a copy-paste, but it's an excellent opportunity to make things actually correct. | |
Comment Actions
I'm not sure if we should change the start and end frame range as this matches how the bake operator works.
Perhaps changing how the bake/unbake works for the bake and now unbake operator could be done in an other commit?
(When we perhaps have a more concrete plan on how we want this to work)
Comment Actions
👍
| source/blender/blenkernel/intern/fcurve.c | ||
|---|---|---|
| 1072 | TODOs should have the name of the person who knows more about this. | |
| source/blender/editors/space_graph/graph_edit.c | ||
| 1864 | Sentences should end in a period. | |
| 1900 | Comments should be full sentences (start with capital, end with period). | |
| 1906 | TODOs should have the name of the person who knows more about this. | |
| 1906 | No C++ comments in C files. | |
| 1915 | No C++ comments in C files. | |
| 1936 | TODOs should have a name (and be capitalised). | |
Comment Actions
The recent comments are for copy/pasted code. It would be weird to me to fix these here but not for the rest of the code.
How about we do this in multiple steps? One commit with the unbake operator and one clean up commit.
This could have been done in one go in phabricator worked with the pull request workflow (github/gitlab/gittea etc).
I'm guessing it is not possible to have multiple commits in one differential, right?
Comment Actions
Good idea.
I'm guessing it is not possible to have multiple commits in one differential, right?
That is possible, but not so nice to deal with. I also don't think you can do a review per commit.
Comment Actions
So I guess I'll create a separate pull request from the clean up first and then update this one after it has been merged?
Comment Actions
Yeah, that's the way. You can also use arc diff --base=git:somerevision to create diffs based on other revisions. That way you don't have to wait for one rev to land before uploading the next.
Comment Actions
I can understand why the Bake operatoror would have a start/end (because you may want to bake the extrapolated curves as determined by modifiers, for example). It is also a requirement, because you can't edit the baked curves any more, so any trimming of the bake range has to be done beforehand.
To me it doesn't make much sense for the un-bake operator, though. After un-baking, the FCurve is editable again, and so you can just delete whatever you want. Let's keep the un-bake operator as simple as possible, doing only one thing. Trimming animation data can be done by other tools.
As for the notes I left on the code that you simply moved, I think it's a good idea to just commit this patch first, without these changes, and then commit a cleanup right after that. Or, and that would probably be even better, do the cleanup first. That way, in the hypothetical case that your functional changes have to be rolled back, we still have the cleanup. In this case I think the chances of a rollback are minimal, but as a general rule of thumb I think it's good to do cleanup before functional changes.
| source/blender/blenkernel/intern/fcurve.c | ||
|---|---|---|
| 1068 | start and end can be const. | |
| 1071 | Comments should be full sentences, so start with capital and use punctuation. That includes copy-pasted comments. | |
| 1095–1097 | Try to avoid overly abbreviated names. | |
| 1096–1097 | tot_kf and tot_sp can be declared const. | |
| 1103 | Why the (float) cast? I don't think that's necessary here. | |
| 1107 | The comment below says that the points are linear, which means that they won't have handles. Since that is the case, what does "flat points" mean? What are "dummy" points, and why are they needed at all? | |
| 1108 | This skips the fpt->vec[0] == start case, which is also skipped by the above for-loop. | |
| 1128–1131 | This exact same code is duplicated twice. Better to turn it into a small function and call it three times. | |
| source/blender/editors/space_graph/graph_edit.c | ||
| 1866 | This is exactly the opposite of what this function is doing. | |
| 1871–1874 | filter can be declared and initialised one the same line, and declared const. | |
| 1882 | No need to repeat the function name in a comment. | |
| source/blender/blenkernel/intern/fcurve.c | ||
|---|---|---|
| 1068 | I can't as I increment start in the code. | |
| 1096–1097 | They can't be declared as const as they are decremented in the code. | |
| 1107 | This is so that points are added if there are no data before the start frame. So if for example the bakes keys start at frame 10 but we start at frame 5. I'm guessing this is so that there is always data on the unbake frame range. | |
| 1108 | Yes? | |
Comment Actions
I think it would be wasteful do to a const as the int is not a reference and I would need to delare and alternate start variable if I declare it as const.
I think readability is more important than code efficiency (unless it's in an inner-inner loop or in otherwise really hot code, which most code isn't). The code as it is now changes the value of start, but after that it's no longer "the start", it's "the frame number". In my opinion, that change in semantics deserves a new variable name. The same for the variables that are named "the total number of X". Once those are used as "how much there is left to do" instead of "the total number of", IMO they should be given a new name. I find it hard to read when code promises one thing but then does something else; for example, tot_keyframes-- implies that the total number of keyframes goes down, but that's not what is happening in the code.
I know that you just moved some code around, and that these issues were already there, but I also think Sergey's approach (when you touch it, you clean it up) is a healthy one given that so much of Blender's code deserves a cleanup.
| source/blender/blenkernel/intern/fcurve.c | ||
|---|---|---|
| 1069 | "We match" is ambiguous. My grammar terminology is failing me here; I can read "we match the baked curve shape" as "we make sure [the unbaked curve shape] matches the baked curve shape", but also as "our shape matches the baked curve shape". Are you saying I should go to the gym more often? ;-) I think "Baked FCurve points always use linear interpolation." would be clearer. | |
| 1079 | The name is supposed to be in the form TODO(aligorith). But, given upcoming changes to the commenting guidelines (T81452, most importantly Brecht's comment T81452#1029052), I'm not even sure what will be the final verdict on/form of this. I think I agree with Brecht in that the comment should be so complete that it doesn't require any knowledge of who added it or who to ask for more information. So in that light, and sorry for the inconsistency, I withdraw my earlier "add names to TODOs" note. | |
| source/blender/blenkernel/intern/fcurve.c | ||
|---|---|---|
| 1079 | This is consistent with the rest of the file now. | |
| release/scripts/startup/bl_ui/space_graph.py | ||
|---|---|---|
| 303–305 | In a future commit, IMO these could move from the Key menu to the Channel menu. After all, they bake entire channels and don't operate on individual/selected keys only. | |
| source/blender/blenkernel/intern/fcurve.c | ||
| 1079 | 👍 | |
| source/blender/editors/space_graph/graph_edit.c | ||
| 1906–1907 | Let's not copy "For now" comments from 2009. | |
| 1930 | I just noted this one, and I think we can remove it. Un-baking is a lossless process (ignoring the start/end frame), and we have an undo system that people can use if they're not happy with the result. I don't see the need for a confirmation pop-up. | |
| 1937 | This is a new operator, so I don't see why it should have a TODO from 2009 in there. | |
Comment Actions
I tested the patch, and it works well 👍
Do whatever you wish with my comments about the TODOs. The confirmation popup is a bit weird I think. Because it's not common for operators to require confirmation, I didn't expect it, and my mouse moved away, which discarded the popup. If I hadn't looked well, I would have missed it and thought that the operator didn't do anything.
| source/blender/editors/space_graph/graph_edit.c | ||
|---|---|---|
| 1937 | But muh vintage code comments! D: | |