Curves: Bezier handles for new Curves #119053

Merged
Jacques Lucke merged 25 commits from laurynas/blender:bezier-handles-for-new-curves into main 2024-03-19 10:39:25 +01:00
Contributor

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.

Draws Bezier handles. <video src="/attachments/5132ccef-ca1c-4b4f-ab33-e5c20ad493b7" title="handles.mov" controls></video> 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.
Laurynas Duburas added 1 commit 2024-03-04 13:42:16 +01:00
Laurynas Duburas requested review from Hans Goudey 2024-03-04 13:42:31 +01:00
Member

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)

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)
Hans Goudey requested changes 2024-03-04 20:18:06 +01:00
Hans Goudey left a comment
Member

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.

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

Is this used in the shaders actually? Maybe I'm missing something

Is this used in the shaders actually? Maybe I'm missing something
Author
Contributor

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.

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.
Author
Contributor

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]") in overlay_edit_mode_info.hh.

More extensive answer. One int is needed, but didn't work with push_constant(). In draw_cache_impl_curves.cc don't have access to DRWShadingGroup *lines_shgrp, in overlay_edit_curves.cc value of right_handles_offset isn't yet calculated. The only solution could think of was UBO.

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]")` in `overlay_edit_mode_info.hh`. More extensive answer. One `int` is needed, but didn't work with `push_constant()`. In `draw_cache_impl_curves.cc` don't have access to `DRWShadingGroup *lines_shgrp`, in `overlay_edit_curves.cc` value of `right_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;
Member

curvess -> curves

`curvess` -> `curves`
laurynas marked this conversation as resolved
@ -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);
Member

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.

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.
Author
Contributor

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.

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

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.

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],
Member

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.

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.
Author
Contributor

That blender::bke::crazyspace thing was a puzzle for me. From what I understood there are three cases:

  1. there is no extra transformation deform_mats.size() == 0
  2. transformation exits, but it doesn't change vertex count deformation.positions.size() == curves_orig_id->geometry.point_num
  3. transformation exits and vertex count changes.

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.

That `blender::bke::crazyspace` thing was a puzzle for me. From what I understood there are three cases: 1. there is no extra transformation `deform_mats.size() == 0` 2. transformation exits, but it doesn't change vertex count `deformation.positions.size() == curves_orig_id->geometry.point_num` 3. transformation exits and vertex count changes. 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.
Member

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.

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.
laurynas marked this conversation as resolved
@ -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));
Member

Smaller change:

  MutableSpan<float> data(static_cast<float *>(GPU_vertbuf_get_data(cache.edit_points_selection)),
                          vert_count);
Smaller change: ``` MutableSpan<float> data(static_cast<float *>(GPU_vertbuf_get_data(cache.edit_points_selection)), vert_count); ```
laurynas marked this conversation as resolved
@ -274,0 +383,4 @@
int left_handle_offset = curves.points_num();
for (const int curve : curves.curves_range()) {
Member

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

  IndexMaskMemory memory;
  const IndexMask bezier_curves = bke::curves::indices_for_type(curves.curve_types(),
                                                                curves.curve_type_counts(),
                                                                CURVE_TYPE_BEZIER,
                                                                curves.curves_range(),
                                                                memory);

  Array<int> bezier_point_offset_data(bezier_curves.size() + 1);
  const OffsetIndices bezier_offsets = offset_indices::gather_selected_offsets(
      points_by_curve, bezier_curves, bezier_point_offset_data);

  array_utils::gather_group_to_group(
      points_by_curve,
      bezier_offsets,
      bezier_curves,
      attribute_left,
      data.slice(IndexRange::from_begin_size(curves.points_num(), bezier_offsets.total_size())));

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.

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 ``` IndexMaskMemory memory; const IndexMask bezier_curves = bke::curves::indices_for_type(curves.curve_types(), curves.curve_type_counts(), CURVE_TYPE_BEZIER, curves.curves_range(), memory); Array<int> bezier_point_offset_data(bezier_curves.size() + 1); const OffsetIndices bezier_offsets = offset_indices::gather_selected_offsets( points_by_curve, bezier_curves, bezier_point_offset_data); array_utils::gather_group_to_group( points_by_curve, bezier_offsets, bezier_curves, attribute_left, data.slice(IndexRange::from_begin_size(curves.points_num(), bezier_offsets.total_size()))); ``` 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.
Member

BTW, I also think the way Jacques coded this was reasonable-- combining the values like left, point, right, left, point, right, left... etc.

BTW, I also think the way Jacques coded this was reasonable-- combining the values like `left, point, right, left, point, right, left`... etc.
Author
Contributor

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

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 :)
Author
Contributor

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.

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.
Author
Contributor

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.

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.
laurynas marked this conversation as resolved
@ -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
Member

