This patch contains the work done in the filebrowser area, in relation to asset engine project.
AbandonedPublic

Authored by Bastien Montagne (mont29) on May 21 2015, 3:53 PM.

Details

Summary

It basically rewrites most of filelist.c, with some more limited changes in other areas of filebrowser.

From user perspective, it:

  • Removes some info in 'long' drawing mode (owner, permissions) - OS-specific data that do not really matter in Blender!
  • Makes short/long display 'fixed' size (among four choices, like thumbnails mode).
  • Allows to list several layers of dirtree at once, in a flat way (inside .blend files and/or real directories).
  • Consequently, adds datablocks types filtering.
  • Uses way less RAM when listing big directories, especially in thumbnail mode (we are talking of several hundred of MiB saved).
  • Generates thumbnails way faster.
  • Adds preview to Object, Group and Scene IDs, along with some basic pyscripting to generate such previews.
  • Exposes ID previews to RNA.

From code perspective, it:

  • Is ready for asset engine needs (on data structure level in filebrowser's listing).
  • Simplifies and makes 'generic' file listing much lighter.
  • Separates file listing in three different aspects:
    • 'generic' filelisting (in BLI), which becomes a shallow wrapper around stat struct.
    • 'filebrowser drawing' filelisting, which only contains current visible subset of the whole list (sliding window), with extra drawing data (strings for size, date/time, preview, etc.).
    • 'asset-ready' filelisting, which is used for operations common to 'basic' filehandling and future asset-related one.
  • Uses uuid's to handle file selection/state in the browser, instead of using flags in filelisting items.
  • Uses much lighter BLI_task handling for previews, instead of heavy 'job' system (adding a 'notifier' timer to handle UI refresh, in similar way to jobs).
  • Moves .blend datablocks preview handling to IMB_thumbnail (necessary to avoid storing all datablock previews at once, and gives better consistency and performances too).

Notes:

  • This patch is big! It's most probably not yet ready for master, but would like to get first review asap - and ideally merge as much as possible of it after 2.75 is released.
  • Final merge will be split in a few commits - but unfortunately, most changes in filelist.c are too much inter-related to be separated now.

Diff Detail

Repository
rB Blender
Branch
asset-experiments
There are a very large number of changes, so older changes are hidden. Show Older Changes

This is quite a large change, which should be easy to split into 3-4 independent patches like core changes in the file space, support of previews for object/groups, generation of previews i.e.

For now jut some basic feedback, more is to come after finishing reading the full patch..

source/blender/blenkernel/intern/idcode.c
185

Do we really need separate filter thing? It seems rather 1:1 mapping to ID codes here.

source/blender/blenkernel/intern/object.c
1564

I'm not sure object copy should copy previews. Several areas creates temporary copies of object for which you don't really need previews and don't want time/memory to be wasted.

source/blender/blenlib/intern/BLI_filelist.c
255

Why not to support terabytes. With alembic files around that'd make files list easier to read.

source/blender/editors/space_file/file_draw.c
265–266

Not totally clear why do we need to duplicate path now actually Worth mentioning this somewhere.

source/blender/editors/space_file/file_ops.c
1340

Use fullstop insteaod of bangs in comments.

Campbell Barton (campbellbarton) requested changes to this revision.May 29 2015, 8:44 AM

Reading over patch, generally nothing alarming or worry some :)... though this will take some multiple passes.

  • If this patch could be split at all, that'd be nice... but realize if split is extensive work... not really worth doing either. - Just to say if there are superficial edits that make patch noisy, these could be applied to master after release.
  • Would be good to explain what advantages GHash/UUID has over flag selection (reply here is OK but better include comment in code).
release/scripts/modules/bl_previews_utils/bl_previews_render.py
2

Would prefer to review this separately to general file-space changes.

release/scripts/startup/bl_operators/wm.py
2121

*picky* - would prefer to have release/scripts/startup/bl_operators/file.py - wm.py is is already getting a bit messy.

2165

would just use a list in this and append.

2167

nice to pass -noaudio too :)

2177

I never needed to do this? - what fails if its removed?

source/blender/blenkernel/intern/idcode.c
188

suggest to use macro which expands suffix... one line per case, no chance of typos. eg: CASE_IDFILTER(AC);

source/blender/blenloader/intern/writefile.c
606–607

*picky* - spaces around ops.

source/blender/editors/include/UI_interface.h
474–475

*picky*, would put use_free last

source/blender/editors/space_file/file_ops.c
171–172

Could be const?

331–333

Could be const?

