Page MenuHome

Fix for a crash in Freestyle in Blender 2.8 when using Cycles
AbandonedPublic

Authored by Tamito Kajiyama (kjym3) on Feb 5 2018, 8:09 AM.

Details

Summary

This patch addresses a crash in Freestyle in the Blender 2.8 branch when using the line renderer with Cycles.

The crash occurs in a call of RNA_property_copy() -- see the removed part in the patch (the old code was working in 2.7x but seems no longer valid in the 2.8 branch). The modified part of the code is intended to copy the Cycles options from one scene to another, so that the settings in the user-defined scene are used also for a temporary scene auto-generated by Freestyle for rendering of stylized lines.

The proposed revision changes two things:

  1. The way how the Cycles settings are copied. Now only the Cycles options relevant for Freestyle stroke rendering are explicitly listed in the code and copied one after another.
  2. The timing when the copying is done. The old code was doing the copy operation in a wrong place, and copying was unnecessarily repeated for every single Freestyle stoke.

Diff Detail

Repository
rB Blender

Event Timeline

Brecht Van Lommel (brecht) requested changes to this revision.Feb 5 2018, 12:30 PM

This looks more like a workaround than a fix for the core issue. Why does the crash occur in the first place? Why only copy specific Cycles settings?

The crash I get seems to be due to this mistake in RNA_property_copy, which should be return false:

if (ELEM(NULL, prop_dst, prop_src)) {
    false;
}
This revision now requires changes to proceed.Feb 5 2018, 12:30 PM
Sergey Sharybin (sergey) added inline comments.
source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp
129–156

I would really suggest getting rid of type here. That would add yet another thing to keep in sync with Cycles. Worst thing is that we can not catch it at compile time, and this thing is not covered with regression test.

To me it seems at least logical to have some RNA utility function which will copy value from one RNA pointer to another one.

Not familiar with RNA too well, but maybe it's matter of using RNA_property_copy and keep iterator over specified properties only?

165

Ideally such switch should be moved to RNA's access, but for the record: case is to be indented.

Dalai Felinto (dfelinto) requested changes to this revision.Feb 5 2018, 3:35 PM

This issue was introduced specifically on 638afb9bd428fb697fb5136e8bb5f31b05458db7, aka the second part of static overrides. And as Brecht suggested I find strange that the fix has to be freestyle specific. @Bastien Montagne (mont29) can you take a look at it?

Thank you all for the prompt reviews, and thanks @Brecht Van Lommel (brecht) for spotting the cause of the crash. I confirmed that fixing the suggested line of code resolved the crash. But still the copying of Cycles options from one scene to another does not work. Even after the RNA_STRUCT_BEGIN(&freestyle_cycles_ptr, prop) ... RNA_STRUCT_END block as in the removed code block, the Cycles parameter values remain the default ones (e.g., cycles.samples is 128 no matter how it is set in the user-defined scene). Any suggestions of where to check out?

I've committed a fix for the bug in RNA_property_copy in rBa8a77609d3bd: Fix Cycles + Freestyle rendering crash, due to bug in RNA override code.. Freestyle rendering seems to work after that, but it's slower than master here.

If there's other reasons to copy only specific Cycles settings, it would be good to understand them.

I missed your reply before I wrote mine. If it's rendering with the default 128 samples that would explain why it's rendering slower. Let me see why the copying is not working..

I've committed a fix for this in rB354f92a49458: Fix Freestyle not copying Cycles properties to stroke rendering scene. now. I don't see a reason to do the copying through RNA in the first place, the code can be simpler and more generic.

There is something broken in RNA through, for that I've created T54005: Copy to selected not working for Python defined properties.