Unnecessary line wrapping changes here

Unnecessary line wrapping changes here
laurynas marked this conversation as resolved
Author
Contributor

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)

Sorry about that and thanks :)
It's not the first time I'm in a such situation, I'll have to make some conclusions.

> 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) Sorry about that and thanks :) It's not the first time I'm in a such situation, I'll have to make some conclusions.
Laurynas Duburas added 2 commits 2024-03-04 22:42:38 +01:00
Laurynas Duburas added 1 commit 2024-03-05 09:38:07 +01:00
Laurynas Duburas added 1 commit 2024-03-05 10:37:43 +01:00
Laurynas Duburas changed title from Curves: Bezier handles for new Curves to WIP: Curves: Bezier handles for new Curves 2024-03-05 11:20:02 +01:00
Laurynas Duburas added 1 commit 2024-03-05 12:27:56 +01:00
Laurynas Duburas added 1 commit 2024-03-06 09:33:38 +01:00
Laurynas Duburas added 1 commit 2024-03-06 10:25:07 +01:00
Laurynas Duburas added 1 commit 2024-03-06 10:35:46 +01:00
Laurynas Duburas changed title from WIP: Curves: Bezier handles for new Curves to Curves: Bezier handles for new Curves 2024-03-06 10:36:02 +01:00
Laurynas Duburas added 1 commit 2024-03-06 12:19:53 +01:00
Laurynas Duburas added 1 commit 2024-03-06 12:24:30 +01:00
Laurynas Duburas changed title from Curves: Bezier handles for new Curves to WIP: Curves: Bezier handles for new Curves 2024-03-06 14:21:06 +01:00
Laurynas Duburas added 1 commit 2024-03-06 14:59:23 +01:00
Laurynas Duburas changed title from WIP: Curves: Bezier handles for new Curves to Curves: Bezier handles for new Curves 2024-03-06 14:59:34 +01:00
Laurynas Duburas added 2 commits 2024-03-06 19:56:09 +01:00
Laurynas Duburas requested review from Hans Goudey 2024-03-06 20:01:00 +01:00
Laurynas Duburas added 1 commit 2024-03-07 08:50:19 +01:00
Member

I'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'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?
Author
Contributor

I'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.

> I'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.
Member

You can use the drawing of the legacy curve edit mode as reference. I basically just want the same thing for the new curves.

You can use the drawing of the legacy curve edit mode as reference. I basically just want the same thing for the new curves.
Member

I didn't intend to touch drawing of evaluated curves in this patch.

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.

> I didn't intend to touch drawing of evaluated curves in this patch. 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.
Laurynas Duburas added 2 commits 2024-03-12 05:56:14 +01:00
Laurynas Duburas added 1 commit 2024-03-12 19:51:20 +01:00
adae5af152 Merge branch 'main' into bezier-handles-for-new-curves
To have "Fix #119247: Curves: Extra point in evaluated spline of Curves geometry" in branch.
Laurynas Duburas added 1 commit 2024-03-12 20:38:40 +01:00
Author
Contributor

@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 from curves_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 to edit_curves_handle_points_ps.

@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` from `curves_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 to `edit_curves_handle_points_ps`.
Member

but hesitate to commit/push

There shouldn't really a hesitation to commit code changes. Reverting a change later is easy enough.

Would you mind me removing GeometryDeformation from curves_batch_cache_ensure_edit_curves_lines_pos definition?
Unless you know for sure that deformed evaluated curves geometry will appear there with time.

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.

When I started working on this, handle lines already had in-front passes support.

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.

Now I tend to do the same with control points edit_curves_points_ps. Also I'm thinking about renaming them to edit_curves_handle_points_ps.

Sounds good to me.

> but hesitate to commit/push There shouldn't really a hesitation to commit code changes. Reverting a change later is easy enough. > Would you mind me removing `GeometryDeformation` from `curves_batch_cache_ensure_edit_curves_lines_pos` definition? > Unless you know for sure that deformed evaluated curves geometry will appear there with time. 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. > When I started working on this, handle lines already had in-front passes support. 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. > Now I tend to do the same with control points `edit_curves_points_ps`. Also I'm thinking about renaming them to `edit_curves_handle_points_ps`. Sounds good to me.
Laurynas Duburas added 1 commit 2024-03-13 12:44:29 +01:00
Author
Contributor

There shouldn't really a hesitation to commit code changes. Reverting a change later is easy enough.

I wanted to control PR scope to get it reviewed and accepted faster.

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.

Support is easy. Just handle lines now are drawn with state = DRW_STATE_WRITE_COLOR and control points are not.

