blender
/
blender
133
398
Fork 579
Code Issues 6.3k Pull Requests 746 Projects 19 Wiki Activity

Anim: View FCurve of Property in the Graph Editor #114407

Merged
Christoph Lendenfeld merged 40 commits from ChrisLend/blender:focus_in_ge into main 2023-11-21 14:07:03 +01:00
Conversation 35 Commits 40 Files Changed 2 +284 -3
Christoph Lendenfeld commented 2023-11-02 15:02:27 +01:00
Member
Copy Link

This PR is inspired by a tool that the animators from Goo studios showed me at the Blender Conference 23

Description

This PR adds a new operator that allows to frame an FCurve in the Graph Editor from the animated property.

Features

  • Frame a single property or a whole array property by right-clicking an animated property
  • Works on a property anywhere in blender
  • Framed FCurves are selected and set to visible
  • Works on the selection. If an object/bone doesn't have a property it is ignored.
  • Works with NLA offset and normalization
  • Isolate curves. This is a property on the operator, so when assigning the operator as a hotkey you can choose the behavior.

Caveats

  • Frames on the first Graph Editor it finds. While that is a bit arbitrary, it is definitely better than framing all Graph Editors because the only use case I see for having more than one is if you want to see different parts of the curves.
  • Since it works on the selection but the n-panel works on the active object, you can create a situation where nothing happens because you can have an active object without it being selected.
  • Assigning a shortcut doesn't work through right clicking the menu entry. You have to go to the keymap and create a new entry manually (e.g. in the user interface category)

Demo

This PR is inspired by a tool that the animators from Goo studios showed me at the Blender Conference 23 # Description This PR adds a new operator that allows to frame an FCurve in the Graph Editor from the animated property. # Features * Frame a single property or a whole array property by right-clicking an animated property * Works on a property anywhere in blender * Framed FCurves are selected and set to visible * Works on the selection. If an object/bone doesn't have a property it is ignored. * Works with NLA offset and normalization * Isolate curves. This is a property on the operator, so when assigning the operator as a hotkey you can choose the behavior. # Caveats * Frames on the first Graph Editor it finds. While that is a bit arbitrary, it is definitely better than framing all Graph Editors because the only use case I see for having more than one is if you want to see different parts of the curves. * Since it works on the selection but the n-panel works on the active object, you can create a situation where nothing happens because you can have an active object without it being selected. * Assigning a shortcut doesn't work through right clicking the menu entry. You have to go to the keymap and create a new entry manually (e.g. in the user interface category) # Demo <video src="/attachments/f67a667f-2581-461b-ae83-41b3e5db9b7f" title="Frame Fcurve from anywhere" controls></video>
view_fcurve.mp4
9.3 MiB
❤️ 7
Christoph Lendenfeld added the
Module
Animation & Rigging
 label 2023-11-02 15:02:27 +01:00
Christoph Lendenfeld added 12 commits 2023-11-02 15:02:40 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
11b0d974b2 don't show menu entry when thing isn't animated
Christoph Lendenfeld added the
Interest
User Interface
 label 2023-11-02 15:04:35 +01:00
Christoph Lendenfeld commented 2023-11-02 15:05:46 +01:00
Author
Member
Copy Link

@blender-bot package

@blender-bot package
Blender Bot commented 2023-11-02 15:05:49 +01:00
Member
Copy Link

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR114407) when ready.
Christoph Lendenfeld changed title from WIP: Anim: View Property FCurve in the Graph Editor to WIP: Anim: Frame FCurve of Property in the Graph Editor 2023-11-02 15:06:11 +01:00
hamza.SMA commented 2023-11-02 15:29:34 +01:00
First-time contributor
Copy Link

hi, @ChrisLend nice waited feature ,I'll be direct 👍
suggestion one : add quick shortcut like" hover on it+Ctlr+LClick" for quick reaction.
suggestion two: if graph editor is not opened in actual workspace:
1- display a pup up graph editor if it is not opened in actual workspace
2- switch any existing editor type in the bottom
3- switch any existing animations editor type to graph editor
4- switch to a specific workspace dedicated to animation named "Animation" by default, can be changed in preference..

hi, @ChrisLend nice waited feature ,I'll be direct 👍 suggestion one : add quick shortcut like" hover on it+Ctlr+LClick" for quick reaction. suggestion two: if graph editor is not opened in actual workspace: 1- display a pup up graph editor if it is not opened in actual workspace 2- switch any existing editor type in the bottom 3- switch any existing animations editor type to graph editor 4- switch to a specific workspace dedicated to animation named "Animation" by default, can be changed in preference..
Christoph Lendenfeld commented 2023-11-02 17:06:20 +01:00
Author
Member
Copy Link

@hamza-el-barmaki
I didn't add a hotkey on purpose since that would clutter the global keymap. I expect the user to do that if needed.

I'll talk to the UI module about the popup. I think it violates Blenders design principles of a non overlapping GUI. That said it is being done for the render window...
I'd like to avoid switching the workspace. That feels too extreme and unpredictable

@hamza-el-barmaki I didn't add a hotkey on purpose since that would clutter the global keymap. I expect the user to do that if needed. I'll talk to the UI module about the popup. I think it violates Blenders design principles of a non overlapping GUI. That said it is being done for the render window... I'd like to avoid switching the workspace. That feels too extreme and unpredictable
👍 1
Raymond-Luc commented 2023-11-02 20:05:13 +01:00
First-time contributor
Copy Link

