Page MenuHome
Differential D2924

T53158: VSE Scene Strip Render Engine Settings
Needs ReviewPublic
Actions

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

Details

Reviewers
Dalai Felinto (dfelinto)
Campbell Barton (campbellbarton)
Sergey Sharybin (sergey)
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

Event Timeline

Danrae Pray (spockTheGray) created this revision.Nov 20 2017, 3:39 AM
Danrae Pray (spockTheGray) edited the summary of this revision. (Show Details)Nov 20 2017, 3:07 PM
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
Campbell Barton (campbellbarton) added a subscriber: Campbell Barton (campbellbarton).

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
Sergey Sharybin (sergey) added a subscriber: Sergey Sharybin (sergey).Dec 6 2017, 3:14 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?

Campbell Barton (campbellbarton) added inline comments.Dec 6 2017, 3:15 PM
source/blender/editors/render/render_update.c
192–193

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.

Danrae Pray (spockTheGray) added a comment.Dec 7 2017, 3:56 AM

@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

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

Campbell Barton (campbellbarton) added a comment.EditedDec 7 2017, 7:46 AM

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

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) edited the summary of this revision. (Show Details)Dec 10 2017, 3:11 AM
Danrae Pray (spockTheGray) marked 2 inline comments as done.
Danrae Pray (spockTheGray) added inline comments.
source/blender/editors/render/render_update.c
192–193

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
3325

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

5071–5094

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

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

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
723

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).

723–759

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
Campbell Barton (campbellbarton) added inline comments.Dec 11 2017, 2:41 PM
source/blender/editors/render/render_update.c
199

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.

Danrae Pray (spockTheGray) updated this revision to Diff 9688.Dec 12 2017, 4:25 AM

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
723–759

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

Danrae Pray (spockTheGray) edited the summary of this revision. (Show Details)Dec 12 2017, 4:47 AM
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
Campbell Barton (campbellbarton) added inline comments.Dec 13 2017, 12:52 PM
source/blender/makesrna/intern/rna_sequencer.c
1464

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

Campbell Barton (campbellbarton) added a comment.Dec 13 2017, 12:56 PM

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

Danrae Pray (spockTheGray) updated this revision to Diff 9719.Dec 20 2017, 4:04 AM

Update D2924 for T53158:

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

Vuk Gardašević (lijenstina) added a subscriber: Vuk Gardašević (lijenstina).Dec 20 2017, 4:38 AM

Some small python style issues.

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

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

1157

Trailing spaces

Campbell Barton (campbellbarton) added inline comments.Dec 20 2017, 10:57 AM
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 ↗(On Diff #9719)

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 ↗(On Diff #9719)

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.

Danrae Pray (spockTheGray) updated this revision to Diff 9730.Dec 22 2017, 5:32 AM

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...

Scott Wilson (propersquid) added a subscriber: Scott Wilson (propersquid).May 14 2020, 3:12 AM

Revision Contents
Changeset List

PathSize
release/
scripts/
startup/
bl_ui/
39 lines
source/
blender/
blenkernel/
intern/
2 lines
editors/
render/
8 lines
space_sequencer/
4 lines
makesdna/
4 lines
makesrna/
intern/
7 lines

Diff 9575

View Options

release/scripts/startup/bl_ui/space_sequencer.py

Loading...
View Options

source/blender/blenkernel/intern/sequencer.c

Loading...
View Options

source/blender/editors/render/render_update.c

Loading...
View Options

source/blender/editors/space_sequencer/sequencer_add.c

Loading...
View Options

source/blender/makesdna/DNA_sequence_types.h

Loading...
View Options

source/blender/makesrna/intern/rna_sequencer.c

Loading...