Now I tend to do the same with control points edit_curves_points_ps. Also I'm thinking about renaming them to edit_curves_handle_points_ps.

Sounds good to me.

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.

> There shouldn't really a hesitation to commit code changes. Reverting a change later is easy enough. I wanted to control PR scope to get it reviewed and accepted faster. >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. Support is easy. Just handle lines now are drawn with `state = DRW_STATE_WRITE_COLOR` and control points are not. >> Now I tend to do the same with control points edit_curves_points_ps. Also I'm thinking about renaming them to edit_curves_handle_points_ps. >Sounds good to me. 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.
Member

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.

image

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. ![image](/attachments/5ffeabc1-635b-4cdf-a408-a3722468e127)
Author
Contributor

OK.
Same applies to CURVE_TYPE_POLY curves I guess.

OK. Same applies to CURVE_TYPE_POLY curves I guess.
Laurynas Duburas added 1 commit 2024-03-15 14:23:44 +01:00
Jacques Lucke requested changes 2024-03-15 18:27:31 +01:00
@ -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);
Member

Is useWeight used?

Is `useWeight` used?
laurynas marked this conversation as resolved
@ -0,0 +1,8 @@
/* SPDX-FileCopyrightText: 2022 Blender Authors
Member

Fix year, same below.

Fix year, same below.
laurynas marked this conversation as resolved
@ -0,0 +8,4 @@
float4 get_bezier_handle_color(uint color_id, float sel)
{
switch (color_id) {
case 0u: // BEZIER_HANDLE_FREE
Member

Comment style: /* ... */

Comment style: `/* ... */`
laurynas marked this conversation as resolved
@ -45,22 +48,39 @@
namespace blender::draw {
#define NURBS_CONTROL_POINT (1u << 1)
Member

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 and EDIT_CURVES_BEZIER_HANDLE.

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` and `EDIT_CURVES_BEZIER_HANDLE`.
Author
Contributor

I had same question: why it is done with legacy curves? And my guess is:

#ifdef GPU_SHADER

in "overlay_shader_shared.h". Just tried to add #include "../engines/overlay/overlay_private.hh" instead. Constants can't be resolved.

I had same question: why it is done with legacy curves? And my guess is: ```c #ifdef GPU_SHADER ``` in "overlay_shader_shared.h". Just tried to add `#include "../engines/overlay/overlay_private.hh"` instead. Constants can't be resolved.
Member

Interesting, would have assumed that constants are simpler to share than e.g. structs.

Interesting, would have assumed that constants are simpler to share than e.g. structs.
JacquesLucke marked this conversation as resolved
@ -47,1 +50,4 @@
#define NURBS_CONTROL_POINT (1u << 1)
#define BEZIER_HANDLE (1u << 3)
#define COLOR_SHIFT 5
Member

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.

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.
Author
Contributor

It is used, but naming is not accurate. Used both in draw_cache_impl_curves.cc and in overlay_edit_curves_handle_vert.glsl, renaming it to EDIT_CURVES_HANDLE_TYPES_SHIFT.

It is used, but naming is not accurate. Used both in `draw_cache_impl_curves.cc` and in `overlay_edit_curves_handle_vert.glsl`, renaming it to `EDIT_CURVES_HANDLE_TYPES_SHIFT`.
Member

Split into EDIT_CURVES_LEFT_HANDLE_TYPE_SHIFT and same for right. This avoids the magic number + 2 in a few places.

Split into `EDIT_CURVES_LEFT_HANDLE_TYPE_SHIFT` and same for right. This avoids the magic number `+ 2` in a few places.
laurynas marked this conversation as resolved
@ -59,1 +71,4 @@
GPUVertBuf *edit_points_data;
GPUUniformBuf *curves_ubo_storage;
Member

Some comments explaining what these new members contain would be useful.

Some comments explaining what these new members contain would be useful.
laurynas marked this conversation as resolved
@ -264,0 +287,4 @@
return bezier_data_value(handle_type, handle_type);
}
static void curves_batch_cache_ensure_edit_points_pos(
Member

_pos_and_data

`_pos_and_data`
laurynas marked this conversation as resolved
@ -264,0 +291,4 @@
const bke::CurvesGeometry &curves,
const IndexMask bezier_curves,
const OffsetIndices<int> bezier_dst_offsets,
bke::crazyspace::GeometryDeformation deformation,
Member

Can be const I think.

Can be const I think.
laurynas marked this conversation as resolved
@ -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);
Member

It looks like we only need a GPU_COMP_U8 currently?

It looks like we only need a `GPU_COMP_U8` currently?
laurynas marked this conversation as resolved
@ -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());
Member

