Animation: Improve drawing of locked FCurves #106052

Merged
Christoph Lendenfeld merged 12 commits from ChrisLend/blender:fcurve_locked_drawing into main 2023-07-07 15:05:58 +02: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
image

After
image


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)
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
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
Christoph Lendenfeld added 1 commit 2023-06-02 12:13:23 +02:00
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
Member

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:
image

After:
image

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.
@ -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)

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
Christoph Lendenfeld added 1 commit 2023-07-07 14:35:46 +02:00
Author
Member

@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
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
@ -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;

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
Member

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

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
Author
Member

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
Christoph Lendenfeld merged commit 6332d1b8a8 into main 2023-07-07 15:05:58 +02:00
Christoph Lendenfeld deleted branch fcurve_locked_drawing 2023-07-07 15:06:00 +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.
70 KiB
Author
Member

@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)

@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
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>
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.

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

@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.
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser Project (Legacy)
Interest
Asset System
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
No Milestone
No Assignees
4 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#106052
No description provided.