Curves: Bezier handles for new Curves #119053
No reviewers
Labels
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
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#119053
Loading…
Reference in New Issue
No description provided.
Delete Branch "laurynas/blender:bezier-handles-for-new-curves"
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?
Draws Bezier handles.
It's more an initial version as handles are drawn as line strip instead of triangles.
So handle edges aren't smooth as in legacy curves, but for now I'd give priority to handle selection and movement.
I was actually working on a very similar thing just now. Didn't know someone else is working on this thing specifically. I'm happy to let you finish this though :) (my initial patch: #119036)
Thanks for working on this. It's a tricky problem to do this elegantly but you have a nice start.
One thing that would help "self-document" your code is having three
IndexRange
variables for the different sections of the buffers. Using the[]
accessor for those ranges is usually clearer than doing index addition multiple times.As a theme for my comments, it's important to optimize for the case when there are no Bezier curves (in other words, doing no new work for that case), and also using existing utilities can avoid the need to check the type for every curve index.
@ -48,1 +51,4 @@
#define COLOR_SHIFT 5
/* ---------------------------------------------------------------------- */
struct CurvesUboStorage {
Is this used in the shaders actually? Maybe I'm missing something
Yes. In overlay_edit_curves_handle_vert.glsl. Actually at the moment one bit would be sufficient, but in legacy curves it is 5 and thought there could go curve type information to make lattice colors different. So left it the same.
Miss understood the question.
Yes it is used in frag shader, but I didn't know how to define this struct there before definition of variable, so it is defined as
.uniform_buf(0, "int", "curvesInfoBlock[4]")
inoverlay_edit_mode_info.hh
.More extensive answer. One
int
is needed, but didn't work withpush_constant()
. Indraw_cache_impl_curves.cc
don't have access toDRWShadingGroup *lines_shgrp
, inoverlay_edit_curves.cc
value ofright_handles_offset
isn't yet calculated. The only solution could think of was UBO.@ -59,1 +69,4 @@
GPUVertBuf *edit_points_data;
GPUUniformBuf *curvess_ubo_storage;
curvess
->curves
@ -249,2 +278,3 @@
if (format_pos.attr_len == 0) {
pos = GPU_vertformat_attr_add(&format_pos, "pos", GPU_COMP_F32, 3, GPU_FETCH_FLOAT);
GPU_vertformat_attr_add(&format_pos, "pos", GPU_COMP_F32, 3, GPU_FETCH_FLOAT);
GPU_vertformat_attr_add(&format_data, "data", GPU_COMP_U32, 1, GPU_FETCH_INT);
It seems important not to upload the extra data VBO when there are no Bezier curves. We should basically fall back on the existing code in that case.
Shader expects "data" buffer to be and have data allocated. For this I see two possible solutions: either two separate shaders or
compute shader. First solution would be difficult to support and maybe effort is not worth the benefit. Compute shader could get only types per curve giving a lot smaller buffer to upload, but same shader could compute index buffer and maybe do merge of point pos array with left/right handles arrays... This leads to huge rewrite.
I'd rather keep it simple for now.
Also my Vulkan knowledge is very limited, I might be missing something.
For legacy curve data this buffer is
GPU_COMP_U8
which would make it smaller. Seems ok to not worry about it too much right now.@ -256,0 +331,4 @@
for (const int point : curve_points) {
const int handle_index = point - curve_points.start() + handle_offset;
left_handles[handle_index] = math::transform_point(deformation.deform_mats[point],
Using the transform here seems a bit wrong. The drawing code should use the evaluated handle positions without changing them. If they're not changed by geometry nodes evaluation, it's probably because a particular node setup doesn't really support Bezier curves. The solution to that is elsewhere. Probably the
GeometryDeformation
system will need to store handle positions as well as control point positions in the future.That
blender::bke::crazyspace
thing was a puzzle for me. From what I understood there are three cases:deform_mats.size() == 0
deformation.positions.size() == curves_orig_id->geometry.point_num
Case 1. doesn't affect anything, it's like identity case.
In case 2., as far as I understand from the existing curves overlay code, transforms original vertices and stores transform matrices in
deformation.deform_mats
for each point. So it seemed logical to transform left/right handle by appropriate matrix as old code draws transformed positions.In case 3. it is impossible to guess point's index the so I fall back to old behavior by making
bezier_curve_count = 0; bezier_point_count = 0;
. While writing this it came to me, that in this case it doesn't make sense trying to connect points with lines also. Anyway old code did it.In conclusion it is an easy change in case 2. to fall back to case 3. behavior.
Also need to mention I didn't test 2. and 3. cases, those are only my considerations.
The
GeometryDeformation
struct is a storage for the last deformed data during evaluation before there is a topology change. Sometimes there is no topology change. And sometimes there is not enough information to fill the deform matrices. The purpose of those is to allow editing the deformed positions while changing the original positions, even if everything has been rotated, etc.Bezier handles were just not considered in that system at all. But they are positions just the same as the position attribute, and they should be handled the same way.
@ -268,2 +365,2 @@
MutableSpan<float> data(static_cast<float *>(GPU_vertbuf_get_data(cache.edit_points_selection)),
curves.points_num());
GPU_vertbuf_data_alloc(cache.edit_points_selection, vert_count);
float *buffer_data = static_cast<float *>(GPU_vertbuf_get_data(cache.edit_points_selection));
Smaller change:
@ -274,0 +383,4 @@
int left_handle_offset = curves.points_num();
for (const int curve : curves.curves_range()) {
I'd suggest structuring this code a bit differently. Using some higher level utilities, we can more easily avoid checking the curve type many times, more easily optimize for the cases when all curves are Bezier curves, and automatically have multi-threading
The offsets local to just the Bezier curve points and the IndexMask should be useful elsewhere too. They should be able to replace the
count_curve_type
utility completely.BTW, I also think the way Jacques coded this was reasonable-- combining the values like
left, point, right, left, point, right, left
... etc.For higher lever utils I need more time and clearer head.
Regarding
left, point, right, ...
. One benefit I see is that positions are copied in chunks not one by one.And most importantly this layout is used in
overlay_edit_curves_handle_frag.glsl
to treat right handles differently.Not sure it's an elegant solution or more a hack, but other options were to duplicate all Bezier points (knots) in a vertex buffer or
use geometry shader ( leading to *_no_geom.glsl for Metal).
I'm not sure it will be possible to transition from line to triangles this way, but for now it is simple.
From Jacques code I saw I missed cyclic cases. Those will affect index generation, but it can be treated as of the patch scope thing :)
The problem with handles is that right and left can have different types, leading to different colors for lines.
So Bezier point (knot) has to be drawn twice, for left and right handle with a different color.
Sorry all that second reason with right handles is unrelated. It is a matter of layout in index buffer not in vertex buffer.
So copying in chunks is the reason.
@ -343,3 +501,1 @@
* the Blender convention, it should be `vec4(s, s, s, 1)`. This could be resolved using a
* similar texture state swizzle to map the attribute correctly as for volume attributes, so we
* can control the conversion ourselves. */
/* TODO(@kevindietrich): float4 is used for scalar attributes as the implicit conversion
Unnecessary line wrapping changes here
Sorry about that and thanks :)
It's not the first time I'm in a such situation, I'll have to make some conclusions.
Curves: Bezier handles for new Curvesto WIP: Curves: Bezier handles for new CurvesWIP: Curves: Bezier handles for new Curvesto Curves: Bezier handles for new CurvesCurves: Bezier handles for new Curvesto WIP: Curves: Bezier handles for new CurvesWIP: Curves: Bezier handles for new Curvesto Curves: Bezier handles for new CurvesI'm not sure if this can be fully implemented without taking into account that nurb curves also need special drawing for the control points and without drawing the actual evaluated curve as in my patch. Do you think it would be complex to integrate that functionality here?
I don't have a clear understanding what you mean. How it would look from the user perspective?
I didn't intend to touch drawing of evaluated curves in this patch. Looking at your patch can't find what couldn't be brought here.
You generate control leg for NURBS and handles for Bezier. Here leg is generated for types
CURVE_TYPE_POLY
,CURVE_TYPE_NURBS
,CURVE_TYPE_CATMULL_ROM
and handles for Bezier. Maybe I should exclude Catmull Rom and Polylines from this.Can't find NURBS referenced in other place in your version, but not sure I'm not answering your question.
You can use the drawing of the legacy curve edit mode as reference. I basically just want the same thing for the new curves.
I understand that. I started out wanting to implement just the bezier handles too. However, I found that just adding bezier handles made the edit mode even more confusing because we don't draw the evaluated curve at all. Once that is implemented, one has to add back drawing the control points, because those may not lie on the evaluated curve (for nurbs). And only then it makes sense to add bezier handles imo.
@JacquesLucke NURBS are treated differently now.
Also I have your evaluated curves drawing added to this patch, but hesitate to commit/push.
Would you mind me removing
GeometryDeformation
fromcurves_batch_cache_ensure_edit_curves_lines_pos
definition?Unless you know for sure that deformed evaluated curves geometry will appear there with time.
Also not tightly related thing bathers me. When I started working on this, handle lines already had in-front passes support.
DRWPass *edit_curves_lines_ps[2];
In legacy curves and your PR
edit_curves_handle_ps
are not arrays. So I decided to change it here also.Now I tend to do the same with control points
edit_curves_points_ps
. Also I'm thinking about renaming them toedit_curves_handle_points_ps
.There shouldn't really a hesitation to commit code changes. Reverting a change later is easy enough.
Well, we can have deformed curves already. I think the code would have to deform the original points and then calculate the evaluated points based on that. I'm fine if you ignore this for now. I can look into that once your patch is ready.
Honestly, I don't have a good intution yet for when we want to have the in-font pass and when we don't want it. Does it increase complexity for you to keep the in-front pass where it was already? This also seems like something we can solve separately once the general drawing is in place.
Sounds good to me.
I wanted to control PR scope to get it reviewed and accepted faster.
Support is easy. Just handle lines now are drawn with
state = DRW_STATE_WRITE_COLOR
and control points are not.Changed my mind on this, because it leads to renaming points in
CurvesBatchCache
also and changes explode. There is only one type of points in the context, maybe it will be clear.Nice thanks. Still haven't looked at the code in detail yet, but the behavior feels good.
I only noticed one thing: For catmul rom curves, it shouldn't draw the poly curve between the control points. Just like for bezier curves, that's not necessary because the control points are always on the evaluated curve.
OK.
Same applies to CURVE_TYPE_POLY curves I guess.
@ -66,0 +68,4 @@
sh = OVERLAY_shader_edit_curves_handle();
grp = pd->edit_curves_handles_grp = DRW_shgroup_create(sh, psl->edit_curves_handles_ps);
DRW_shgroup_uniform_block(grp, "globalsBlock", G_draw.block_ubo);
DRW_shgroup_uniform_bool_copy(grp, "useWeight", false);
Is
useWeight
used?@ -0,0 +1,8 @@
/* SPDX-FileCopyrightText: 2022 Blender Authors
Fix year, same below.
@ -0,0 +8,4 @@
float4 get_bezier_handle_color(uint color_id, float sel)
{
switch (color_id) {
case 0u: // BEZIER_HANDLE_FREE
Comment style:
/* ... */
@ -45,22 +48,39 @@
namespace blender::draw {
#define NURBS_CONTROL_POINT (1u << 1)
Maybe a stupid question, but why do we have to define this here again, if it's already defined in
overlay_shader_shared
?Also I think it would be good to distance ourselves more from the legacy curve drawing by using a prefix like
EDIT_CURVES_NURBS_CONTROL_POINT
andEDIT_CURVES_BEZIER_HANDLE
.I had same question: why it is done with legacy curves? And my guess is:
in "overlay_shader_shared.h". Just tried to add
#include "../engines/overlay/overlay_private.hh"
instead. Constants can't be resolved.Interesting, would have assumed that constants are simpler to share than e.g. structs.
@ -47,1 +50,4 @@
#define NURBS_CONTROL_POINT (1u << 1)
#define BEZIER_HANDLE (1u << 3)
#define COLOR_SHIFT 5
From what I can tell, the only reason for having the
COLOR_SHIFT
define here is that it also exists for the curve edit mode. It doesn't really seem necessary for this patch.I think it's better to just remove all unnecessary complexity and to keep tthese flags as simple as possible.
It is used, but naming is not accurate. Used both in
draw_cache_impl_curves.cc
and inoverlay_edit_curves_handle_vert.glsl
, renaming it toEDIT_CURVES_HANDLE_TYPES_SHIFT
.Split into
EDIT_CURVES_LEFT_HANDLE_TYPE_SHIFT
and same for right. This avoids the magic number+ 2
in a few places.@ -59,1 +71,4 @@
GPUVertBuf *edit_points_data;
GPUUniformBuf *curves_ubo_storage;
Some comments explaining what these new members contain would be useful.
@ -264,0 +287,4 @@
return bezier_data_value(handle_type, handle_type);
}
static void curves_batch_cache_ensure_edit_points_pos(
_pos_and_data
@ -264,0 +291,4 @@
const bke::CurvesGeometry &curves,
const IndexMask bezier_curves,
const OffsetIndices<int> bezier_dst_offsets,
bke::crazyspace::GeometryDeformation deformation,
Can be const I think.
@ -264,0 +297,4 @@
static GPUVertFormat format_pos = single_attr_vertbuffer_format(
"pos", GPU_COMP_F32, 3, GPU_FETCH_FLOAT);
static GPUVertFormat format_data = single_attr_vertbuffer_format(
"data", GPU_COMP_U32, 1, GPU_FETCH_INT);
It looks like we only need a
GPU_COMP_U8
currently?@ -264,0 +312,4 @@
uint32_t *data_buffer_data = static_cast<uint32_t *>(
GPU_vertbuf_get_data(cache.edit_points_data));
MutableSpan<float3> pos_span(pos_buffer_data, deformed_positions.size());
pos_span
->pos_dst
, same withdata_span
below.@ -264,0 +329,4 @@
const OffsetIndices<int> points_by_curve = curves.points_by_curve();
auto handle_other_curves = [&](const uint32_t fill_value) {
return [=](const IndexMask &selection) {
I prefer to explicitly list the captured values instead of using
=
to avoid surprises when it unexpectedly copies something big.@ -266,0 +368,4 @@
pos_buffer_data + deformed_positions.size() + bezier_point_count, bezier_point_count);
array_utils::gather_group_to_group(
points_by_curve, bezier_dst_offsets, bezier_curves, left_handle_positions, left_handles);
I think technically we have to take the deformation into account here too, but that can be fixed in a separate commit. Please add a todo comment.
@ -274,0 +398,4 @@
}
const VArray<float> attribute_left = *curves.attributes().lookup_or_default<float>(
".selection_handle_left", bke::AttrDomain::Point, 0.0f);
Those attributes don't exist yet. Better omit this part for now. Can be added once we support selecting handles.
I'd keep them, this allowed me to test data alignment and drawing code. Without them handle drawing is not complete.
Attribute naming is arranged with @HooglyBoogly.
Also I have ready (at least for review) Bezier handle selection code. Hope you will not see this as reason not to keep. :)
@ -716,0 +943,4 @@
static struct {
uint pos;
} attr_id;
static GPUVertFormat format = [&]() {
Can use your utility for the format here.
My utility looses attr_id. I would have to write another one.
I think you can get rid of
attr_id
here and just copy the data into the buffer manually as is done in other places.While playing with
GPU_COMP_U8
fordata
came up with a different way:If this variable is too ugly, could wrapper function of the same name without last parameter instead of default value to it.
If both are no go, will do are as suggested above.
Looks mostly good now, some more comments are inline.
@ -0,0 +4,4 @@
void main()
{
fragColor = gl_PrimitiveID < curvesInfoBlock[0] ? leftColor : finalColor;
This could use a comment. Without some more context, it's very confusing what this check does. Might also be worth to add a line like
int bezier_point_count = curvesInfoBlock[0]
. I honestly also don't quite understand how this can work tbh. Looks like it ignores the right handle color.@ -264,0 +323,4 @@
GPU_vertbuf_data_alloc(cache.edit_points_data, size);
float3 *pos_buffer_data = static_cast<float3 *>(GPU_vertbuf_get_data(cache.edit_points_pos));
uint32_t *data_buffer_data = static_cast<uint32_t *>(
I'm getting crashes because this is still using
uint32_t
and notuint8_t
.On my GPU
minimum_per_vertex_stride
is 4. Meaning even if I create buffer of GPU_COMP_U8,for each entry there will be 4 bytes allocated anyway. It also forces using of
functions like
GPU_vertbuf_attr_set
instead of direct assignment to Span<>.So memory saving not guaranteed, readability less and slowdown enforced. Do we really need to switch to GPU_COMP_U8?
Interesting. Seems like that's incredibly important information that should at least be mentioned in the description of
GPU_vertbuf_get_data
and probably alsoGPU_COMP_U8
. I might even go so far to assert that there is no extra (potential) padding whenGPU_vertbuf_get_data
is called.Anyways, that's independent of this patch. It's fine with me to use u32 in this case for now. A comment mentioning that in theory 8 bit would suffice currently might be good anyways.
GPU_vertbuf_get_data
has TODO inside, stating the same, but it would be very restricting.I'd suggest template version of
GPU_vertbuf_get_data
casting pointer to a certain type before returning.Maybe even
Span
andMutableSpan
versions somewhere in the project.Assert
sizeof(type) == stride
would not be excessive in these.Looks good to me now. Thanks for working on this and for addressing the feedback so quickly.
@ -60,0 +83,4 @@
*/
GPUVertBuf *edit_points_data;
/* Buffer used to store CurvesUboStorage value. push_constant() could not be used for this
Double whitespace.
@ -284,0 +452,4 @@
const int index_len_for_nurbs = nurbs_offsets.total_size() + nurbs_curves.size() +
array_utils::count_booleans(cyclic, nurbs_curves);
const int index_len = index_len_for_bezier_handles + index_len_for_nurbs;
GPUIndexBufBuilder elb, right_elb;
/* Use two index buffer builders for the same underlying memory. */
or similar. Found that a bit confusing at first. Also becauseright_elb
is used by nurbs too.