This is awesome! A few of our findings after talking to our animators about it:

  • Our current tool hides the rest of the curves once the operator is run. Could we add that as well? Or perhaps a preference setting to add that functionality. Right now we have to run the Show in the Graph editor operator, then hit the Shift H hotkey to filter out the curves we want and hide everything else.

  • Caveat for above point. Ideally once we figure out the system that prevents keys from being hidden in other workspaces when hiding curves in the graph editor, we would favour that instead of the Shift H workflow instead,

  • Assuming if the graph editor is not open. If we do a workspace switch once using the operator, it should only switch the dopesheet/action editor to the graph and not the timeline. But we are okay with it not switching workspaces.

Thanks a bunch!

This is awesome! A few of our findings after talking to our animators about it: - Our current tool hides the rest of the curves once the operator is run. Could we add that as well? Or perhaps a preference setting to add that functionality. Right now we have to run the ``Show in the Graph editor`` operator, then hit the Shift H hotkey to filter out the curves we want and hide everything else. - Caveat for above point. Ideally once we figure out the system that prevents keys from being hidden in other workspaces when hiding curves in the graph editor, we would favour that instead of the Shift H workflow instead, - Assuming if the graph editor is not open. If we do a workspace switch once using the operator, it should only switch the dopesheet/action editor to the graph and not the timeline. But we are okay with it not switching workspaces. Thanks a bunch!
Chris Clyne (Lateasusual) commented 2023-11-03 00:42:51 +01:00
First-time contributor
Copy Link

To add to @Raymond-Luc 's above comment, here's the goo engine code for reference. It's very naive and a little "dumb" especially in finding an active fcurve editor, i think a tidier solution would suit main blender better (ours tailored to the way most of our animators work)

1574a31c15

To add to @Raymond-Luc 's above comment, here's the goo engine code for reference. It's very naive and a little "dumb" especially in finding an active fcurve editor, i think a tidier solution would suit main blender better (ours tailored to the way most of our animators work) https://github.com/dillongoostudios/goo-engine/commit/1574a31c15022dfa87e669c7b2734cd939858a71
Chris Clyne (Lateasusual) commented 2023-11-03 00:47:53 +01:00
First-time contributor
Copy Link

Also, our implementation is more like a python macro translated to C code - it's calling operators and all other kinds of nonsense (it was originally written in python using some pointer offsets to get the right-clicked property, which isn't accessible from the py API. Maybe a better solution wouldn've been to just expose that value instead...)

Also, our implementation is more like a python macro translated to C code - it's calling operators and all other kinds of nonsense (it was originally written in python using some pointer offsets to get the right-clicked property, which isn't accessible from the py API. Maybe a better solution wouldn've been to just expose that value instead...)
Philipp Oeser commented 2023-11-03 08:51:32 +01:00
Member
Copy Link

@hamza-el-barmaki
I didn't add a hotkey on purpose since that would clutter the global keymap. I expect the user to do that if needed.

I'll talk to the UI module about the popup. I think it violates Blenders design principles of a non overlapping GUI. That said it is being done for the render window...
I'd like to avoid switching the workspace. That feels too extreme and unpredictable

Even if not ideal either I could imagine it working like the Edit Source operator (so there would be some kind of consistency)?
It simply spits out this info banner to the status bar if no Text Editor is open at the time of execution:
image

> @hamza-el-barmaki > I didn't add a hotkey on purpose since that would clutter the global keymap. I expect the user to do that if needed. > > I'll talk to the UI module about the popup. I think it violates Blenders design principles of a non overlapping GUI. That said it is being done for the render window... > I'd like to avoid switching the workspace. That feels too extreme and unpredictable Even if not ideal either I could imagine it working like the `Edit Source` operator (so there would be some kind of consistency)? It simply spits out this info banner to the status bar if no Text Editor is open at the time of execution: ![image](/attachments/44732858-17ae-4207-869a-8e163a370e2e)
Christoph Lendenfeld commented 2023-11-03 09:57:14 +01:00
Author
Member
Copy Link

To add to @Raymond-Luc 's above comment, here's the goo engine code for reference. It's very naive and a little "dumb" especially in finding an active fcurve editor, i think a tidier solution would suit main blender better (ours tailored to the way most of our animators work)

1574a31c15

thanks for the link to your implementation, I didn't know that was written in C. My way of finding the Graph Editor is mostly the same. Not sure what a smart way would look like, I feel like iterating through the areas is the best one can do.

@Raymond-Luc
About hiding FCurves. I need to think about this. I see a use case where people do not want to have their other curves hidden.
For the second point, I am not quite sure I understand. Where else are curves hidden apart from the Graph Editor?

@lichtwerk
Yeah that's not a bad idea, I'll need to check the code for that operator because I am not sure how to set things in an editor that isn't currently open

