Details

Reviewers

Sergey Sharybin (sergey)
Jacques Lucke (JacquesLucke)

Maniphest Tasks

T91445: Convert curve object data to mesh does not work when curve does not evaluate to mesh

Commits

rBeb0eb54d9644: Fix D12533: Simplify curve object to mesh conversion

Summary

This patch simplifies the curve object to mesh conversion used by the
object convert operator and exporters.

The existing code had a convoluted model of ownership, and did quite a
bit of unnecessary work. It also assumed that curve objects always
evaluated to a mesh, which is not the case anymore.

Now the code checks if the object it receives is evaluated. If so, it can
simply return a copy of the evaluated mesh (or convert the evaluated
curve wire edges to a mesh if there was no evaluated mesh). If the
object isn't evaluated, it uses a temporary copy of the object with
modifiers removed to create the mesh in the same way.

This follows up on the recent changes to curve evaluation,
namely that the result is always either a mesh or a wire curve.

The new code works in my own testing, and the automated tests
pass as well.

Fixes T91445

Diff Detail

Repository: rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Sep 17 2021, 2:02 AM

Hans Goudey (HooglyBoogly) created this revision.

Harbormaster completed remote builds in B17081: Diff 41954.Sep 17 2021, 2:02 AM

Hans Goudey (HooglyBoogly) mentioned this in D12580: Move curve to mesh node implementation to geometry module.Sep 21 2021, 12:49 AM

Fix the problem with wire mesh conversion

Harbormaster completed remote builds in B17174: Diff 42130.Sep 21 2021, 1:07 AM

Hans Goudey (HooglyBoogly) retitled this revision from Cleanup: Simplify curve object to mesh conversion (WIP) to Cleanup: Simplify curve object to mesh conversion.Sep 21 2021, 1:11 AM

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)

Hans Goudey (HooglyBoogly) added reviewers: Sergey Sharybin (sergey), Jacques Lucke (JacquesLucke).

Hans Goudey (HooglyBoogly) added a parent revision: D12580: Move curve to mesh node implementation to geometry module.

Hans Goudey (HooglyBoogly) retitled this revision from Cleanup: Simplify curve object to mesh conversion to Fix/Simplify curve object to mesh conversion.Sep 21 2021, 1:13 AM

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)

Sergey Sharybin (sergey) added inline comments.Sep 21 2021, 9:34 AM

source/blender/blenkernel/CMakeLists.txt
481 ↗	(On Diff #42130)	This creates a dependency cycles between libraries. While this is something what historically is used in many areas, this is an indicative of weak architectural design of Blender modules and the way they interact. Why is there an entire module for a couple of functions? Why cant those be in the blenkernel avoiding dependency cycle?

Hans Goudey (HooglyBoogly) added inline comments.Sep 21 2021, 2:22 PM

source/blender/blenkernel/CMakeLists.txt
481 ↗	(On Diff #42130)	The module is meant to contain higher level geometry operations: T86869: Create new geometry-kernel module I was going back and forth about doing that, so thanks for having a stronger opinion, I will move it to blenkernel instead.

Hans Goudey (HooglyBoogly) mentioned this in D12585: Move curve to mesh node implementation to blenkernel.Sep 21 2021, 4:37 PM

Hans Goudey (HooglyBoogly) edited parent revisions, added: D12585: Move curve to mesh node implementation to blenkernel; removed: D12580: Move curve to mesh node implementation to geometry module.Sep 21 2021, 4:57 PM

Update for conversion in blenkernel instead

Harbormaster completed remote builds in B17199: Diff 42168.Sep 21 2021, 4:57 PM

Generally looks ok, better than before. Got a few inline comments.

source/blender/blenkernel/intern/mesh_convert.cc
879	Where is this checked for exactly? Why wasn't this necessary before?
944	double free
970–981	typo (`evalauted_object`)

This revision now requires changes to proceed.Sep 21 2021, 6:52 PM

Hans Goudey (HooglyBoogly) mentioned this in rB3642e1742844: Cleanup: Move curve to mesh node implementation to blenkernel.Sep 21 2021, 6:57 PM

Merge master after committing move of curve to mesh node
Slightly change comment

Harbormaster completed remote builds in B17204: Diff 42180.Sep 21 2021, 7:01 PM

Fix double free
Add more detail to comment

Harbormaster completed remote builds in B17207: Diff 42184.Sep 21 2021, 7:19 PM

Hans Goudey (HooglyBoogly) added inline comments.Sep 21 2021, 7:21 PM

source/blender/blenkernel/intern/mesh_convert.cc
879	I added more detail to the comment. The function I removed from `displist.cc` did not assign evaluated data. But it's better to use the same function here, since we are creating a duplicate fake object for evaluation anyway.
944	Thanks, good catch.

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Sep 21 2021, 7:23 PM

Actually fix typo

Harbormaster completed remote builds in B17208: Diff 42186.Sep 21 2021, 7:25 PM

Jacques Lucke (JacquesLucke) added inline comments.Sep 21 2021, 7:28 PM

source/blender/blenkernel/intern/mesh_convert.cc
879	The comment on `LIB_ID_COPY_SET_COPIED_ON_WRITE` says that `LIB_TAG_COPIED_ON_WRITE` has to be set as well. I wonder if there is a better way that avoids this flag hackery. Wouldn't be the first time though. And fortunately, this object is very short lived, so it might not be too bad.

Hans Goudey (HooglyBoogly) added inline comments.Sep 21 2021, 7:34 PM

source/blender/blenkernel/intern/mesh_convert.cc
879	I think "must" in the comment for this flag should actually be "will", that seems to describe the behavior. When evaluating temporary objects like this there will probably always be some hackiness. Maybe this whole area could change more in the future.

Jacques Lucke (JacquesLucke) accepted this revision.Sep 21 2021, 7:43 PM

Jacques Lucke (JacquesLucke) added inline comments.

source/blender/blenkernel/intern/mesh_convert.cc
879	An alternative would be to create a temporary depsgraph only for this object and its dependencies. That should be possible without too much hackery. Might be overkill for now, but something to keep in mind in the future. Maybe write that in a comment. I've only done that once a while ago while testing something with particle nodes, but I did some cleanup on the related api back then (rB6a4f5e6a8c399). One probably needs `DEG_graph_new`, `DEG_graph_build_from_ids`, `DEG_evaluate_on_refresh`, `DEG_get_evaluated_object` and `DEG_graph_free`.

This revision is now accepted and ready to land.Sep 21 2021, 7:43 PM

Closed by commit rBeb0eb54d9644: Fix D12533: Simplify curve object to mesh conversion (authored by Hans Goudey (HooglyBoogly)). · Explain WhySep 23 2021, 6:42 PM

This revision was automatically updated to reflect the committed changes.

Hans Goudey (HooglyBoogly) added a commit: rBeb0eb54d9644: Fix D12533: Simplify curve object to mesh conversion.

Fix/Simplify curve object to mesh conversion
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 42325

source/blender/blenkernel/BKE_displist.h

source/blender/blenkernel/intern/displist.cc

source/blender/blenkernel/intern/mesh_convert.cc

Fix/Simplify curve object to mesh conversionClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 42325

source/blender/blenkernel/BKE_displist.h

source/blender/blenkernel/intern/displist.cc

source/blender/blenkernel/intern/mesh_convert.cc

Fix/Simplify curve object to mesh conversion
ClosedPublic
Actions

Revision Contents
Changeset List