Fix: View in Graph Editor not working when object is unselected #118658

Christoph Lendenfeld · 2024-02-23T12:15:23+01:00

2024-02-23 12:15:23 +01:00

The new operator "View in Graph Editor" (#114407) only looked at the selection
when generating the FCurve bounds.

This failed in certain cases if things are not selected, or not selectable.
The simplest example is to key the emission strength of the world settings and
try to view that.
This was mentioned by Jonathan Lampel on the PR: #114407 (comment)

The fix is to also resolve the bounds of the animation data on the button.
This means for a few FCurves the bounds might be calculated twice but it
doesn't change the overall result.

This has another side effect though:
The Graph Editor might zoom into an area where there is no FCurve
visible because the thing is not selected so it isn't shown.
This issue existed before if filters were applied, but is now more pronounced
since by default the Graph Editor doesn't show unselected objects.

A warning is raised when at least one of the FCurves that are viewed is not
visible with the current filter settings. The view will still zoom into that area.

This PR extracts the function get_fcurves_of_property from calculate_selection_fcurve_bounds_and_unhide.
That way it can be passed into count_fcurves_hidden_by_filter as well.

For the review:
I am not sure using a blender::Map is the right datastructure for comparing two Arrays.
Basically I just need a fast way to see if an FCurve is in an Array of FCurves. I am only using the keys of the Map for this.

The new operator "View in Graph Editor" (#114407) only looked at the selection when generating the FCurve bounds. This failed in certain cases if things are not selected, or not selectable. The simplest example is to key the emission strength of the world settings and try to view that. This was mentioned by Jonathan Lampel on the PR: https://projects.blender.org/blender/blender/pulls/114407#issuecomment-1123913 The fix is to also resolve the bounds of the animation data on the button. This means for a few FCurves the bounds might be calculated twice but it doesn't change the overall result. This has another side effect though: The Graph Editor might zoom into an area where there is no FCurve visible because the thing is not selected so it isn't shown. This issue existed before if filters were applied, but is now more pronounced since by default the Graph Editor doesn't show unselected objects. A warning is raised when at least one of the FCurves that are viewed is not visible with the current filter settings. The view will still zoom into that area. -------- This PR extracts the function `get_fcurves_of_property` from `calculate_selection_fcurve_bounds_and_unhide`. That way it can be passed into `count_fcurves_hidden_by_filter` as well. For the review: I am not sure using a `blender::Map` is the right datastructure for comparing two Arrays. Basically I just need a fast way to see if an FCurve is in an Array of FCurves. I am only using the keys of the `Map` for this.

Christoph Lendenfeld added this to the 4.1 milestone 2024-02-23 12:15:23 +01:00

Christoph Lendenfeld added the

Module

Animation & Rigging

label 2024-02-23 12:15:23 +01:00

Christoph Lendenfeld added 1 commit 2024-02-23 12:15:24 +01:00

ef20c401ef the fix

Christoph Lendenfeld changed title from ~~Fix: View in Graph Editor not working with some properties~~ to Fix: View in Graph Editor not working when object is unselected

2024-02-23 12:17:25 +01:00

Nathan Vegdahl commented

2024-02-27 15:07:40 +01:00

Option 1 to fix it would be to change the selection, which we definitely don't want to mess with.
Option 2 would be to change the filter settings, but this is also very intrusive.
Option 3 is to ignore bounds if the FCurve isn't visible under the current filter settings. This adds
a lot of complexity for very little benefit since the user has to change the settings anyway, and
then call the operator again.

Hmm... I'm not actually sure if this PR is the right way to do it vs these other options. I think(?) what I would expect as a user is:

If the f-curve is simply hidden on its own (via the curve's visibility property) it gets unhidden and shown.
If the f-curve is invisible due to filtering, I get an error message like "Cannot show in graph editor: this curve is currently hidden due to filter settings".

(Incidentally, case 1 is already handled that way, which is great. However, there's a bug: if the channel group is also hidden, the group does not also get toggled to be visible as it does when manually toggling a channel's visibility as a user, and thus the f-curve is still hidden. Separate issue, though.)

> Option 1 to fix it would be to change the selection, which we definitely don't want to mess with. > Option 2 would be to change the filter settings, but this is also very intrusive. > Option 3 is to ignore bounds if the FCurve isn't visible under the current filter settings. This adds > a lot of complexity for very little benefit since the user has to change the settings anyway, and > then call the operator again. Hmm... I'm not actually sure if this PR is the right way to do it vs these other options. I think(?) what I would expect as a user is: 1. If the f-curve is simply hidden on its own (via the curve's visibility property) it gets unhidden and shown. 2. If the f-curve is invisible due to filtering, I get an error message like "Cannot show in graph editor: this curve is currently hidden due to filter settings". (Incidentally, case 1 is already handled that way, which is great. However, there's a bug: if the channel group is also hidden, the group does not also get toggled to be visible as it does when manually toggling a channel's visibility as a user, and thus the f-curve is still hidden. Separate issue, though.)

Christoph Lendenfeld commented

2024-02-27 16:19:16 +01:00

If the f-curve is invisible due to filtering, I get an error message like "Cannot show in graph editor: this curve is currently hidden due to filter settings".

My argument against that is that it is slower because it adds one step into the process.

run operator, change filter settings, run operator again
vs: run operator, change filter settings

What I think would be better is to only throw a warning but still do the zoom. That way the user knows why they don't see something and can do something about it if they so choose. Still, that would leave the challenge of talking to the filtering code to see if an FCurve can be visible atm...

I will put it on the agenda to see what people think.

> 2. If the f-curve is invisible due to filtering, I get an error message like "Cannot show in graph editor: this curve is currently hidden due to filter settings". My argument against that is that it is slower because it adds one step into the process. * run operator, change filter settings, run operator again * vs: run operator, change filter settings What I think would be better is to only throw a warning but still do the zoom. That way the user knows why they don't see something and can do something about it if they so choose. Still, that would leave the challenge of talking to the filtering code to see if an FCurve can be visible atm... I will put it on the agenda to see what people think.

Nathan Vegdahl commented

2024-02-29 11:40:28 +01:00

What I think would be better is to only throw a warning but still do the zoom.

Yeah, I like that. Best of both worlds.

> What I think would be better is to only throw a warning but still do the zoom. Yeah, I like that. Best of both worlds.

Christoph Lendenfeld commented

2024-03-05 09:59:14 +01:00

this was discussed in the Animation & Rigging module meeting. The consensus was to zoom to the place but give a warning if the curve isn't visible

this was discussed in the [Animation & Rigging](https://devtalk.blender.org/t/2024-02-29-animation-rigging-module-meeting/33614#patches-review-decision-time-5) module meeting. The consensus was to zoom to the place but give a warning if the curve isn't visible

Christoph Lendenfeld added 2 commits 2024-03-05 16:46:37 +01:00

e67a7e9c3f Merge branch 'blender-v4.1-release' into fix_view_in_ge

8179525320 provide warning when viewing an FCurve that isnt visible

Christoph Lendenfeld requested review from Sybren A. Stüvel 2024-03-05 16:49:28 +01:00

Sybren A. Stüvel commented

2024-03-05 17:04:04 +01:00

It still doesn't work quite well for me. When I animate the world viewport color, and right-click, 'View in Graph Editor', I get the error Error: F-Curves have no valid size. I've attached the file I tested with.

It still doesn't work quite well for me. When I animate the world viewport color, and right-click, 'View in Graph Editor', I get the error `Error: F-Curves have no valid size`. I've attached the file I tested with.

118658-view-fcurve-unselectable-thing.blend

106 KiB

Christoph Lendenfeld added 1 commit 2024-03-05 17:36:04 +01:00

482725956c fix issue when viewing color properties

Christoph Lendenfeld commented

2024-03-05 17:39:16 +01:00

@dr.sybren fixed by adding this line
const bool whole_array = RNA_boolean_get(op->ptr, "all") || index < 0;

it seems that index can be lower than 0 on e.g. color properties that are an array, but the array is not exposed as individual fields

@dr.sybren fixed by adding this line `const bool whole_array = RNA_boolean_get(op->ptr, "all") || index < 0;` it seems that `index` can be lower than 0 on e.g. color properties that are an array, but the array is not exposed as individual fields

Christoph Lendenfeld added 1 commit 2024-03-07 09:30:55 +01:00