> To add to @Raymond-Luc 's above comment, here's the goo engine code for reference. It's very naive and a little "dumb" especially in finding an active fcurve editor, i think a tidier solution would suit main blender better (ours tailored to the way most of our animators work) > > https://github.com/dillongoostudios/goo-engine/commit/1574a31c15022dfa87e669c7b2734cd939858a71 thanks for the link to your implementation, I didn't know that was written in C. My way of finding the Graph Editor is mostly the same. Not sure what a smart way would look like, I feel like iterating through the areas is the best one can do. @Raymond-Luc About hiding FCurves. I need to think about this. I see a use case where people do not want to have their other curves hidden. For the second point, I am not quite sure I understand. Where else are curves hidden apart from the Graph Editor? @lichtwerk Yeah that's not a bad idea, I'll need to check the code for that operator because I am not sure how to set things in an editor that isn't currently open
Nika Kutsniashvili commented 2023-11-03 10:41:03 +01:00
Contributor
Copy Link

I also have python operator for this, and I also hide F-curves. Because reality is most of the time I need this feature is when I'm not in Animation workspace and quickly need to inspect some f-curve and make slight adjustments. When I inspect F-curve, I want to see that one only.

But most important for me is that if there is no Graph Editor in the workspace, it creates a pop-up window with Graph Editor. I know Blender doesn't like Pop-up editors, but it's the best way to quickly tweak animation without having to dive fully in Animation workspace when you just want slightest adjustment.

I also have python operator for this, and I also hide F-curves. Because reality is most of the time I need this feature is when I'm not in Animation workspace and quickly need to inspect some f-curve and make slight adjustments. When I inspect F-curve, I want to see that one only. But most important for me is that if there is no Graph Editor in the workspace, it creates a pop-up window with Graph Editor. I know Blender doesn't like Pop-up editors, but it's the best way to quickly tweak animation without having to dive fully in Animation workspace when you just want slightest adjustment.
❤️ 1
Raymond-Luc commented 2023-11-03 17:11:48 +01:00
First-time contributor
Copy Link

@Raymond-Luc
About hiding FCurves. I need to think about this. I see a use case where people do not want to have their other curves hidden.
For the second point, I am not quite sure I understand. Where else are curves hidden apart from the Graph Editor?

Ah what I meant is the behaviour when if you hide a curve in the graph editor, the keys for that curve are also hidden in the dopesheet/action editor and timeline as well. I don't quite remember if it was in a BCON talk or an offline chat with Sybren, but I recall hearing that a new system was being drafted to address this (or I may be completely wrong)

> @Raymond-Luc > About hiding FCurves. I need to think about this. I see a use case where people do not want to have their other curves hidden. > For the second point, I am not quite sure I understand. Where else are curves hidden apart from the Graph Editor? Ah what I meant is the behaviour when if you hide a curve in the graph editor, the keys for that curve are also hidden in the dopesheet/action editor and timeline as well. I don't quite remember if it was in a BCON talk or an offline chat with Sybren, but I recall hearing that a new system was being drafted to address this (or I may be completely wrong)
Christoph Lendenfeld added 3 commits 2023-11-07 13:47:57 +01:00
Christoph Lendenfeld changed title from WIP: Anim: Frame FCurve of Property in the Graph Editor to WIP: Anim: View FCurve of Property in the Graph Editor 2023-11-07 16:22:15 +01:00
Christoph Lendenfeld added 5 commits 2023-11-09 15:21:43 +01:00
Christoph Lendenfeld added 2 commits 2023-11-09 15:35:13 +01:00
Christoph Lendenfeld added 3 commits 2023-11-09 16:27:32 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
c25f4939fa add userpref setting for isolating F-Curves
Christoph Lendenfeld commented 2023-11-09 16:28:00 +01:00
Author
Member
Copy Link

@blender-bot package

@blender-bot package
Blender Bot commented 2023-11-09 16:28:04 +01:00
Member
Copy Link

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR114407) when ready.
Christoph Lendenfeld commented 2023-11-09 16:31:47 +01:00
Author
Member
Copy Link

@Raymond-Luc @Nika-Kutsniashvili

Ignore that comment, was changed right after to be a modifier key. Now pressing Alt while clicking isolates the curves

I made a new build with the following changes.

  • Terminology changed to "View ...", that was feedback from the UI module
  • There is now a user preference under "Animation" to isolate F-Curves when the are framed

The latter creates an inconsistency: There is also the option to "Frame" an FCurve by right clicking in the channels.
That currently does not isolate the curves. Would you expect that to do it as well?
If not, how do we make that consistent and not confusing.

@Raymond-Luc @Nika-Kutsniashvili Ignore that comment, was changed right after to be a modifier key. Now pressing `Alt` while clicking isolates the curves > I made a new build with the following changes. > * Terminology changed to "View ...", that was feedback from the UI module > * There is now a user preference under "Animation" to isolate F-Curves when the are framed > > The latter creates an inconsistency: There is also the option to "Frame" an FCurve by right clicking in the channels. > That currently does not isolate the curves. Would you expect that to do it as well? > If not, how do we make that consistent and not confusing.
Christoph Lendenfeld added 2 commits 2023-11-09 17:27:58 +01:00
Christoph Lendenfeld changed title from WIP: Anim: View FCurve of Property in the Graph Editor to Anim: View FCurve of Property in the Graph Editor 2023-11-09 17:28:57 +01:00
Christoph Lendenfeld added 1 commit 2023-11-09 21:19:11 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
4ef5128e0a remove user preference and add modifier key
Christoph Lendenfeld commented 2023-11-09 21:21:01 +01:00
Author
Member
Copy Link

@blender-bot package

