Animation: Improve drawing of locked FCurves #106052

Christoph Lendenfeld · 2023-03-23T17:36:56+01:00

Christoph Lendenfeld commented

2023-03-23 17:36:56 +01:00

Locked FCurves had 2 visual issues

the dashing is so short it just creates visual noise
keyframes are drawn in white looking like they are selected

This PR changes this by

Increasing the dash width
Keyframes are drawn in black on locked curves
To indicate that they can't be selected, draw them in a X shape
To further reduce visual noise, locked curves no longer draw thicker when selected

Before

After

Follow up tasks
The drawing code seems to add points to the line strip at constant intervals irrespective of the viewport zoom. This is wasteful on the one hand and also creates the issue with dashed lines that the dashes merge into a line if zoomed out.
Ideally the points are at roughly even distance in screen space.

Design Task

Part of the changes discussed in this design task
#104867: Design: Graph Editor Curve Display

Locked `FCurves` had 2 visual issues * the dashing is so short it just creates visual noise * keyframes are drawn in white looking like they are selected This PR changes this by * Increasing the dash width * Keyframes are drawn in black on locked curves * To indicate that they can't be selected, draw them in a X shape * To further reduce visual noise, locked curves no longer draw thicker when selected Before ![image](/attachments/c706ea1b-42ba-43f1-aa82-a61b6d3ae013) After ![image](/attachments/b2f671df-f22c-4fb1-80e9-596c84f75dc4) ------ **Follow up tasks** The drawing code seems to add points to the line strip at constant intervals irrespective of the viewport zoom. This is wasteful on the one hand and also creates the issue with dashed lines that the dashes merge into a line if zoomed out. Ideally the points are at roughly even distance in screen space. ### Design Task Part of the changes discussed in this design task [#104867: Design: Graph Editor Curve Display](https://projects.blender.org/blender/blender/issues/104867)

image.png

19 KiB

image.png

20 KiB

image.png

16 KiB

image.png

16 KiB

image.png

15 KiB

Christoph Lendenfeld added the

Module

Animation & Rigging

label 2023-03-23 17:36:56 +01:00

Christoph Lendenfeld added 4 commits 2023-03-23 17:37:06 +01:00

fcurve drawing changes ec18a8799e

Merge branch 'main' into fcurve_locked_drawing edfe7dfbc9

fix change to U.scale_factor 5775ae95a7

draw x for keys on locked curves 01eb147c0d

Christoph Lendenfeld referenced this pull request

2023-03-23 17:42:09 +01:00

Design: Graph Editor Curve Display #104867

Christoph Lendenfeld referenced this pull request

2023-03-23 17:42:46 +01:00

Design: Graph Editor Curve Display #104867

Sybren A. Stüvel added this to the Animation & Rigging project 2023-03-30 17:44:16 +02:00

Christoph Lendenfeld added 4 commits 2023-06-02 12:04:10 +02:00

Merge branch 'main' into fcurve_locked_drawing 2db59fc232

Merge branch 'main' into fcurve_locked_drawing 06d3c58c03

Merge branch 'main' into fcurve_locked_drawing 1c82145fed

change dash widthjand refactor function name eb1b55b376

Christoph Lendenfeld added 1 commit 2023-06-02 12:13:23 +02:00

change dash width to 0.35 190b9c3de7

Christoph Lendenfeld changed title from ~~WIP: Animation: Improve drawing of locked FCurves~~ to Animation: Improve drawing of locked FCurves

2023-06-02 12:13:52 +02:00

Christoph Lendenfeld requested review from Sybren A. Stüvel 2023-06-02 12:14:00 +02:00

Sybren A. Stüvel requested changes 2023-06-22 11:07:18 +02:00

Sybren A. Stüvel left a comment

In the 'Before' image I see three curves, but two of them are no longer visible in the 'After' image. The PR description doesn't mention anything about not drawing certain curves any more, though.

This PR definitely improves things, but I also see a glitch. This is with the Hi my name is Amy demo file:

Before:

After:

In the 'Before' situation I really don't like the attention-grabbing white keys (those are the locked ones), but the vertical lines in the 'After' seem to be wrong.

In the 'Before' image I see three curves, but two of them are no longer visible in the 'After' image. The PR description doesn't mention anything about not drawing certain curves any more, though. This PR definitely improves things, but I also see a glitch. This is with the [Hi my name is Amy](https://studio.blender.org/characters/5f1ed640e9115ed35ea4b3fb/showcase/1/) demo file: Before: ![image](/attachments/477adaba-4987-4df4-81d8-794cec9cfa3b) After: ![image](/attachments/4dc480d5-cbbc-4766-acdc-80410db6b047) In the 'Before' situation I really don't like the attention-grabbing white keys (those are the locked ones), but the vertical lines in the 'After' seem to be wrong.

