Page MenuHome

Curves editmode: edit points are drawn from evaluated curves
ClosedPublic

Authored by Philipp Oeser (lichtwerk) on Oct 18 2022, 11:30 AM.

Details

Summary

Editmode should display the original (non-evaluated) points unless there
is something like an "On Cage" option of a modifier [which none of the
curves modifiers have].

This was not the case since the introduction in rBe15320568a29.

So we now draw the editpoints from the original curves. This also means
that original and evaluated curves might not have the same number of
points, so we have to get independent of proc_point_buf since that
would possibly create vertexbuffers of different sizes (compared to the
original curves) which is not allowed for a single batch.

  • remove the "pos" alias from the vertex buffer format of proc_point_buf
  • instead, create a new "edit_points_pos" vertex buffer
  • fill that with the original (un-evaluated) curves positions
  • dont request proc_point_buf anymore
  • rename the editpoints flags buffer to be more consistent

And since original and evaluated points might also be in completely
different positions, we also need to draw connecting lines between those
editpoints to not have them float in thin air. For drawing these in
editmode, a simple polyline was chosen (instead of drawing lines in a
resolution that is take from the old particle system -- which is not
depending on points even but has a hardcoded resolution that can only be
upped by the hair_subdiv scene render setting)

  • create appropriate batch and indexbuffer for this
  • positions vertex buffer can be reused
  • reuse particle edit shader (instead of curve edi shader) to get segment highlighting

NOTE: this also removes the broken depth handling and instead makes it work (also XRay is properly taken into account) by binding the correct overlay framebuffer.
NOTE: to further clarify the distinction between curves drawing (that is based on the old partice system drawing) and drawing in editmode, the corresponding vertexbuffers have been moved out of CurvesEvalCache into CurvesBatchCache directly.
NOTE: drawing the lines in editmode could be improved (taking the "real" resolution of splines into account, but since this should happen on the GPU in a compute shader, this is for later)

Potentionally fixes T101889.

Diff Detail

Repository
rB Blender

Event Timeline

Philipp Oeser (lichtwerk) requested review of this revision.Oct 18 2022, 11:30 AM
Philipp Oeser (lichtwerk) created this revision.
Philipp Oeser (lichtwerk) edited the summary of this revision. (Show Details)

I was a bit skeptical of the use of the depsgraph to get the original ID initially, since I had trouble making that work with the legacy curve type and resorted to adding edit mode pointers to Curve. But it would be great to avoid that, and the solution here seems to work even with instancing.

However, I'm running into a problem with changing the curves in geometry nodes:


This started out as the default "Random" primitive. Any ideas?

source/blender/draw/intern/draw_cache_impl_curves.cc
304

clang format

314–317

A simple copy is more obvious here and should be faster.

749

Would DEG_get_original_id(curves) make more sense, since it's the original curves data-block we're drawing?

Hans Goudey (HooglyBoogly) requested changes to this revision.Oct 19 2022, 12:51 AM
This revision now requires changes to proceed.Oct 19 2022, 12:51 AM
source/blender/draw/intern/draw_cache_impl_curves.cc
749

Hm, the object ID has an orig_id and is LIB_TAG_COPIED_ON_WRITE whereas the curves ID does not have an orig_id and is not LIB_TAG_COPIED_ON_WRITE

Bit on shaky ground, will dig deeper... So, no, until further notice this would not work...

source/blender/draw/intern/draw_cache_impl_curves.cc
749

Oh, I think this might make sense. The evaluated curves might be totally new (e.g. created from geometry nodes), so it won't have the "orig ID" pointer. But the object should, at least in most cases. So just ignore this comment :)

However, I'm running into a problem with changing the curves in geometry nodes:


This started out as the default "Random" primitive. Any ideas?

Yeah, this is because in its current state the patch still requests the proc_point_buf vertex buffer for the batch (but this has a different size than the other vertex buffers -- which is seemingly not allowed -- because that uses the evaluated curves).
But we dont really need to use that vertex buffer (since we now have everything separated), so I have that solved in a local commit.
Will update shortly (in its current form, it also has the same "lifetime" issue that D16704 solves, that I will update as well), plus, I still need to draw the lines...
But: we are getting there.

  • correct lifetime of uint pos (make static)
  • dont rely on proc_point_buf anymore (solving issues with no-matching vertexbuffer sizes for same batch)