source/blender/editors/space_file/filelist.c
926–981

Would make this into a UI_ function.

This revision now requires changes to proceed.May 29 2015, 8:44 AM
Bastien Montagne (mont29) marked 10 inline comments as done.May 29 2015, 10:55 AM

Thanks for this first pass (yes, of course, will need much more ;) ). Addressed most points raised so far, updated patch is incoming.

Re splitting

I can do it for sure, will have to anyways before final commit into master. Issue is, my other branch (asset-engine) is based on that one, so if I split it in three or four parts then… A bit afraid of having to manage that.

Re GHash for select states

Added quick comment in code. Thing is, I put everything not strictly related to filelisting outside of direntry struct, for two reasons:

  • Save space (important when you end up with several tens of thousands of items in the list).
  • Consistency (this BLI part of filelisting may be used elsewhere in code, no reason to store filebrowser-specific stuff here!).

Furthermore, we now only create 'full' items (needed for drawing) only on demand, so we cannot use them to store that kind of persistent data.

Note this will be even more critical with asset engines, since in this case filebrowser does not have any complete listing of available assets, it just query asset engine about total number, and to get small subset of items actually visible…

release/scripts/startup/bl_operators/wm.py
2177

Not sure which parser you are using, but I can assure you argparse needs this, otherwise it 'eats'/ignores the first given arg...

source/blender/blenkernel/intern/idcode.c
185

Yes we need them, FILTER_ID_XX is bitflag... Mandatory to combine several ID types in a same filter (integer).

And we cannot do 1:1 mapping, because we have more than 32 IDs (see comment in DNA_ID.h). :/

source/blender/blenkernel/intern/object.c
1564

Hmmm… Here I just followed what was done for existing IDs with previews (materials, textures, etc.).

I think we should keep same behavior regarding previews for all ID types. Also, copying previews might take a small amount of time, but generating previews (especially for objects, groups and scenes) is much more heavy process, so… not sure.

source/blender/blenloader/intern/writefile.c
606–607

Hehe, this is just a raw oving of the function from below in code… But yes, good occasion to clean it up :P

source/blender/editors/include/UI_interface.h
474–475

well… it only applies to path, so… maybe renaming it 'use_path_free' then?

source/blender/editors/space_file/file_draw.c
265–266

Path is now generated from root + relpath, no more static in filelist. Added some comment about it.

Bastien Montagne (mont29) marked an inline comment as done.
  • Refactor code in multi-append/link operator.
  • Address most points from first reviewing pass.
  • Tweaks and fixes in py part (preview handling).
  • Add comment about why we need to store fileselection state outside of filelist items now.
source/blender/editors/space_file/filelist.c
1979–1980

Use switch-case which will be compiled into a jump table. Not as if it's some bottleneck part of code, but still nice to avoid sloppyness when it's simple to do :)

2386–2389

Dead code is forbidden.

source/blender/editors/space_file/filesel.c
206

Do we really need that granularity level for filtering?

We had really similar discussion in the outliner filtering task, and the agreed way to go there was to use less granular filter flags (like Shading, Geometry, Light).

release/scripts/startup/bl_operators/wm.py
2117

Its possible to use argparse in a way that its not needed. See: release/scripts/templates_py/background_job.py -> parser.parse_args(argv).

source/blender/editors/include/UI_interface.h
474–476

think in the context of the function its obvious that its only applying to the path.

source/blender/editors/space_file/filelist.c
2526

Is there a good reason to use queue instead of BLI_task?

source/blender/editors/space_file/filesel.c
206

Its a tricky one - but think it could be worth not hard-coding in filter categories.

Asset browsers are going to have to their own kinds of categories anyway.

Couldn't there be a more generic tagging system? (so browsing blend files loads in one set of tags that can be swapped out for other kinds of asset browsers).

Tags (asset categories) can have icons, descriptions for tooltips.


Think it could be better not to include the filtering code in this patch. Then add some filtering/tagging system separately.

Bastien Montagne (mont29) marked 3 inline comments as done.Jun 3 2015, 3:44 PM

Thanks for this second round, most points addressed in incoming update, other answers inlined.

source/blender/editors/space_file/filelist.c
2386–2389

That whole func is dead anyway, not used anywhere. Just kept it (more or less) in sync with other changes I made, in case I need this zombie later…

2526

First, I’m not sure it is worth creating a task for each and every preview? Would mean potentially creating thousands of tasks in a very short amount of time…