@blender-bot package
Blender Bot commented 2023-11-09 21:21:05 +01:00
Member
Copy Link

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR114407) when ready.
Christoph Lendenfeld commented 2023-11-09 21:26:58 +01:00
Author
Member
Copy Link

Added a property to the operator isolate
When that is true, all other F-Curves other than the framed are hidden
This can be triggered by pressing ALT and clicking the menu entry

This was done after a discussion in the Animation&Rigging module meeting, where it was decided that this is the best way to implement this.

Added a property to the operator `isolate` When that is true, all other F-Curves other than the framed are hidden This can be triggered by pressing `ALT` and clicking the menu entry This was done after a discussion in the Animation&Rigging module meeting, where it was decided that this is the best way to implement this.
Christoph Lendenfeld added 2 commits 2023-11-10 09:19:42 +01:00
Nathan Vegdahl requested changes 2023-11-13 12:51:05 +01:00
Nathan Vegdahl left a comment
Member
Copy Link

Over-all the code looks good to me. Just some nits, and a suggested refactor of one of the functions.

I haven't tested it yet, however. I'll do that tomorrow.

Over-all the code looks good to me. Just some nits, and a suggested refactor of one of the functions. I haven't tested it yet, however. I'll do that tomorrow.
source/blender/editors/animation/anim_channels_edit.cc Outdated
@ -4271,0 +4303,4 @@
return found_graph_editor;
}
static void deselect_all_fcurves(bAnimContext *ac, const bool isolate)
Nathan Vegdahl commented 2023-11-13 12:15:21 +01:00
Member
Copy Link

Nit: out-of-context I don't think isolate is very descriptive for the second argument. It's not actually isolating something, rather it's hiding everything. So maybe just hide?

Nit: out-of-context I don't think `isolate` is very descriptive for the second argument. It's not actually isolating something, rather it's hiding everything. So maybe just `hide`?
nathanvegdahl marked this conversation as resolved
source/blender/editors/animation/anim_channels_edit.cc Outdated
@ -4271,0 +4328,4 @@
char *id_to_prop_path,
const int index,
const bool whole_array,
rctf *r_bounds)
Nathan Vegdahl commented 2023-11-13 12:38:56 +01:00
Member
Copy Link

Since rctf is a fairly small plain-old-data struct, I think it would be clearer to simply return it rather than using an output parameter. And then the callee can union it with another rctf if desired, rather than that being an implicit part of this function.