Here is a testcase @Hans Goudey (HooglyBoogly) was talking about (this now works)

The code change looks quite simple. I think it makes sense to merge this and worry about lines and other things later. It's enough of an improvement by itself.

ownership of batch cache is not entirely clear to me yet; is it COW or original object?

The batch cache should be owned by the evaluated object, original objects aren't drawn by renderers. That's my understanding anyway.

source/blender/draw/intern/draw_cache_impl_curves.cc
318–321

I'd expect GPU_vertbuf_attr_fill to work fine here. Faster and simpler than setting the data per-element in a loop.

  • review comments
  • add (poly for now) lines between editpoints
Philipp Oeser (lichtwerk) retitled this revision from [WIP] Curves editmode: edit points are drawn from evaluated curves to Curves editmode: edit points are drawn from evaluated curves.Dec 9 2022, 2:30 PM
Philipp Oeser (lichtwerk) edited the summary of this revision. (Show Details)

Hi Philipp, some feedback after testing the patch:

  • Drawing should respect depth (unless x-ray is on).
  • Better to only draw the lines now when in wireframe mode (to keep the patch simpler).
  • When drawing lines, we should draw their selected segments with the selected color.

Agreed with the above from Dalai (we went over the patch together). I'll just add some thoughts about the lines though.

I see the point of having them when the evaluated result has been replaced or moved somewhere totally different, etc.
But for most common situations the edit mode lines are redundant with the existing evaluated curve lines, which are drawn with the material, etc.
So for most situations they won't be necessary. I think it will make more sense to have features that add the edit-mode lines instead.
Also, eventually I'd hope that the edit mode lines would be smoothly evaluated on the GPU just like the the regular evaluated curve. But best to leave that out for now.

Hi Philipp, some feedback after testing the patch:

Thx for the feedback!

  • Drawing should respect depth (unless x-ray is on).

That was also wrong before (maybe this should be kept as a separate bugfix? we could also include it here, but this deviates from the subject "orig vs evaluated" -- no strong opinion here)

  • Better to only draw the lines now when in wireframe mode (to keep the patch simpler).

Afraid I dont get it, you would want floating points in solid? (ah, was missing @Hans Goudey (HooglyBoogly) 's clarification, thinking about it now...)

  • When drawing lines, we should draw their selected segments with the selected color.

Old bezier curves also dont highlight their segments, you mean like the old particle hair, right?

respect depth

maybe this should be kept as a separate bugfix?

Totally fine with me!

Old bezier curves also dont highlight their segments, you mean like the old particle hair, right?

Yeah, that was the thought. But if this is that complicated, fine to leave it to a separate commit IMO.

  • segment highlighting on selection for connecting lines (like the "old" hair system)
  • fix depth/XRay
Philipp Oeser (lichtwerk) edited the summary of this revision. (Show Details)

Depth/XRay is fixed, segment highlighting is in.
Still very skeptical about skipping line drawing...

It is fine to draw the lines, it mimics the behaviour we have for mesh editing. It would be nice if the line drawn on top of the other line wouldn't be so ugly, but it can be addressed later.

The lines isn't the best solution long term, but if others prefer having them now that's fine with me.

source/blender/draw/intern/draw_cache_impl_curves.cc
322

These aren't really flags anymore are they?
Do we really have to upload a float for every point when we're only concerned about whether it's selected or not?

source/blender/draw/intern/draw_curves_private.h
67

"Evaluated points" has a different meaning for curves, best to be clear here

source/blender/draw/intern/draw_cache_impl_curves.cc
322

Will rename that function.

Regarding the float: it is not deal, but the patch is now reusing the old particle line/point shaders (and these used a float to be able to show the weight gradient on the lines). We could get it back to just a flag, just dont think there is an existing shader that is capable of both selection and segment highlighting -- we would have to add one I think.

  • rename function
  • improve comment
Hans Goudey (HooglyBoogly) added inline comments.
source/blender/draw/intern/draw_cache_impl_curves.cc
322

Okay, that will be much easier to change when the particle system is gone then. It's probably not worth adding a new shader right now.

This revision is now accepted and ready to land.Dec 14 2022, 3:33 PM