But main reason is memory management. If we create one task per preview, then we have to pass FileListEntryCache *cache as pool userdata, and a single FileListEntryPreview *preview to each task's userdata - with free_taskdata set to true. But then, since preview will be systematically freed after filelist_cache_previewf is done, I’d need to duplicate it to add it to done queue…

So think using input and output queue, with limited number of working tasks, is more efficient here?

source/blender/editors/space_file/filesel.c
206

I would not add tagging system to 'simple' filebrowser. Asset Engines are responsible of filtering and sorting items anyway, so they handle this themselves (Amber does have tag system in asset-engine branch, e.g.). And adding this to core filebrowser would mean we need to store those tags in .blend file, with a way to edit them, and then read them while listing datablocks in file, etc. Would really want to keep 'basic mode' as simple as possible.

However, a way to sort datablocks by type is mandatory if we want flat view, otherwise your fileview explode under brushes and other 'useless' IDs like that.

So if we really do not want a per-ID filtering, I’d rather follow Sergey's idea and define more 'global' filter types (we'd still need per-ID values internally anyway), something like that:

  • Scenes
  • Animations (actions)
  • Objects & Groups
  • Geometry (meshes, curves, lattice, armatures, metaballs)
  • Shading (materials, nodetrees, textures, linestyles)
  • Images, Movieclips & Sounds
  • Environment (worlds, lamps, cameras, speakers)
  • Misc (brushes, text, font, paintcurve, palette, mask, grease pencil)
  • Address new review points.
  • Arg, missed those in previous 'review' commit.
  • Fix bad ID preview clearing in RNA API.
  • Fix error when batch-rendering preview of scene with no world.

Theres still some review work to be done, but general comment...

Being able to view all data-blocks at once is OK, am not totally against and recall you worked on this early on in the library browser work.
But how important is this view-everything-together option? Are artists really needing it?

This then adds the need to do more comprehensive filtering.
Perhaps its too much hassles to split this up forther as mentioned before, But would really like if its not too much trouble to just to have the patch first do internal data structure changes.

Then have UI changes added after and we can review more UI level changes and not worry so much about internal stuff.

Being able to view all data-blocks at once is OK, am not totally against and recall you worked on this early on in the library browser work.
But how important is this view-everything-together option? Are artists really needing it?

Imho it makes browsing items to be linked much much easier than previously… Also, (scarce!) feedback I got so far (see e.g. this thread) also sounds rather positive, but… it’s scarce, as usual. :(

This then adds the need to do more comprehensive filtering.

Imho by-type filtering is enough for now (together with by-name filtering that was added early this year), it already makes finding a given datablock much easier than it used to be. Really think more advanced filtering is asset-engine topic.

Perhaps its too much hassles to split this up forther as mentioned before, But would really like if its not too much trouble to just to have the patch first do internal data structure changes.

Well, thing is, this patch is 99% “internal data structure changes”. UI here is just a few lines of code.

Then have UI changes added after and we can review more UI level changes and not worry so much about internal stuff.

I really do not understand why we hit again the UI point tbh, this is detail that can be addressed any time, even in master, if you really want it I can 'disable' the 'flat listing' feature with a few line changes in space_file.py (and maybe in rna code), and we are done. But again, what’s important here is internal changes, the way we present it (UI) is easy to tweak, and quite out of scope of this review - especially since changes in this area are very limited compared to master.

  • Merge branch 'master' into asset-experiments
  • Fix/adapt new 'keyboard select' code to changes from branch.
  • Merge branch 'master' into asset-experiments
  • Fix some fileops operators that could try to get out-of-bound indices.
  • Make imbuf_thumb for .blend a bit less stupid & noisy regarding missing previews.
  • Selection/UUIDs handling enhancements:
  • Make selection & co really working even during listing process.
  • Cleanup: get rid of ugly bool in structs, replaced by bitflags.
release/scripts/modules/bl_previews_utils/bl_previews_render.py
157

Is it possible to reduce scope of the try-catch block?

source/blender/blenkernel/intern/object.c
1564

Thing of objects is that they're being created/copied to a temporary storage during Cycles and Freestyle renderers. There you don't really want extra memory usage.

But even in the viewport, you can't reliably copy review. For example, you Shift-D an object which uses world space texture mapping.

source/blender/editors/space_file/filelist.c
2386–2390

But do we need the function? :)

2529–2530

Ok. Still would love to de-duplicate all the threading blocks we've got. Thinking of similar thing we did for parallel for, could do the same here. But wouldn't consider this is to be changed prior the merge.

2658

It's a bit weird code. And more weird is why we don't need it anymore?

Some not-so-important picky comments.

