T53158: VSE Scene Strip Render Engine Settings
Needs ReviewPublic

Authored by Danrae Pray (spockTheGray) on Nov 20 2017, 3:39 AM.

Details

Summary

T53158: VSE Scene Strip Render Engine Settings

  • VSE scene strips ('Sequence' types in code) now have their own ViewRender engine settings - by default, scene strips are rendered using *current* scene's ViewRender settings. This can be optionally overridden per-strip.
  • Moved strip preview settings from the VSE 'Preview' view to the 'Sequencer' view since they are now 'per-strip' (can be found under "Preview Settings" panel in "Strip" tab as shown below)

Diff Detail

Repository
rB Blender
Branch
arcpatch-D2924_3
Build Status
Buildable 1050
Build 1050: arc lint + arc unit
Danrae Pray (spockTheGray) edited the summary of this revision. (Show Details)
Campbell Barton (campbellbarton) requested changes to this revision.Dec 6 2017, 3:04 PM

From this discussion we agreed to keep the default option to use scene engines, with optional override. T53158

This revision now requires changes to proceed.Dec 6 2017, 3:04 PM

One of the issues i see is that if you set one strip to use Cycles, and other one to use Eevee, and then hit "Render Animation", what will happen is:

  • Cycles trip will render all fine.
  • When render pipeline will start rendering Eevee strip, Blender will likely crash.

This is because OpenGL can not be used from a non-main thread (not currently, at least). This is something what will also happen in compositor, and this issue is to be resolved anyway.

So do we accept such a known broken state for the time being, and make changes in sequencer anyway?

