Animation: Graph Editor curve drawing performance improvement #110301
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
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
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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 project
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#110301
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "ChrisLend/blender:graph_draw_performance"
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?
In current blender, when drawing curves in the Graph Editor,
it always iterates over the whole curve.
From first to last vertex no matter what is shown on screen.
Instead of that, find the bounding keyframes and iterate within them.
Additionally to that, break apart the logic into two sections
float2
into aVector
The second optimization is to make the Bezier resolution
dependent on the keyframe distance in screenspace.
Previously it just went off the distance between keys in frames,
but that doesn't make sense if you are zoomed out.
Performance Numbers
The following performance numbers are the
average time per curve on a mocap file of 6000f (each frame has a key).
The numbers were generated without showing keyframes to only focus
on the performance of this patch.
Also they are from a debug build, so they just make sense relative to each other
This only affects the actual curves.
Keyframe and handle drawing can likely still be improved
Future improvement ideas
draw_fcurve_curve_bezts
anddraw_fcurve_curve
. That is because the former doesn't deal with all key types currently, so the latter is called which samples the fcurve. But I think we can implement a fallback indraw_fcurve_curve_bezts
that uses samples if needed. Then we'd save a check that potentially iterates the whole fcurve:fcurve_can_use_simple_bezt_drawing
Video Demo
WIP: Animation: Graph Editor draw performance improvementto WIP: Animation: Graph Editor curve drawing performance improvementWIP: Animation: Graph Editor curve drawing performance improvementto Animation: Graph Editor curve drawing performance improvementNice work!
There's something going wrong with the 'Hello, my name is Amy' scene though:
@ -876,0 +868,4 @@
const int resolution_x = (int)((bezt->vec[1][0] - prevbezt->vec[1][0]) * resolution_scale[0]);
const int resolution_y = (int)(fabs(bezt->vec[1][1] - prevbezt->vec[1][1]) *
resolution_scale[1]);
return resolution_x + resolution_y;
What's the idea behind adding the two together? Would be good to have a comment that explains.
I think it might make sense to do this differently anyway. In particular, this doesn't take into account the handles of the f-curve segment. For example, you could have two keys that are very close together, but with handles that extend very far vertically, creating a tall narrow hump (like in this image). Drawing that accurately requires a lot more resolution than just the key frame distance would suggest.
I think the simplest robust thing to do would be to take the bounding box of the segment's four control points (i.e. the keys and inner handles) and base the resolution on the maximum dimension of that bounding box in viewport space.
I also suspect we'll want to keep a maximum resolution, and maybe add a minimum resolution. But they can probably just be hard-coded rather than being a parameter.The maximum resolution is useful because in practice beyond a certain number of samples you're very unlikely to visually approximate the cubic curve any better at any "reasonable" scale. And it would help prevent unexpected performance drops when a really long/tall segment just barely peeks into the viewport range. Blender's 3d bezier curves cap the resolution at a maximum of 1024, so maybe use the same here. Although probably just 256 or even 128 would be reasonable.The minimum resolution could be as low as 2 points, just to avoid accidentally doing something degenerate. But I think 4 points is a good default, since that way it can always at least reflect the possible points of inflection of the cubic curve.Re: maximums and minimums, never mind. It looks like those are in
add_bezt_vertices()
, which probably makes more sense.how about I move the call to
calculate_bezt_draw_resolution
intoadd_bezt_vertices
that way it is in the same place as the clamping happens. I would then just pass the
float2 resolution_scale
toadd_bezt_vertices
regarding the bounding box including the handles, good idea. Will add that
I think I slightly prefer the call being in
draw_fcurve_curve_bezts()
as it is now. It keeps things a little "flatter" (you aren't chasing functions that call functions that call functions).But I don't think it matters much either way. In this case, my mis-comment was just me being a goof ball and not reading the whole PR before submitting my comments.
added a comment and included the handles in the calculations
@ -883,0 +878,4 @@
static void add_bezt_vertices(BezTriple *bezt,
BezTriple *prevbezt,
int resolution,
blender::Vector<blender::float2> &curve_vertices)
Add a comment that explains that the vertices are added to
curve_vertices
.@ -894,0 +892,4 @@
float prev_key[2], prev_handle[2], bez_handle[2], bez_key[2];
/* Allocation needs +1 on resolution because BKE_curve_forward_diff_bezier uses it to iterate
* inclusively. */
float *data = static_cast<float *>(
What is
data
?I kept the name from how it was previously
but good point. very non telling name. changed it
@ -919,0 +939,4 @@
return {first, last};
}
static void get_extrapolation_point_left(FCurve *fcu,
This is already much of an improvement over having this functionality inline as part of some other code. I'm still amazed that this was part of the drawing code, though -- it's likely to be copied into other areas as well. Let's refactor that some other time ;-)
I'm not sure I agree. (I've used a lot of words below, but I don't actually feel strongly about this, so feel free to ignore. I just wanted to present a different point of view in case it's useful.)
If it returned a point + slope pair (or something similar) which could be used for arbitrary evaluation of the extrapolated line then I could see it being used elsewhere (e.g. when evaluating f-curves for animation). And it could certainly make sense to break that part of the code out into a function that can be used elsewhere, to ensure that extrapolation evaluation is consistent everywhere. But as-is this function computes and adds a viewport-bound point to an in-construction list of evaluated f-curve points, which seems pretty narrow in scope and unlikely to be used elsewhere.
As an out-of-context stand-alone function it also left me scratching my head a bit trying to figure out what it was for, and it wasn't until I looked at the call site that its purpose became clear. And similarly, I think if I had started at the call site, I would have needed to come read the code of this function to understand what was happening as well. That could just be me, of course. But to me that's usually a sign to leave something inline (unless you already have another place to use it, at least).
@nathanvegdahl I think a big chunk of your confusion comes from C/C++ being old-fasioned, and functions needing either a declaration or their definition before they can be used. This means that often a function is first seen out of context (when reading a file from top to bottom) and only then do you see where it was used in its higher-level context.
Still, IMO it's good to have this code in a separate function.
That's fair. I think we have a bit different sensibilities here, but that's fine. And as I said, I don't feel strongly about it.
I think part of the confusion also comes from the fact that the function is called
get_extrapolation_point_...
but it doesn't actually return anything.How do you guys feel about returning
float2
here?Oh, that's a good point. But rather than changing what they return, I wonder if just changing the names to
add_extrapolation_point_left/right
would make sense. Then they're consistent withadd_bezt_vertices()
, which has the same behavior and is used in the same way.I have an itchy finger just waiting to hit "approve". 😉 Just waiting on these function renames.
done now :)
@ -892,2 +886,4 @@
}
/* If the resolution goes too high the line will not end exactly at the keyframe. Probably due to
* accumulating floating point issues in BKE_curve_forward_diff_bezier.*/
Definitely doesn't need to be part of this PR, but it sounds to me like either using Kahan summation or simply switching to doubles (just for the computations) in the forward differencing code would make sense. Then we won't need to have an arbitrary and (IMO) too-low maximum here.
I never heard of Kahan summation before so thanks for pointing that out :)
btw the max resolution is now double than what it was before. Not sure we will ever run into artifacts with it
Over all this looks great! Just some minor comments, and a suggestion about how to compute curve resolution a bit more robustly.
@ -877,4 +873,4 @@
/**
* Draw a segment from `prevbezt` to `bezt` at the given `resolution`.
* #immBeginAtMost is expected to be called with enough space for this function to run.
I believe this note about
immBeginAtMost
should now be moved todraw_fcurve_curve_bezts()
, since this function no longer makes any graphics calls.(And just generally this doc comment can be updated to reflect what the function actually does now, but Sybren mentioned that already.)
good point. updated the comment
@ -986,3 +1056,2 @@
vertex_position[1] = prevbezt->vec[1][1];
immVertex2fv(pos, vertex_position);
curve_vertices.append({fcu->bezt[0].vec[1][0], fcu->bezt[0].vec[1][1]});
}
Is this redundant with the code in the
if (bounding_indices[0] == bounding_indices[1])
block below? Or rather, I think that code handles a superset of this code. If there's only one vertex, thenbounding_indices[0] == bounding_indices[1]
will be true (I think?) and it will be added there anyway.actually you are right, removed this if now
@ -990,0 +1059,4 @@
const int2 bounding_indices = get_bounding_bezt_indices(fcu, v2d->cur.xmin, v2d->cur.xmax);
if (bounding_indices[0] == bounding_indices[1]) {
BezTriple *bezt = &fcu->bezt[bounding_indices[0]];
It took me a bit of thinking to realize the conditions that this handles:
Might be worth adding a brief comment mentioning that, so people don't need to puzzle it out.
good idea. done that
if only_one_key
since it was already handled byif (bounding_indices[0] == bounding_indices[1])
@ -876,0 +875,4 @@
const int resolution_y = (int)((max_y - min_y) * resolution_scale[1]);
/* The resolution should be the distance between the two keys, but to save the square root
* calculation use an approximation by the two values. */
return resolution_x + resolution_y;
I was puzzling if this or
max(resolution_x, resolution_y)
makes more sense. And I've come to the tentative conclusion that summing is probably not only better than max but actually even better than the square-root (Euclidean) distance calculation.Basically, the Euclidean distance effectively computes what the resolution would need to be for a straight diagonal line between the corners of the bounding box. But that is frequently very far from the case with bezier curves. Bezier curves will often be bending towards one corner or the other, which actually makes the length of the curve somewhere between the euclidean distance and the simple sum.
In other words, I think the sum isn't even an optimization, it's just being more conservative, which also makes it more correct in a way.
(Incidentally, this kind of sum can be called the Manhattan distance. So you could start the comment with "We use the Manhattan distance rather than Euclidean distance because...". Although I think it's pretty clear either way, so not a big deal.)
This also has me thinking that there are probably further improvements we can make to this metric that would still be pretty cheap to compute. But I think we should land this as-is, and if we feel like it needs further improvements we can do those in a future PR.
ha I've read Manhattan distance in blenders code before and I didn't know what it was, the name makes so much sense now
I'd actually prefer to not use this specific term just because I wasn't aware of what it meant, and there might be others reading the code that never heard of it either.
I thought about using
max()
as well, but came to the same conclusion as you.I'd love to hear of other ways to compute the resolution though :)
Totally fair! And indeed, you not knowing what it meant is probably a good indication to avoid the math jargon here.
Makes sense! I think it would be good to change the comment to explain that the simple sum is actually a better metric, then. Right now the comment makes it sound like it's just to avoid the sqrt at the expense of accuracy.
Looks good to me!