release/scripts/startup/bl_operators/file.py
38–57

Picky, can use same indentation used elsewhere?

files = CollectionProperty(
        type=bpy.types.OperatorFileListElement,
        options={'HIDDEN', 'SKIP_SAVE'},
        )

also for other files...

76

odd name. call cmd ?

84–89

odd mixing append and list syntax...
would use

cmd.extend([
    ...
    ])
release/scripts/startup/bl_ui/space_filebrowser.py
87

*picky* no need for parens.

source/blender/blenlib/BLI_fileops.h
97–107

input values can be const.

source/blender/blenlib/intern/BLI_filelist.c
212–217

can call r_filelist

source/blender/editors/space_file/filelist.c
1808

This does a full linked list count, suggest to add BLI_thread_queue_is_empty to master, and call that.

Campbell Barton (campbellbarton) requested changes to this revision.Jun 19 2015, 11:06 AM

One issue that came up during the discussion is the use of the UUID,

These aren't really ensured to be unique... (realize in context of git it _can_ work... but in this case looks not totally reliable...).

Suggest to use a regular string hash. (ghash of strings).

Or we could string intern, (common practice, done in Python, OpenImageIO), then just to pointer comparisons. But in that case we'd want to be a but smart managing lifespan of interned strings (could clear at an appropriate moment - on refresh and its probably fine).

(implementation note)
String interning could use a gset of strings, where strings are allocated in a BLI_memarena so freeing can be done in one go..., but not sure of advantages here over ghash.

This revision now requires changes to proceed.Jun 19 2015, 11:06 AM
Bastien Montagne (mont29) marked 10 inline comments as done.Jun 19 2015, 1:19 PM

Thanks for this new round! Addressed most of points in upcoming update.

Regarding uuid.

I would really keep them as a four-integers array (i.e. raw 128 bits value). This is not only a matter of performances in ghash, this is also a question of memory, and is related to asset engines work. Those are not required to give a unique 'path' value for each of their entries - uniqueness is enforced at the uuid level (which in asset case will also be used to identify assets across time - file save & load, updates of asset, etc.).

In 'basic' filelisting however, those uuids must only be unique in the filebrowser listing, and in this context we know paths are all unique. I think the odds of a md5sum collision over a few hundreds of thousands of paths at most is rather “femtoscopic”, but if we want to be 100% rock solid here I would rather solve the issue locally (i.e. at the filelisting level, by either gset-ing all generated uuids to ensure their uniqueness, or by simply using a counter maybe?)…

release/scripts/modules/bl_previews_utils/bl_previews_render.py
157

The thing is, a (python) crash in this part is *very* bad from user point of view, because it would leave unwanted crap in its file (temp scene etc.), and that part was rather brittle in early stage of dev. So this is more like a safety stuff to report the error but not fail on it.

Still, can split this big block into smaller ones, will do.

source/blender/blenkernel/intern/object.c
1564

Oki, then we shall remove that copy from Groups as well I guess? Will do.

source/blender/editors/space_file/filelist.c
2386–2390

Good question, to which I do not have an answer yet. Thing is, we may need it again for asset engines (more precisely, to the 'create/edit assets' part, which is not fully defined yet). So… would rather keep it for now, easy to clean it up later in case we definitively do not need 'current' .blend file listing in filebrowser.

2529–2530

yes, see the point, but indeed not in scope of this patch. :)

2658

That comes from the *thumbnail previewing* job, which has been removed now. Just remaining 'dirt', removed.

Bastien Montagne (mont29) marked 3 inline comments as done.
  • Merge branch 'master' into asset-experiments
  • Various minor cleanup & fixes from review.
  • Remove copying of preview data for objects and groups datablocks.
  • Filebrowser internal listing & UUIDs: simpler & safer UUIDs generation.

Note about new UUID generation of internal file listing: switched to a simple incremental
counter, using atomic op. Much simpler and efficient than hashsum, and 100% guaranteed to
generate unique IDs. ;)

  • Merge branch 'master' into asset-experiments
  • Be much smarter with file size display...
  • Fix windows bug with scons builds.
  • Remove queing TODO previews when starting preview task, this is already handled
  • Previews: do not keep preview task and timer running continuously in 'Preview' mode.
  • Add API to IMB_thumb to enable thread safety.
  • UI-fix (bad 'recusion level' value in RNA enum, 1 is only valid in library listing case).
  • Expose fewest more generic ID filtering options.
  • Merge branch 'master' into asset-experiments

Committed in several steps, main part as rBf69e9681fa3.