source/blender/editors/render/render_update.c
192–193 ↗(On Diff #9575)

Why is this needed?

source/blender/editors/space_sequencer/sequencer_add.c
315

This is not default, this is just copying, if scene changes engine, the sequencer wont follow.

@Campbell Barton (campbellbarton) thx for feedback - added inline replies. RE @Sergey Sharybin (sergey) 's comment - should I continue work on this or hold off for now?

source/blender/editors/render/render_update.c
192–193 ↗(On Diff #9575)

Was my attempt at clearing the VSE cache when the user changes preview engine for strip. If user has previewed 3 frames (say 1, 2, 3) w/ eevee as preview engine, then switches to clay, frames 1, 2, and 3 would still show the eevee frames (since they are cached, they don't get re-rendered I think) even though clicking on other frames renders w/ clay as expected. Seems to work in initial tests.

source/blender/editors/space_sequencer/sequencer_add.c
315

Ah, sorry I misunderstood. thought "default" meant just at strip initialization - didn't know strip settings needed to stay (optionally) synced w/ scene render settings thereafter too. I'll fix this

Think its fine to continue w/ this patch, the issue @Sergey Sharybin (sergey) mentions will exist elsewhere in Blender (compositor for eg), so think it can be resolved separate from this patch and is no reason to postpone it.

source/blender/editors/render/render_update.c
192–193 ↗(On Diff #9575)

Shoudn't this just clear the cache for a single strip?

source/blender/editors/space_sequencer/sequencer_add.c
315

The intention is - by default this can follow the scene, ability to change the engine is an optional override.

This avoids a situation where you might need to mess with 100's of strips engine setting because there is no good way to simply use whatever the scene has set.

Danrae Pray (spockTheGray) updated this revision to Diff 9679.EditedDec 10 2017, 3:07 AM

VSE scene strip's render engine settings now stay synced with scene
render engine settings by default. User also now has the ability to
optionally override this by checking a box in the scene strips settings panel.

Moreover, the VSE scene strip cache is now only cleared on a *per-strip*
basis when the scene render engine settings are changed *or* the strip's
render engine override settings are changed.

Danrae Pray (spockTheGray) marked 2 inline comments as done.
Danrae Pray (spockTheGray) added inline comments.
source/blender/editors/render/render_update.c
192–193 ↗(On Diff #9575)

Yes - sorry my first attempt was a bit off lol. Was a bit of a doozy - but I got it working in a slightly more sane manner this time :)

Campbell Barton (campbellbarton) requested changes to this revision.Dec 11 2017, 7:37 AM
Campbell Barton (campbellbarton) added inline comments.
release/scripts/startup/bl_ui/space_sequencer.py
606

Should be strip is not None

source/blender/blenkernel/intern/sequencer.c
3355–3361

The boolean should be checked here seq->use_scene_render_engine using a scene or seq depending on it's value.

5107–5130

This is not a reliable way of ensuring the correct value is used.

Strips can be copy-pasted for example. An override should be a flag which is checked at runtime.

source/blender/editors/render/render_update.c
192–193 ↗(On Diff #9575)

Think it would be better to have a separate function for the sequencer,

eg:

void ED_render_engine_changed(Main *bmain);
void ED_render_engine_changed_sequencer(Main *bmain);
199 ↗(On Diff #9679)

Logic here is a bit strange too, the engine has not changed for the current scene, it is for a sequence strip within the current scenes Edit struct.

So the update call looks wrong. If for some reason this is correct, that should be explained in a comment since it's not intuitive.

source/blender/editors/space_sequencer/sequencer_add.c
315

Again, why is this being done? the engine used when adding a strip shouldn't matter.

source/blender/makesdna/DNA_sequence_types.h
209–210

Why not make this a flag? re-use SEQ_USE_COLOR_BALANCE for eg.

source/blender/makesrna/intern/rna_sequencer.c
731

Why not expose sequence.view_render using the ViewRenderSettings type?

Then there is no need to duplicate it's logic.

This way instead of accessing seq.engine, seq.view_render.engine would be used (from Python).

731–767

This is too much duplication, logic should be shared with rna_ViewRenderSettings_engine_* functions, Check on how enum functions are shared already.

This revision now requires changes to proceed.Dec 11 2017, 7:37 AM
source/blender/editors/render/render_update.c
199 ↗(On Diff #9679)

Correction, when I made this comment I assumed BKE_sequencer_scene_render_engine_changed was just some updating function. As it turns out I think it should not be needed at all. so perhaps all changes here can be removed.

Updating D2924: T53158: VSE Scene Strip Render Engine Settings

  • We now key off an override flag in Sequence to decide whether to render using scene ViewRender directly or use sequence ViewRender.
  • Removed code attempting to handle clearing sequence strip cache & send notifiers when ViewRender is updated for strips / scene - this is left as a TODO for now.
Danrae Pray (spockTheGray) marked 6 inline comments as done.Dec 12 2017, 4:30 AM

@Campbell Barton (campbellbarton) made fixes per our conversation in IRC - thanks again :)

source/blender/makesdna/DNA_sequence_types.h
209–210

Wasn't sure what flags were OK to use still - thanks :)

source/blender/makesrna/intern/rna_sequencer.c
731–767

Per our talk in IRC, I just removed this for now and left as a TODO to clear strip cache on ViewRender.engine updates.

Sergey Sharybin (sergey) requested changes to this revision.Dec 13 2017, 12:50 PM

You can not re-use DNA flags, those might be saved in old files made by Blender where those flags were not deprecated yet. While we are on this, other deptrecated flags sequence flags could be cleared as well.

This revision now requires changes to proceed.Dec 13 2017, 12:50 PM
source/blender/makesrna/intern/rna_sequencer.c
1477

since this is a boolean, should use use_ prefix. eg: use_view_render_override - since view render may contain other settings in the future.

Example of commit that clears/reuses flags rB214bbd4c027518c24bcd8e7eaf5226453e2e978c - its for user preferences, something similar is needed for sequencer strips.

Update D2924 for T53158:

This revision now handles scene->view_render & sequence->view_render changes from UI appropriately. Moreover, cleared deprecated sequence->flag bits.

Some small python style issues.

release/scripts/startup/bl_ui/space_sequencer.py
588

Style: Expected 2 blank lines before class definitions (E302)

1167

Trailing spaces

release/scripts/startup/bl_ui/space_sequencer.py
604

It's odd this is shown before checking if it could be None, probably this panel should have a poll that checks for a scene?

Also, strip.use_view_render_override is not allocated and can never be NULL.

source/blender/blenloader/intern/versioning_260.c
1885–1888

This only clears for old files. The flag may remain in files re-saved from 2.6x.

A reliable way to handle this would be to check if "view_render" exists in "Sequence".

See: DNA_struct_elem_find use in versioning code for an example.

Doing so means this block of code is redundant.

source/blender/makesrna/intern/rna_layer.c
993

The state of the flag doesn't matter - it could be enabled or disabled AFAICS.

Also, this function could be generalized so we don't need to write a new one for each struct member. SequenceSearchData can store an offset. then access the member with POINTER_OFFSET

See: source/blender/editors/armature/armature_select.c:933: for similar code.

source/blender/makesrna/intern/rna_sequencer.c
714

No need for case insensitive comparison, STREQ macro can be used instead.

Updated D2924:

  • Fixed strip preview panel checks
  • Fixed versioning code
  • Genericized method for finding sequence from ListBase by member address
  • Other small housekeeping bits
Danrae Pray (spockTheGray) marked 7 inline comments as done.Dec 22 2017, 5:36 AM

Made updates based on feedback.

release/scripts/startup/bl_ui/space_sequencer.py
604

Sorry this was sloppy for sure - nice catch :) Updated to use poll as other classes do...