Since `rctf` is a fairly small plain-old-data struct, I think it would be clearer to simply return it rather than using an output parameter. And then the callee can union it with another `rctf` if desired, rather than that being an implicit part of this function.
nathanvegdahl marked this conversation as resolved
source/blender/editors/animation/anim_channels_edit.cc Outdated
@ -4271,0 +4383,4 @@
MEM_freeN(path);
for (FCurve *fcurve : fcurves) {
fcurve->flag |= (FCURVE_SELECTED | FCURVE_VISIBLE);
Nathan Vegdahl commented 2023-11-13 12:36:54 +01:00
Member
Copy Link

The name of this function led me to believe that it was side-effect free, but in fact it's unhiding the fcurves as well. So I'm wondering if it makes sense to split this out into functions that serve as clearer vocabulary. For example:

  1. A function that finds the relevant fcurves and returns a Vector of them.
  2. A function that computes the bounds of a vector of fcurves.
  3. A function that unhides a vector of fcurves.

And then you can call them individually. In fact, 3 is probably short enough to just inline rather than making a function for it.

The name of this function led me to believe that it was side-effect free, but in fact it's unhiding the fcurves as well. So I'm wondering if it makes sense to split this out into functions that serve as clearer vocabulary. For example: 1. A function that finds the relevant fcurves and returns a `Vector` of them. 2. A function that computes the bounds of a vector of fcurves. 3. A function that unhides a vector of fcurves. And then you can call them individually. In fact, 3 is probably short enough to just inline rather than making a function for it.
Christoph Lendenfeld commented 2023-11-14 11:31:37 +01:00
Author
Member
Copy Link

I am not sure I can do that.
To do the NLA mapping, I need the AnimData struct
to get that, I need the ID

If I split the functions to handle a vector of FCurve * I no longer know which ID each FCurve belongs to.

I could make a struct that encapsulates the FCurve * with the AnimData and make a Vector of that.
Or rename the function to something more appropriate.
Let me know what you think

I am not sure I can do that. To do the NLA mapping, I need the `AnimData` struct to get that, I need the `ID` If I split the functions to handle a vector of `FCurve *` I no longer know which ID each FCurve belongs to. I could make a struct that encapsulates the `FCurve *` with the `AnimData` and make a Vector of that. Or rename the function to something more appropriate. Let me know what you think
👍 1
Nathan Vegdahl commented 2023-11-16 13:04:27 +01:00
Member
Copy Link

Ah, that's a good point. Yeah, splitting this up could be tricky.

The functional programmer in me wants to split out the idea of filtering fcurves (i.e. finding the fcurves that satisfy a given criteria) from doing things with those fcurves (e.g. computing their total bounds, or unhiding them, etc.). But that would need to be part of a larger refactor of how a larger chunk of the animation code works, I think, and certainly doesn't belong in this PR even if we want to do it.

So yeah, for now let's just change the name. calculate_selection_fcurve_bounds_and_unhide is fine, I think. It's awkward, but I think that awkwardness is reflective of the multiple things the function is doing. And it's at least descriptive, so people won't accidentally reuse it thinking that all it does is compute bounds.

Ah, that's a good point. Yeah, splitting this up could be tricky. The functional programmer in me wants to split out the idea of filtering fcurves (i.e. finding the fcurves that satisfy a given criteria) from doing things with those fcurves (e.g. computing their total bounds, or unhiding them, etc.). But that would need to be part of a larger refactor of how a larger chunk of the animation code works, I think, and certainly doesn't belong in this PR even if we want to do it. So yeah, for now let's just change the name. `calculate_selection_fcurve_bounds_and_unhide` is fine, I think. It's awkward, but I think that awkwardness is reflective of the multiple things the function is doing. And it's at least descriptive, so people won't accidentally reuse it thinking that all it does is compute bounds.
Christoph Lendenfeld commented 2023-11-16 14:18:16 +01:00
Author
Member
Copy Link

good idea done that
at some point refactoring all the animation functions to check if they actually need AnimData would be good.
I am pretty sure we can simplify a lot of these functions

good idea done that at some point refactoring all the animation functions to check if they actually need `AnimData` would be good. I am pretty sure we can simplify a lot of these functions
👍 1
nathanvegdahl marked this conversation as resolved
Christoph Lendenfeld added 2 commits 2023-11-14 11:13:58 +01:00
Christoph Lendenfeld requested review from Nathan Vegdahl 2023-11-14 11:32:19 +01:00
Christoph Lendenfeld commented 2023-11-14 11:33:06 +01:00
Author
Member
Copy Link

@nathanvegdahl I implemented the changes, except for splitting the function which I am not sure how to best do.

@nathanvegdahl I implemented the changes, except for splitting the function which I am not sure how to best do.
Nathan Vegdahl commented 2023-11-14 12:42:49 +01:00
Member
Copy Link

Some notes from testing:

  • Over-all this is great!
  • In Blender you always have an active object/bone, which is what determines what properties are displayed in the UI (e.g. in the n-panel). This is true even when there are no selected objects/bones. Because of this, with this new operator you can easily run into a situation where you right click on a property and run this operator, but because nothing is selected it errors out. This seems fine to me, but the error message is "F-Curves have no valid size" which I think is pretty confusing. I think we should explicitly check for this case and give a more helpful error message. Maybe something like "No selected objects/bones: cannot focus f-curves"
  • When there are multiple graph editors open, it just uses the "first" one. I doubt having multiple graph editors open is common. But just in case, maybe there could be some kind of heuristic here, like picking the one with the largest window area. Not sure if that's actually worth it, though.

@Nika-Kutsniashvili

But most important for me is that if there is no Graph Editor in the workspace, it creates a pop-up window with Graph Editor. I know Blender doesn't like Pop-up editors, but it's the best way to quickly tweak animation without having to dive fully in Animation workspace when you just want slightest adjustment.

There actually is some precedence for this: in the right-click menu on a property, there's "Open Drivers Editor" which opens the full graph-editor-like drivers editor in a separate window. I don't know if that actually passes Blender's recent UI standards (maybe it's grandfathered in or something). But it's at least not unheard of.

