WIP: Animation&Rigging: Extra operators for the sliders #106281

AresDeveaux · 2023-03-29T22:28:53+02:00

AresDeveaux commented

2023-03-29 22:28:53 +02:00

First-time contributor

This is work in progress. I wanted to show to see if I'm going in the right direction.

We decided to implement Animaide sliders as added functionalities to Christoph Lendenfeld slider operators. That made things kind of ease for me because all the hard work was already done. I'm basically following the structure he created to add the new functionality.

The main work is in Keyframes_general.c where I added comments, but all the other code on all the other files still have Christoph's old comments from his tools I used as a base.

There are two sliders missing: Wave-Noise and Blend-Frame.

-The first because the way I implemented it on my add-on was by creating a copy of the curve and adding modifiers to it, then using that as a reference for the "Y" values, but maybe it should have noise and wave formulas to accomplish the same. Are there functions in the code I could use for that?

-The second slider missing is Blend-Frame. This one should not be too hard to add but I have a GUI related question. For this tool to work, the user needs to add reference frames. On Animaide I use the "N" panel to have that:

Even if I just use markers I would need operators to add them, but I don't know if commands on a menu is the way to go. I even thought people would like to archive multitud of reference frames for latter use:

Any ideas?

This is my first Pull Request ever so bear with me if I'm not doing things right. There is a file, DNA_modifier_types.h, that apparently I added a space by mistake and it is on the latest commit without me realizing it (but don't know how to take it out now).