image.png

157 KiB

image.png

97 KiB

source/blender/editors/space_graph/graph_draw.c Outdated

						
				@ -205,6 +220,31 @@ static void draw_fcurve_selected_keyframe_vertices(

				  immEnd();

				}

				static void draw_locked_keyframe_vertices(FCurve *fcu, View2D *v2d, uint attr_id)

Sybren A. Stüvel commented

2023-06-22 10:56:29 +02:00

const can be added for at least the attr_id. fcu could probably also be const, but that would have to ripple to other functions, which IMO is not worth it for this PR.

`const` can be added for at least the `attr_id`. `fcu` could probably also be `const`, but that would have to ripple to other functions, which IMO is not worth it for this PR.

Christoph Lendenfeld added 1 commit 2023-07-07 12:34:21 +02:00

Merge branch 'main' into fcurve_locked_drawing 964d1f3e8f

Christoph Lendenfeld added 1 commit 2023-07-07 14:35:46 +02:00

fix euler rotations unit scale 83f8a0a63f

Christoph Lendenfeld commented

2023-07-07 14:37:12 +02:00

@dr.sybren thanks for finding that issue
I learned that Euler rotations are stored as radians internally and are just displayed as degrees in the editor

I had to pass through the unit scale factor to normalize the cross size with it

@dr.sybren thanks for finding that issue I learned that Euler rotations are stored as radians internally and are just displayed as degrees in the editor I had to pass through the unit scale factor to normalize the cross size with it

🚀 1

Christoph Lendenfeld requested review from Sybren A. Stüvel 2023-07-07 14:37:20 +02:00

Sybren A. Stüvel reviewed 2023-07-07 14:55:13 +02:00

source/blender/editors/space_graph/graph_draw.c Outdated

						
				@ -208,0 +234,4 @@

				  scale[0] /= vertex_size;

				  /* Dividing by the unit scale is needed to display euler correctly (internally they are radians

				   * but displayed as degrees) and all curves when normalization is turned on. */

				  scale[1] /= vertex_size / unit_scale;

Sybren A. Stüvel commented

2023-07-07 14:55:13 +02:00

This is the same as:

scale[1] = scale[1] / (vertex_size / unit_scale);

which is the same as:

scale[1] = scale[1] * unit_scale / vertex_size;

which would remove a division from the computations. And since multiplication is faster than division, give a minute performance gain.

I'll leave it up to you to decide on which one you want (slightly longer but faster code, vs. slightly shorter and likely just as fast because the compiler can optimize). I don't have any strong feelings here.

This is the same as: ``` scale[1] = scale[1] / (vertex_size / unit_scale); ``` which is the same as: ``` scale[1] = scale[1] * unit_scale / vertex_size; ``` which would remove a division from the computations. And since multiplication is faster than division, give a minute performance gain. I'll leave it up to you to decide on which one you want (slightly longer but faster code, vs. slightly shorter and likely just as fast because the compiler can optimize). I don't have any strong feelings here.

Sybren A. Stüvel approved these changes 2023-07-07 14:55:47 +02:00

Sybren A. Stüvel left a comment

LGTM, just one inline note that I'll leave to you to decide on before landing the PR.

Christoph Lendenfeld added 1 commit 2023-07-07 15:02:26 +02:00

change scale[1] to remove a division 12d7433b4d

Christoph Lendenfeld commented

2023-07-07 15:04:34 +02:00

thanks for the review. I changed the line
might be minor, but faster = yes

thanks for the review. I changed the line might be minor, but faster = yes

👍 1

Christoph Lendenfeld merged commit 6332d1b8a8 into main

2023-07-07 15:05:58 +02:00

Christoph Lendenfeld referenced this issue from a commit

2023-07-07 15:05:59 +02:00

Animation: Improve drawing of locked FCurves

Christoph Lendenfeld deleted branch fcurve_locked_drawing

2023-07-07 15:06:00 +02:00

Pierrick PICAUT commented

2023-07-07 15:30:12 +02:00

Hi,

Do you have any possibility to play with line thickness and depth.
I believe curves are ordered in the graph editor as they are in the left panel.

But, whatever design, if the active or unlocked curve where shown "in front" that would probably already help.