Some notes from testing: - Over-all this is great! - In Blender you always have an active object/bone, which is what determines what properties are displayed in the UI (e.g. in the n-panel). This is true even when there are *no selected objects/bones*. Because of this, with this new operator you can easily run into a situation where you right click on a property and run this operator, but because nothing is selected it errors out. This seems fine to me, but the error message is "F-Curves have no valid size" which I think is pretty confusing. I think we should explicitly check for this case and give a more helpful error message. Maybe something like "No selected objects/bones: cannot focus f-curves" - When there are multiple graph editors open, it just uses the "first" one. I doubt having multiple graph editors open is common. But just in case, maybe there could be some kind of heuristic here, like picking the one with the largest window area. Not sure if that's actually worth it, though. @Nika-Kutsniashvili > But most important for me is that if there is no Graph Editor in the workspace, it creates a pop-up window with Graph Editor. I know Blender doesn't like Pop-up editors, but it's the best way to quickly tweak animation without having to dive fully in Animation workspace when you just want slightest adjustment. There actually is some precedence for this: in the right-click menu on a property, there's "Open Drivers Editor" which opens the full graph-editor-like drivers editor in a separate window. I don't know if that actually passes Blender's recent UI standards (maybe it's grandfathered in or something). But it's at least not unheard of.
👍 1
Christoph Lendenfeld added 1 commit 2023-11-14 13:09:39 +01:00
Christoph Lendenfeld added 1 commit 2023-11-14 14:12:28 +01:00
Christoph Lendenfeld commented 2023-11-14 14:16:45 +01:00
Author
Member
Copy Link

@nathanvegdahl changed the error message when nothing is selected to actually say "Nothing selected"

Also I realized that framing the fcurve of a shader node no longer worked, so I fixed that.

About which Graph Editor to choose when multiple are open. I am not sure picking the biggest one feels any less random. The only sensible way I could think of is to have an "active" graph editor. But that in itself complicates the matter a lot more

@nathanvegdahl changed the error message when nothing is selected to actually say "Nothing selected" Also I realized that framing the fcurve of a shader node no longer worked, so I fixed that. About which Graph Editor to choose when multiple are open. I am not sure picking the biggest one feels any less random. The only sensible way I could think of is to have an "active" graph editor. But that in itself complicates the matter a lot more
👍 1
hamza.SMA commented 2023-11-15 11:52:54 +01:00
First-time contributor
Copy Link

@nathanvegdahl
About which Graph Editor to choose when multiple are open. I am not sure picking the biggest one feels any less random. The only sensible way I could think of is to have an "active" graph editor. But that in itself complicates the matter a lot more

maybe picking 'the last graphe editor used' or 'the last created area that use graphe editor' ?

> @nathanvegdahl > About which Graph Editor to choose when multiple are open. I am not sure picking the biggest one feels any less random. The only sensible way I could think of is to have an "active" graph editor. But that in itself complicates the matter a lot more maybe picking 'the last graphe editor used' or 'the last created area that use graphe editor' ?
Nika Kutsniashvili commented 2023-11-15 12:07:53 +01:00
Contributor
Copy Link

@nathanvegdahl changed the error message when nothing is selected to actually say "Nothing selected"

Also I realized that framing the fcurve of a shader node no longer worked, so I fixed that.

About which Graph Editor to choose when multiple are open. I am not sure picking the biggest one feels any less random. The only sensible way I could think of is to have an "active" graph editor. But that in itself complicates the matter a lot more

Any method to choose Graph Editor in a smart way will require a lot of work and computation and will always feel random no matter what. I say just open it in every graph editor. Or just create pop-up in any case and not change graph editors in the workspace at all.

> @nathanvegdahl changed the error message when nothing is selected to actually say "Nothing selected" > > Also I realized that framing the fcurve of a shader node no longer worked, so I fixed that. > > About which Graph Editor to choose when multiple are open. I am not sure picking the biggest one feels any less random. The only sensible way I could think of is to have an "active" graph editor. But that in itself complicates the matter a lot more Any method to choose Graph Editor in a smart way will require a lot of work and computation and will always feel random no matter what. I say just open it in every graph editor. Or just create pop-up in any case and not change graph editors in the workspace at all.
Nathan Vegdahl commented 2023-11-16 12:43:52 +01:00
Member
Copy Link

@ChrisLend In retrospect, I don't think changing the graph editor choosing behavior is worth it. Certainly not in this PR, at least. If it's worth changing, we can revisit that in a future PR.

@ChrisLend In retrospect, I don't think changing the graph editor choosing behavior is worth it. Certainly not in this PR, at least. If it's worth changing, we can revisit that in a future PR.
Christoph Lendenfeld added 2 commits 2023-11-16 14:16:00 +01:00
Christoph Lendenfeld added 1 commit 2023-11-16 15:21:41 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
3343a3fc4d change logic of Alt key press 
now allows using "isolate" with a hotkey
Christoph Lendenfeld commented 2023-11-16 15:27:14 +01:00
Author
Member
Copy Link

@nathanvegdahl I had to modify the logic of how the Alt key press is detected.
As @Raymond-Luc pointed out to me, making a hotkey in the keymap that has "isolate" on true was not possible
because the invoke function was overriding that property. I had to make another property "use_modifier_key" that
specifies if the Alt key check is performed.

Though I am not sure that's the best way to do it. Feels a bit hacky

@nathanvegdahl I had to modify the logic of how the `Alt` key press is detected. As @Raymond-Luc pointed out to me, making a hotkey in the keymap that has "isolate" on true was not possible because the `invoke` function was overriding that property. I had to make another property "use_modifier_key" that specifies if the `Alt` key check is performed. Though I am not sure that's the best way to do it. Feels a bit hacky
Christoph Lendenfeld commented 2023-11-16 15:28:08 +01:00
Author
Member
Copy Link

@blender-bot package

@blender-bot package
Blender Bot commented 2023-11-16 15:28:10 +01:00
Member
Copy Link

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR114407) when ready.
Christoph Lendenfeld added this to the Animation & Rigging project 2023-11-16 16:40:27 +01:00
Nathan Vegdahl commented 2023-11-20 12:36:17 +01:00
Member
Copy Link

The code looks good to me. Although maybe taking some time to add documentation comments at the top of the longer new functions.

As @Raymond-Luc pointed out to me, making a hotkey in the keymap that has "isolate" on true was not possible
because the invoke function was overriding that property. I had to make another property "use_modifier_key" that
specifies if the Alt key check is performed.

Hmm. I think this is getting a bit outside of my wheelhouse. I'm also wondering now: is there any precedence for using modifiers to change the behavior of menu items elsewhere in Blender? Maybe we should run that by the UI module. @pablovazquez , could you weigh in on this?

To get this landed sooner rather than later, I think it might make sense to just rip out the alt-modifier stuff for now. Then we can add that back in a future PR after running it by the relevant people.

The code looks good to me. Although maybe taking some time to add documentation comments at the top of the longer new functions. > As @Raymond-Luc pointed out to me, making a hotkey in the keymap that has "isolate" on true was not possible because the invoke function was overriding that property. I had to make another property "use_modifier_key" that specifies if the Alt key check is performed. Hmm. I think this is getting a bit outside of my wheelhouse. I'm also wondering now: is there any precedence for using modifiers to change the behavior of menu items elsewhere in Blender? Maybe we should run that by the UI module. @pablovazquez , could you weigh in on this? To get this landed sooner rather than later, I think it might make sense to just rip out the alt-modifier stuff for now. Then we can add that back in a future PR after running it by the relevant people.
Christoph Lendenfeld commented 2023-11-21 10:19:30 +01:00
Author
Member
Copy Link