pos_span -> pos_dst, same with data_span below.

`pos_span` -> `pos_dst`, same with `data_span` below.
laurynas marked this conversation as resolved
@ -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) {
Member

I prefer to explicitly list the captured values instead of using = to avoid surprises when it unexpectedly copies something big.

I prefer to explicitly list the captured values instead of using `=` to avoid surprises when it unexpectedly copies something big.
laurynas marked this conversation as resolved
@ -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);
Member

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.

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.
laurynas marked this conversation as resolved
@ -274,0 +398,4 @@
}
const VArray<float> attribute_left = *curves.attributes().lookup_or_default<float>(
".selection_handle_left", bke::AttrDomain::Point, 0.0f);
Member

Those attributes don't exist yet. Better omit this part for now. Can be added once we support selecting handles.

Those attributes don't exist yet. Better omit this part for now. Can be added once we support selecting handles.
Author
Contributor

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

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. :)
JacquesLucke marked this conversation as resolved
@ -716,0 +943,4 @@
static struct {
uint pos;
} attr_id;
static GPUVertFormat format = [&]() {
Member

Can use your utility for the format here.

Can use your utility for the format here.
Author
Contributor

My utility looses attr_id. I would have to write another one.

My utility looses attr_id. I would have to write another one.
Member

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.

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.
Author
Contributor

While playing with GPU_COMP_U8 for data came up with a different way:

static uint DUMMY_ID;

static GPUVertFormat single_attr_vertbuffer_format(const char *name,
                                                   GPUVertCompType comp_type,
                                                   uint comp_len,
                                                   GPUVertFetchMode fetch_mode,
                                                   uint &attr_id = DUMMY_ID)

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.

While playing with `GPU_COMP_U8` for `data` came up with a different way: ```c static uint DUMMY_ID; static GPUVertFormat single_attr_vertbuffer_format(const char *name, GPUVertCompType comp_type, uint comp_len, GPUVertFetchMode fetch_mode, uint &attr_id = DUMMY_ID) ``` 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.
JacquesLucke marked this conversation as resolved
Laurynas Duburas added 1 commit 2024-03-15 23:35:48 +01:00
Jacques Lucke requested changes 2024-03-16 11:45:10 +01:00
Jacques Lucke left a comment
Member

Looks mostly good now, some more comments are inline.

Looks mostly good now, some more comments are inline.
@ -0,0 +4,4 @@
void main()
{
fragColor = gl_PrimitiveID < curvesInfoBlock[0] ? leftColor : finalColor;
Member

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.

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.
laurynas marked this conversation as resolved
@ -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 *>(
Member

I'm getting crashes because this is still using uint32_t and not uint8_t.

I'm getting crashes because this is still using `uint32_t` and not `uint8_t`.
Author
Contributor

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?

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

Interesting. Seems like that's incredibly important information that should at least be mentioned in the description of GPU_vertbuf_get_data and probably also GPU_COMP_U8. I might even go so far to assert that there is no extra (potential) padding when GPU_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.

Interesting. Seems like that's incredibly important information that should at least be mentioned in the description of `GPU_vertbuf_get_data` and probably also `GPU_COMP_U8`. I might even go so far to assert that there is no extra (potential) padding when `GPU_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.
Author
Contributor

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 and MutableSpan versions somewhere in the project.
Assert sizeof(type) == stride would not be excessive in these.

`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` and `MutableSpan` versions somewhere in the project. Assert `sizeof(type) == stride` would not be excessive in these.
Laurynas Duburas added 1 commit 2024-03-16 21:29:28 +01:00
Jacques Lucke approved these changes 2024-03-17 09:44:33 +01:00
Jacques Lucke left a comment
Member

Looks good to me now. Thanks for working on this and for addressing the feedback so quickly.

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
Member

Double whitespace.

Double whitespace.
laurynas marked this conversation as resolved
@ -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;
Member

/* Use two index buffer builders for the same underlying memory. */ or similar. Found that a bit confusing at first. Also because right_elb is used by nurbs too.

`/* Use two index buffer builders for the same underlying memory. */` or similar. Found that a bit confusing at first. Also because `right_elb` is used by nurbs too.
laurynas marked this conversation as resolved
Laurynas Duburas added 1 commit 2024-03-18 07:04:50 +01:00
Laurynas Duburas added 1 commit 2024-03-18 08:25:09 +01:00
Jacques Lucke merged commit 15dbfe54e4 into main 2024-03-19 10:39:25 +01:00
Laurynas Duburas deleted branch bezier-handles-for-new-curves 2024-03-19 10:40:48 +01:00
Sign in to join this conversation.
No reviewers
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
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#119053
No description provided.