Support for custom preview file for external render engine
Needs RevisionPublic
Actions

Authored by Andrey Izrantsev (bdancer) on Jun 27 2014, 4:58 PM.

Details

Reviewers

Thomas Dinges (dingto)
Sergey Sharybin (sergey)

Summary

Hey there!

This is a very simple patch that adds support for custom preview file for external render engine.
Usage:

class MyRendererPreview(bpy.types.RenderEngine):
    bl_idname = 'MY_RENDER_PREVIEW'
    bl_label  = "My Preview Renderer"

    bl_use_preview      =  True
    bl_preview_filepath = "/path/to/custom/preview.blend"

Best regards,
Andrey

Diff Detail

Event Timeline

This might work, but main concerns:

Seems path should be absolute, which is really bad. Would rather make it so path is measured relative to render ending addon root. It might also be a getter method which would say which file to use for the preview.
preview_prepare_scene() is still really hardocded to the BI/Cycles and current preview we're using. All that exceptions for texture preview, scene layers, object/textures naming and so. This is either to be communicated somehow with Blender in terms which primitives are supported there, how to prepare scene (or maybe just have RenderEngine.prepare_scene()?

source/blender/blenloader/intern/readfile.c
9408	Ideally we need to get rid of this and pass flags as arguments, but that's for another patch i guess.
source/blender/editors/render/render_preview.c
1136	Unneeded whitespace change.
1166	What's the reason of removing braces? We're tending to sue braces even for single-operator blocks. Changing such things is not really good together with adding new functionality..
source/blender/makesrna/intern/rna_render.c
296	Not sure it's really safe to do here. Tending to think of on-demand loading, so background rendering doesn't spend time on this. But it could be tricky. Maybe @Campbell Barton (campbellbarton) has strong opinion here.

Seems path should be absolute, which is really bad.

I prefer to manage paths from Python.

preview_prepare_scene()

Can't say anything about it, because I'm totally satisfied with my solution =)
User could easily create his own scenes, it just depends how particular addon will manage it.
For example, I'm storing default preview scene with addon and if user has preview.blend under user config directory, then this file will be used.

source/blender/editors/render/render_preview.c
49	Forget to remove this line, it's not needed.
1166	Hm, I have to read Blender's code style guide more carefully.
source/blender/makesrna/intern/rna_render.c
296	This is here because preview loading should be as fast as possible, so the idea was to init the scene on RenderEngine class registration. This has a disadvantage of needed to restart Blender (or addon) to load up a new preview scene. I've tried loading main somewhere in init preview, but then i have to do some synronization because preview is called on every param change without waiting for previous call end.
source/blender/render/extern/include/RE_engine.h
39	Included for PATH_MAX basically, may be better to include just limits.h.

I prefer to manage paths from Python.

You can't manage path which is stored in the property i think. Meaning, you wouldn't be able to move stuff around, like on copying previous blender version settings or running blender from usb stick.

Can't say anything about it, because I'm totally satisfied with my solution =)

That's the thing, i don't really want to expose something semi-finished and really fragile. The idea was to make it a nice configurable way, avoiding any sort of frustration because of the current way dealing with the previews.

source/blender/makesrna/intern/rna_render.c
296	Also meaning slower blender startup time because of loading previews .blend which users probably wound't need.

You can't manage path which is stored in the property i think.

This is how I'm using it right now: https://github.com/bdancer/vb30/blob/master/engine.py#L65 So, some function could easily do all the job and just return the final absolute path.

That's the thing, i don't really want to expose something semi-finished and really fragile.

I'm not sure it that fragile becase it works =) Anyway...

The idea was to make it a nice configurable way

Doest it really worth it? I mean, it's just a preview, final shader tweaking will be done on regular render anyway (becase of lighting and stuff).
Current solution allows to specify a file - no more no less, but better then nothing.

Campbell Barton (campbellbarton) added inline comments.Jul 24 2014, 6:39 PM

source/blender/makesrna/intern/rna_render.c
295	picky, can do `if (*et->preview_filepath) {...}`
296	Agree this should be done on demand, or at least not when registering.

Andrey Izrantsev (bdancer) edited edge metadata.Dec 8 2014, 10:41 AM

Andrey Izrantsev (bdancer) updated this revision to Diff 2980.Dec 8 2014, 10:44 AM

Andrey Izrantsev (bdancer) edited edge metadata.

I'm not happy with such a piercing of fully internal stuff to the outer world. Main issues:

As mentioned, this things totally relies on internal structure of the file, naming, layering hardcoded in blender sources.
Structure of the file is nowhere documented, meaning you can not really use it apart from doing some minor tweaks.

If we ever accept external .blend file support for preview renders the patch should:

Get rid of all the hardcoded naming, layering etc. Use information from the file itself about its layout.
Allow extending/replacing existing primitives. So it'll be able to use different set of primitives.

That's the minimum things.

Now, if you're interested in working on such flexible system we can discuss it in more details. If it's just to allow some customized preview file used by vray i'd suggest using the same approach as lux does -- ignore preview scene sent by blender and use own code-generated one. That'd be much more clear solution which wouldn't open a whole bunch of cans of worms.

This revision now requires changes to proceed.Dec 8 2014, 6:13 PM

Now, if you're interested in working on such flexible system we can discuss it in more details.

No, I'm not interested in this, I'm happy with this approach and will continue using it.

This will require crazy amount of work no one actually need - final shading tweaks are done via some final rendering anyway - preview is just for really simple previewing of stuff (because how shader looks depends a lot on the environment it's used in).

Honestly, I don't understand why this is such a big problem. The patch might not be a perfect solution, but we do compromises in other places too. The patch is not intrusive, it's just a few lines and when that helps external render engine developers, why not add it?

I agree in one point though, the layout/naming should be documented. But people can also just look at the preview blends we have for BI and Cycles.

@Thomas Dinges (dingto), it's not about compromises, it's about functionality improvements. And functionality wise this patch doesn't improve anything. You can not do anything apart form minor tweaks with the patch still, and minor tweaks are possible to do with the render engine api already.

For official public API we do need to solve current hardcoded nature of previews, otherwise you'll just lock everyone in a state when it kind of works for simple tweaks, but for something else you'll still need to other sorts of hacks. It's not terribly hard to finish and this patch is a good starting point, but the work is still to be done before we can consider it landable to master.

Campbell Barton (campbellbarton) removed a reviewer: Campbell Barton (campbellbarton).Aug 11 2016, 6:24 AM