To get this landed sooner rather than later, I think it might make sense to just rip out the alt-modifier stuff for now. Then we can add that back in a future PR after it running it by the relevant people.

Good idea. With the operator property in place, this functionality can be added via the keymap if a users wants that

> To get this landed sooner rather than later, I think it might make sense to just rip out the alt-modifier stuff for now. Then we can add that back in a future PR after it running it by the relevant people. Good idea. With the operator property in place, this functionality can be added via the keymap if a users wants that
👍 1
Christoph Lendenfeld added 2 commits 2023-11-21 11:06:45 +01:00
Nathan Vegdahl approved these changes 2023-11-21 11:39:55 +01:00
Nathan Vegdahl left a comment
Member
Copy Link

Looks good to me!

Something else we might want to add in the future are an option for only selecting the curves without framing, and options for framing vertically vs horizontally. But I think it makes sense to hold off on those until we get actual requests or use cases.

Looks good to me! Something else we might want to add in the future are an option for only selecting the curves without framing, and options for framing vertically vs horizontally. But I think it makes sense to hold off on those until we get actual requests or use cases.
Christoph Lendenfeld added 1 commit 2023-11-21 13:53:27 +01:00
Christoph Lendenfeld merged commit a91a8f3fed into main 2023-11-21 14:07:03 +01:00
Christoph Lendenfeld deleted branch focus_in_ge 2023-11-21 14:07:05 +01:00
Nika Kutsniashvili commented 2023-11-21 15:23:34 +01:00
Contributor
Copy Link

I'm bit late, but Graph Editor icon next to operator in UI would be helpful

I'm bit late, but Graph Editor icon next to operator in UI would be helpful
pranay vaidya commented 2024-01-16 15:01:42 +01:00
First-time contributor
Copy Link

if there are lots of keyframes on one parameter, it would be a boon if there was a way to focus on only the one keyfame in the graph editor when the parameter is showing highlighted in yellow color.

if there are lots of keyframes on one parameter, it would be a boon if there was a way to focus on only the one keyfame in the graph editor when the parameter is showing highlighted in yellow color.
Christoph Lendenfeld commented 2024-01-16 15:37:56 +01:00
Author
Member
Copy Link

if there are lots of keyframes on one parameter, it would be a boon if there was a way to focus on only the one keyfame in the graph editor when the parameter is showing highlighted in yellow color.

Thanks for the feedback.
In the case of having a lot of keyframes on a curve you can enable the preview range and it will only zoom into that range.

But I could maybe add an option to the operator to not change the x-range of the Graph Editor.

> if there are lots of keyframes on one parameter, it would be a boon if there was a way to focus on only the one keyfame in the graph editor when the parameter is showing highlighted in yellow color. Thanks for the feedback. In the case of having a lot of keyframes on a curve you can enable the preview range and it will only zoom into that range. But I could maybe add an option to the operator to not change the x-range of the Graph Editor.
👍 1 🎉 1
pranay vaidya commented 2024-02-01 11:35:44 +01:00
First-time contributor
Copy Link

if there are lots of keyframes on one parameter, it would be a boon if there was a way to focus on only the one keyfame in the graph editor when the parameter is showing highlighted in yellow color.

Thanks for the feedback.
In the case of having a lot of keyframes on a curve you can enable the preview range and it will only zoom into that range.

But I could maybe add an option to the operator to not change the x-range of the Graph Editor.

I wish governments had passionate people like you, the world would have been come together just like Blender

> > if there are lots of keyframes on one parameter, it would be a boon if there was a way to focus on only the one keyfame in the graph editor when the parameter is showing highlighted in yellow color. > > Thanks for the feedback. > In the case of having a lot of keyframes on a curve you can enable the preview range and it will only zoom into that range. > > But I could maybe add an option to the operator to not change the x-range of the Graph Editor. I wish governments had passionate people like you, the world would have been come together just like Blender
😆 1
Jonathan Lampel commented 2024-02-14 23:00:35 +01:00
First-time contributor
Copy Link

In testing this out with the latest builds, I'm not able to frame any animated material properties, node or otherwise (like Base Color or Viewport Display Color). This is also true of world settings (like Mist Pass Start) and scene settings (like Gravity or Audio Volume). They all just give the 'Nothing Selected' error. It's possible to select those things via the outliner I suppose but that doesn't make a difference. Not sure if all that was in scope for this patch so I wasn't sure if I should open a new issue about it or not.

In testing this out with the latest builds, I'm not able to frame any animated material properties, node or otherwise (like Base Color or Viewport Display Color). This is also true of world settings (like Mist Pass Start) and scene settings (like Gravity or Audio Volume). They all just give the 'Nothing Selected' error. It's possible to select those things via the outliner I suppose but that doesn't make a difference. Not sure if all that was in scope for this patch so I wasn't sure if I should open a new issue about it or not.
Campbell Barton commented 2024-02-15 01:23:48 +01:00
Owner
Copy Link

Noticed issues with this PR.
The context is modified but not restored & there was a memory leak in the selection list (resolved 7848533395).