Also, I can't agree more with the fact that white dotes for locked keys is not relevent and inconsistent with blender's other contexts/assets

Generaly a locked object/information may loose saturation, opacity, be dashed or use alpha hashed.

In the action editor/dopesheet/timeline, locked keys are transparent
If you "lock" the selectability of an object in the outliner, the select icon is outlined instead of solid

The problem is that curve handles are displayed as an outlined key.
Maybe handles could be changed in diamond shapes or something else so that we could use an outlined circle as a locked key.

Sorry if this is confusing.

Hi, Do you have any possibility to play with line thickness and depth. I believe curves are ordered in the graph editor as they are in the left panel. But, whatever design, if the active or unlocked curve where shown "in front" that would probably already help. Also, I can't agree more with the fact that white dotes for locked keys is not relevent and inconsistent with blender's other contexts/assets Generaly a locked object/information may loose saturation, opacity, be dashed or use alpha hashed. In the action editor/dopesheet/timeline, locked keys are transparent If you "lock" the selectability of an object in the outliner, the select icon is outlined instead of solid The problem is that curve handles are displayed as an outlined key. Maybe handles could be changed in diamond shapes or something else so that we could use an outlined circle as a locked key. Sorry if this is confusing.

02.jpg

70 KiB

Christoph Lendenfeld commented

2023-07-07 15:54:32 +02:00

@P2design yeah sorry not sure what you mean. If I understand correctly this is more about how to draw handles. And I agree changing that is also a good idea, but possible more related to the design task here
#104867: Design: Graph Editor Curve Display

@P2design yeah sorry not sure what you mean. If I understand correctly this is more about how to draw handles. And I agree changing that is also a good idea, but possible more related to the design task here [#104867: Design: Graph Editor Curve Display](https://projects.blender.org/blender/blender/issues/104867)

Pierrick PICAUT commented

2023-07-07 16:06:03 +02:00

@ChrisLend indeed.
Sorry about that and I see that the new design has already been well thought

@ChrisLend indeed. Sorry about that and I see that the new design has already been well thought

Andras-Borsanyi referenced this issue from a commit

2023-07-12 17:32:37 +02:00

Animation: Improve drawing of locked FCurves

Vyacheslav Kobozev commented

2023-08-01 14:59:44 +02:00

First-time contributor

Hi, i have issues with dashes.

Hi, i have issues with dashes. <video src="/attachments/d0a28cb1-fd69-49df-b5bf-f426b2c4c254" title="2023-08-01_15-55-53.mp4" controls></video>

2023-08-01_15-55-53.mp4

6.5 MiB

Vyacheslav Kobozev commented

2023-08-01 15:03:43 +02:00

First-time contributor

Also i suggest to try half-filled circles for locked nodes. Just as very minimalistic lock pictogram. Х is more fore deletion or solid restriction.

Sybren A. Stüvel commented

2023-09-04 14:22:24 +02:00

@Vyach Thanks for testing the PR. In the future, please write your response in text, rather than attaching a video. Videos are great for visual aid, but developers can read text really quickly (it's all we do all day long). Compared to reading a few well-written sentences, a 1½ minute video is quite long. More importantly, whatever you say in the video cannot be searched for, so any information you give is easily lost -- if it's written down, the search engine indexes it, and things can be found later again. It's just a way to make sure your contribution is used to its maximum. And also not all developers have headphones on all the time, so your voiceover may actually go unnoticed until the video starts to make no sense.

If this sounds overly negative, my apologies. It's just meant as a friendly perspective from the other side of the bug tracker. Your help is certainly valued.

@Vyach Thanks for testing the PR. In the future, please write your response in text, rather than attaching a video. Videos are great for visual aid, but developers can read text **really** quickly (it's all we do all day long). Compared to reading a few well-written sentences, a 1½ minute video is quite long. More importantly, whatever you say in the video cannot be searched for, so any information you give is easily lost -- if it's written down, the search engine indexes it, and things can be found later again. It's just a way to make sure your contribution is used to its maximum. And also not all developers have headphones on all the time, so your voiceover may actually go unnoticed until the video starts to make no sense. If this sounds overly negative, my apologies. It's just meant as a friendly perspective from the other side of the bug tracker. Your help is certainly valued.

Christoph Lendenfeld referenced this pull request

2023-09-05 17:21:12 +02:00

Animation: Graph Editor locked key drawing #111986

Christoph Lendenfeld referenced this issue from a commit

2023-09-14 15:09:16 +02:00

Animation: Graph Editor locked key drawing

Sign in to join this conversation.

No reviewers