This is work in progress. I wanted to show to see if I'm going in the right direction. We decided to implement Animaide sliders as added functionalities to Christoph Lendenfeld slider operators. That made things kind of ease for me because all the hard work was already done. I'm basically following the structure he created to add the new functionality. The main work is in Keyframes_general.c where I added comments, but all the other code on all the other files still have Christoph's old comments from his tools I used as a base. There are two sliders missing: Wave-Noise and Blend-Frame. -The first because the way I implemented it on my add-on was by creating a copy of the curve and adding modifiers to it, then using that as a reference for the "Y" values, but maybe it should have noise and wave formulas to accomplish the same. Are there functions in the code I could use for that? -The second slider missing is Blend-Frame. This one should not be too hard to add but I have a GUI related question. For this tool to work, the user needs to add reference frames. On Animaide I use the "N" panel to have that: ![Blend-Frame panel](https://aresdevo.github.io/animaide/images/blend_frame.gif) Even if I just use markers I would need operators to add them, but I don't know if commands on a menu is the way to go. I even thought people would like to archive multitud of reference frames for latter use: ![Frame Bookmarks panel](https://aresdevo.github.io/animaide/images/frame_bookmark.gif) Any ideas? This is my first Pull Request ever so bear with me if I'm not doing things right. There is a file, DNA_modifier_types.h, that apparently I added a space by mistake and it is on the latest commit without me realizing it (but don't know how to take it out now).

animaide in blender core.gif

4.5 MiB

🎉 2

AresDeveaux added 6 commits 2023-03-29 22:29:05 +02:00

Christoph Lendenfeld commented

2023-03-30 09:25:53 +02:00

Disclaimer: I haven't looked at the code yet

For your first question:
Yes for performance reasons I think it would be ideal to calculate a noise function first and then move the keys from that. You might even be able to use the functions from the modifier directly to do that.

Second question:
It's a bit tricky. When developing an addon you have all the freedoms on how you develop your UX but when in core it should match existing functionality if possible.
One option I see is to use the curve value under the cursor and instead of having a bidirectional slider you just use 0-100%

To remove the space in DNA_modifier_types.h just delete the space, save and push. If there is no difference in the files, it will not show up in the changed files.

Also a bit of housekeeping:

If you are not ready to have your code reviewed, prefix the title with WIP:

Try to keep your pull requests small, so 1 feature per PR this makes it a lot easier to review, and also means 1 operator that needs more work doesn't stop all the others from getting committed

You can add the Animation&Rigging label to this PR to indicate where it belongs

Having said that it's really awesome to see these tools in core. Nice work!

Disclaimer: I haven't looked at the code yet For your first question: Yes for performance reasons I think it would be ideal to calculate a noise function first and then move the keys from that. You might even be able to use the functions from the modifier directly to do that. Second question: It's a bit tricky. When developing an addon you have all the freedoms on how you develop your UX but when in core it should match existing functionality if possible. One option I see is to use the curve value under the cursor and instead of having a bidirectional slider you just use 0-100% To remove the space in `DNA_modifier_types.h` just delete the space, save and push. If there is no difference in the files, it will not show up in the changed files. Also a bit of housekeeping: If you are not ready to have your code reviewed, prefix the title with WIP: Try to keep your pull requests small, so 1 feature per PR this makes it a lot easier to review, and also means 1 operator that needs more work doesn't stop all the others from getting committed You can add the Animation&Rigging label to this PR to indicate where it belongs Having said that it's really awesome to see these tools in core. Nice work!

AresDeveaux changed title from ~~animaide~~ to WIP: Animation&Rigging

2023-03-30 09:52:31 +02:00

AresDeveaux commented

2023-03-30 10:16:15 +02:00

First-time contributor

Thanks for the feedback.

I just changed the title of the PR. I hope I understood correctly. In regard to the PR being too large: Should I submit them again individually? If that is the case, should I just close this one?

In regard to the images: Sorry if me posting them was misleading, I just wanted to show the input I need from the user for the "Reference Frames" (not the slider). I need feedback on how or where the user should input that information. I think the sliders you implemented work nicely with the widget you added, I just need the frame number the user selects as reference. The thing is that because your tools live in the menu, asking for those frames on another part of the interface would feel disjointed and nobody would have a clue what are they for. But the operators that ask for those frames are not sliders, so I don't know if it should put them in the slider menu. The answer to that question is what I was trying to find. Sorry again if putting the add-on images was confussing.

Thanks for the feedback. I just changed the title of the PR. I hope I understood correctly. In regard to the PR being too large: Should I submit them again individually? If that is the case, should I just close this one? In regard to the images: Sorry if me posting them was misleading, I just wanted to show the input I need from the user for the "Reference Frames" (not the slider). I need feedback on how or where the user should input that information. I think the sliders you implemented work nicely with the widget you added, I just need the frame number the user selects as reference. The thing is that because your tools live in the menu, asking for those frames on another part of the interface would feel disjointed and nobody would have a clue what are they for. But the operators that ask for those frames are not sliders, so I don't know if it should put them in the slider menu. The answer to that question is what I was trying to find. Sorry again if putting the add-on images was confussing.

AresDeveaux changed title from ~~WIP: Animation&Rigging~~ to WIP: Animation&Rigging: Extra operators for the sliders

2023-03-30 10:20:38 +02:00

AresDeveaux commented

2023-03-30 10:32:33 +02:00

First-time contributor

hmmm... Now that I think about it, how could I submit a PR of just one tool if I made all the commits already?

By the way, I forgot to mention, When I was adding the sliders I went one by one adding what I have on the add-on, but then realized you already have some of them so didn't add them to the rest of the files, but they are still on "keyframe_general.c" and "ED_keyframes_edit.h". I should delete them but forgot. I'm referring to ease, blend_neighbor, and tween (I added a "b" to "ease_b" when realized you already had it).

hmmm... Now that I think about it, how could I submit a PR of just one tool if I made all the commits already? By the way, I forgot to mention, When I was adding the sliders I went one by one adding what I have on the add-on, but then realized you already have some of them so didn't add them to the rest of the files, but they are still on "keyframe_general.c" and "ED_keyframes_edit.h". I should delete them but forgot. I'm referring to ease, blend_neighbor, and tween (I added a "b" to "ease_b" when realized you already had it).

Christoph Lendenfeld commented

2023-03-30 10:37:17 +02:00

yes splitting it up into 1 operator per pull request is the way to go.

About the reference frames:
I am arguing you don't necessarily need 2 of them. And one reference frame you can get from the current frame. So it would be a "Blend to Current Frame" operator
That is until we get time selection, but that is still on the horizon.

yes splitting it up into 1 operator per pull request is the way to go. About the reference frames: I am arguing you don't necessarily need 2 of them. And one reference frame you can get from the current frame. So it would be a "Blend to Current Frame" operator That is until we get time selection, but that is still on the horizon.

Sybren A. Stüvel added the

Module

Animation & Rigging

label 2023-03-30 11:59:31 +02:00

Sybren A. Stüvel added this to the Animation & Rigging project 2023-03-30 11:59:36 +02:00

Christoph Lendenfeld commented

2023-03-30 16:46:11 +02:00

sorry missed the first part of how to split up a PR
it's a bit of work unfortunately, but what I usually do is

make a new branch for each operator
copy the relevant code from the browser

sorry missed the first part of how to split up a PR it's a bit of work unfortunately, but what I usually do is * make a new branch for each operator * copy the relevant code from the browser

AresDeveaux commented

2023-04-06 19:50:21 +02:00

First-time contributor

Closing This PR since I created separate ones for each slider

AresDeveaux closed this pull request

2023-04-06 19:50:21 +02:00

Sybren A. Stüvel removed this from the Animation & Rigging project 2023-04-11 10:39:36 +02:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

WIP: Animation&Rigging: Extra operators for the sliders #106281

Pull request closed