Overriding the context without restoring it is could break event handling logic, although in this case it didn't seem to cause user visible errors, if you check existing uses of CTX_wm_window_set, the convention is to restore the previous window after setting. Unless they actually manipulate the windowing data (closing the window for e.g.).

As far as I can see this isn't documented though, doc-strings in source/blender/blenkernel/BKE_context.hh should mention this,
https://developer.blender.org/docs/features/core/context/ too.

Noticed issues with this PR. The context is modified but not restored & there was a memory leak in the `selection` list (resolved 78485333957927e52709e40206af5a526d70d60b). Overriding the context without restoring it is could break event handling logic, although in this case it didn't seem to cause user visible errors, if you check existing uses of `CTX_wm_window_set`, the convention is to restore the previous window after setting. Unless they actually manipulate the windowing data (closing the window for e.g.). As far as I can see this isn't documented though, doc-strings in `source/blender/blenkernel/BKE_context.hh` should mention this, https://developer.blender.org/docs/features/core/context/ too.
Christoph Lendenfeld commented 2024-02-15 09:38:52 +01:00
Author
Member
Copy Link

@ideasman42 do I see this right in the commit that the memory leak occurred when the code exited early because it was unable to find the graph editor? Thanks for fixing that and the context move code.

@JonathanLampel-4 make sure you select the node before calling the operator. I'll mention that in the release notes to avoid any confusion. For the mist settings, indeed that seems to not work. I'll see if I can make a fix for that.

@ideasman42 do I see this right in the commit that the memory leak occurred when the code exited early because it was unable to find the graph editor? Thanks for fixing that and the context move code. @JonathanLampel-4 make sure you select the node before calling the operator. I'll mention that in the release notes to avoid any confusion. For the mist settings, indeed that seems to not work. I'll see if I can make a fix for that.
👍 1
Sign in to join this conversation.
Reviewers
No reviewers
Nathan Vegdahl
Labels
Clear labels   
Interest
Alembic

  
Interest
Animation & Rigging

  
Interest
Asset Browser

  
Interest
Asset Browser Project Overview

  
Interest
Audio

  
Interest
Automated Testing

  
Interest
Blender Asset Bundle

  
Interest
BlendFile

  
Interest
Collada

  
Interest
Compatibility
This issue affects/is about backward or forward compatibility

  
Interest
Compositing

  
Interest
Core

  
Interest
Cycles

  
Interest
Dependency Graph

  
Interest
Development Management

  
Interest
EEVEE

  
Interest
EEVEE & Viewport

  
Interest
Freestyle

  
Interest
Geometry Nodes

  
Interest
Grease Pencil

  
Interest
ID Management

  
Interest
Images & Movies

  
Interest
Import Export

  
Interest
Line Art

  
Interest
Masking

  
Interest
Metal

  
Interest
Modeling

  
Interest
Modifiers

  
Interest
Motion Tracking

  
Interest
Nodes & Physics

  
Interest
OpenGL

  
Interest
Overlay

  
Interest
Overrides

  
Interest
Performance

  
Interest
Physics

  
Interest
Pipeline, Assets & IO

  
Interest
Platforms, Builds & Tests

  
Interest
Python API

  
Interest
Render & Cycles

  
Interest
Render Pipeline

  
Interest
Sculpt, Paint & Texture

  
Interest
Text Editor

  
Interest
Translations

  
Interest
Triaging

  
Interest
Undo

  
Interest
USD

  
Interest
User Interface

  
Interest
UV Editing

  
Interest
VFX & Video

  
Interest
Video Sequencer

  
Interest
Virtual Reality

  
Interest
Vulkan

  
Interest
Wayland
Issues relating to Wayland windowing support.

  
Interest
Workbench

  
Interest: X11

Issues relating to Xorg/X11 support.

  
Legacy
Blender 2.8 Project

  
Legacy
Milestone 1: Basic, Local Asset Browser

  
Legacy
OpenGL Error

  
Meta
Good First Issue

  
Meta
Papercut

  
Meta
Retrospective

  
Meta
Security
Issues relating to security: https://wiki.blender.org/wiki/Process/Vulnerability_Reports

  
Module
Animation & Rigging

  
Module
Core

  
Module
Development Management

  
Module
EEVEE & Viewport

  
Module
Grease Pencil

  
Module
Modeling

  
Module
Nodes & Physics

  
Module
Pipeline, Assets & IO

  
Module
Platforms, Builds & Tests

  
Module
Python API

  
Module
Render & Cycles

  
Module
Sculpt, Paint & Texture

  
Module
Triaging

  
Module
User Interface

  
Module
VFX & Video

  
Platform
FreeBSD

  
Platform
Linux

  
Platform
macOS

  
Platform
Windows

  
Priority
High

  
Priority
Low

  
Priority
Normal

  
Priority
Unbreak Now!

  
Status
Archived

  
Status
Confirmed

  
Status
Duplicate

  
Status
Needs Info from Developers

  
Status
Needs Information from User

  
Status
Needs Triage

  
Status
Resolved

  
Type
Bug

  
Type
Design

  
Type
Known Issue

  
Type
Patch

  
Type
Report

  
Type
To Do

No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Animation & Rigging
Assignees
Clear assignees
No Assignees
11 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#114407
No description provided.
Delete Branch "ChrisLend/blender:focus_in_ge"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?