Page MenuHome

Fix T96498: Curve object modifier affects all curves
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Apr 6 2022, 1:26 AM.

Details

Summary

The original mistake I made in b9febb54a492ac6c938 was thinking
that the input curve object data to BKE_displist_make_curveTypes
was already copied from the original. I think I misread some of its
ID flags. This commit places the result of curves evaluation in a
duplicated curve instead, and copies the edit mode pointers
necessary for drawing overlays. Curve needs to know not to
free those pointers.

I still don't have a full understanding of why some of the tactics I've
used work and others don't. I've probably tried around 8 different
solutions at this point, and this is the best I came up with.

The dependency graph seems to have some handling of edit mode
pointers that make the edit mode overlays work if the evaluated
result is only an empty curve created by the evaluated geometry set.
This doesn't work with the current method and I need to set the
edit mode pointers at the end of evaluation explicitly.

We're constrained by the confusing duality of the old curves system
combined with the new design using the evaluated geometry set.
Older areas of Blender expect the evaluated Curve to be a copy
of the original, even if it was replaced by some arbitrary evaluated mesh.

Diff Detail

Repository
rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Apr 6 2022, 1:26 AM
Hans Goudey (HooglyBoogly) created this revision.
source/blender/makesdna/DNA_curve_types.h
300–307

The description mentions removal of curve_eval, but it is still here?
It also feels that it should be in the runtime.

306

What about edit_data_shared to avoid double negations (if not edit_data_not_owned)?

Hans Goudey (HooglyBoogly) added inline comments.
source/blender/makesdna/DNA_curve_types.h
300–307

Ah, totally right, sorry. That was left over from an earlier attempted fix. I updated the description.

The changes look reasonably safe, but it's always hard to predict unintended consequences here...

source/blender/blenkernel/intern/displist.cc
1492–1493

This comment should be updated, it still mentions CurveEval.

source/blender/makesdna/DNA_curve_types.h
306

Guess the main reason for this name is that it has minimal impact on existing code?

This revision is now accepted and ready to land.Apr 7 2022, 1:45 PM
Hans Goudey (HooglyBoogly) marked 4 inline comments as done.
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)

Use a different method that simplifies code instead of making it more complicated

Hans Goudey (HooglyBoogly) planned changes to this revision.Apr 7 2022, 7:18 PM

Revert to the old method. Turns out we can't use the simpler solution because some areas of Blender actually do expect the evaluated Curve to be just a copy of the original Curve no matter what (new obj exporter happened to catch that in its tests).
I think this is where the design of the old system where an object can be drawn as a mesh but actually be another type is fundamentally broken and confusing.

This revision is now accepted and ready to land.Apr 8 2022, 7:35 PM

From the code side seems fine. Moves in the right direction anyway. Don't currently have a setup to test it though.

source/blender/blenkernel/intern/displist.cc
1475

I'd call it input_curve (input as in input of the modifier stack), to not be confused with original curve which is owned by bmain.

Hans Goudey (HooglyBoogly) planned changes to this revision.Apr 21 2022, 6:16 PM
  • Fix memory leak (change in object.cc)
  • Fix crash during animation rendering when ob->data == ob->runtime.data_eval (free derived caches before retrieving input curve).
    • This is the same thing the mesh modifier stack does basically. It's a delicate situation, but nothing new.
  • Update comments

Nothing fundamental has changed in the patch, but I'd like to make sure @Sergey Sharybin (sergey) and @Jacques Lucke (JacquesLucke) at least see the latest version before I commit it.

This revision is now accepted and ready to land.Apr 21 2022, 8:29 PM

Only checked the code. Let me know if you'd like me to run tests or check something specific.