Collection IO: Enable file exporters to be specified on Collections #116646
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
7 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#116646
Loading…
Reference in New Issue
No description provided.
Delete Branch "deadpin/blender:collection-io"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This implements the ability to have file exporters added and configured on Collections.
Exporting is reachable from several locations:
Export
button in each panel headerExport All
button in the main panel interfaceFile
->Export All Collections
buttonVisibility of which collections currently have exporters configured is done by ways of an icon added to the Collection row in the Outliner.
Adding multiple exporters for the same file type is permitted. The user is free to setup several exports of the same format but with different file locations or settings etc.
Notes:
Only USD and Wavefront OBJ are enabled for the initial commit. Additional formats, including those implemented in Python will be added as separate commits after this.
Ref #115690
I think this can done with with ui_list and draw the active one bellow, as is being proposed to modifiers (#111538)
And in the ui_list items can exist buttons to allow quick export for each IO handler
Maybe another concern is how to make operator presets to be supported with file handlers too
The trick on #116047 (comment) that gathers the
WMOperator
makes it work, but it is very rare to use a operator to do UI in that wayYou can add buttons on the panel title row, but on the right side of row. I'll give you example if you're interested in that.
But, why is such panel necessary in the first place? Blender has active collection, why not have them as operators (or extend existing operators) where you can toggle "Active Collection". Is there a benefit of having operator settings always exposed like that in a panel?
Regarding the "Export All" button, I think that should just be "Export". I don't think there needs to be support for exporting a single file type only. And it might get confusing with the planned functionality to export all collections.
I think we should call it "Export" in the UI anyway. If this supports import, I think we would either have a separate "Import" panel or an "Import & Export" panel.
"IO Handler" is too much of a technical programming term.
I think by far the most common case will be to have 1 exporter. For that reason I'm not sure having a list element here communicates things well.
I think the X button to remove could be done similar to modifiers.
For export, I think we only need a single button for all file types.
More details are in #68933 and #115690.
But basically, export is often not a one time thing. You may re-export the same objects many times, with the same settings. Having to pick the right file path and settings every time is slow and error prone.
And if you have a whole library of assets in your file, you could also set that up so you can re-export all of them to their own files, with a single click. It's similar to the way render output or compositing file output are persistent.
I'm wondering if perhaps we should support a static draw method on operators.
Alternatively we could just create an operator instance for the purpose drawing, even if it's a bit ugly. At least in terms of performance it should be a non-issue. But maybe there are consequences I'm not thinking of.
I'd expect these open/closed state here to be stored similar to most other panels in the properties editor. That is in the properties editor space, and with the same open/closed state as you select different objects. That way if you want to inspect or edit something in a particular subpanel for multiple objects, you don't have to reopen it again for each.
This is a bit different than modifiers, where an object may have multiple modifiers of the same type and the open/closed state can not be shared between those.
I have not looked at the implementation to check if this is easy to do in practice as well.
Not that I disagree with this design, I think its nice. But doesn't Blender operators currently remember settings you used last time operation was executed?
I export files 50+ times from each blend file, and I just set settings once and keep clicking button and it works the same.
In the same Blender session, yes. But not when you re-open the blend file potentially weeks or months later and need to remember where it all goes. Or maybe you work in a team with people, where you don't know which export settings the other person used.
You can also think of this in terms of (automated) production pipelines. Maybe you edit a material and want to re-export all assets that use it. If the export is set up in a blend file, a script can do that for you.
Buttons can be added in header preset area like this. @deadpin
Or with emboss. But I like without them better. Looks cleaner
This also would solve the issue of keeping the support to operators presets
@nickberckley The panel API I'm using is different than that for PanelType objects and it currently doesn't allow for changing the header.
@brecht The rationale behind also having an export on the individual Exporters was to alleviate long operations. If you're working on a large asset and want to setup handlers for USD, OBJ, and glTF, perhaps to upload to a market place, maybe you only want to export one of them for testing or whatnot. Only having a single Export will trigger all 3 exports all the time. It's a trivial additional/removal in any case though; can leave it out for first iteration.
If we do want the "X - Remove" button in the header row, like Modifiers, I'll have to investigate using parented Panels again or see about changing the new API to allow for it. I didn't get very far when attempting to use "instanced" PanelTypes and I switched to the new API last week when it landed.
For using op's draw, I tried that first before going with the new draw_export callback. While I could get things to draw correctly, modifications to the actual properties would not register and be saved properly. I think it's a lifetime issue since I only kept the op alive for drawing and immediately destroyed once complete. I'll need to investigate when/where to create and destroy it that still allows me to copy out property values.
Even if we use op's draw, the Python exporters are still going to need quite some modifications. Their draw's are all empty since they use PanelTypes with the help of the file browser to get things going.
Ok, I can see the point of it, it seems reasonable to keep.
Putting the X in the header seems a bit more correct than having it next to the file path where it could be seen as affecting just that path. But this level of polishing is not the most important thing right now.
I'd still be inclined to try to make the new API work well for this use case, but I also have not looked deeply enough at the practical challenges.
Maybe what is needed is to set up the RNA pointer with
owner_id
set to the collection datablock? This is what will cause the UI code to do an undo push and tag the depsgraph for update of that datablock.Yes, definitely. In general it would be nice to offer any operators the option to use a panel layout, for example in the redo panel. Not just for importers and exporters. That's why it makes some sense to me to keep the callback on the operators, though also there it could be a new static method called
draw_properties
or something.9a22bd2f25
to4ea79578f4
I ended up with something working. Seemed to be a combination of lifetime and ownership of the properties I provided to the operation on creation. What's here now is using just the existing
op->ui
callback instead of a new draw_export function. The UI labels have also been changed to not say "IO handler".The
op
I'm creating is just a shell of the real thing, unusable for anything else but drawing. I didn't use the primary way of creating one for two reasons. The first is that only code insidewm_event_system.cc
was creating ops and the new code doesn't live there. The other is due to the property ownership issue; ops like to make a copy of the provided properties. I didn't succeed in getting things working if it did that, and I think it would cause a good amount of additional house keeping to ensure I'm storing updated properties all the time anyways. Now the op is created in a way where it directly uses the properties in storage so things are always in sync.The
op
is "runtime" data and created as-needed right before it's used. It'll be kept alive for the entire session after that until it's asked to be destroyed either on shutdown or collection\exporter removal.I'll look into making the c++ panels more configurable here soon.
One possible implementation of allowing the panel headers to be configurable in the c++ code is here now. It's not very elegant with the boolean needing to be added to the main call though. Suggestions welcome.
I think a
uiLayoutPanelWithHeader
that returns both the header and body layout would make sense. They should be created together.RNA functions support returning multiple values for the Python API (see for example
rna_Object_ray_cast
), so I think exposing that to Python should work too.Regarding undo not working for the properties. Combined with #117640, the following patch should fix it.
@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
@ -417,0 +433,4 @@
bke::FileHandlerType *fh = bke::file_handler_find(name);
if (!fh) {
return OPERATOR_CANCELLED;
Add
BKE_report(op->reports, RPT_ERROR, "File handler '%s' not found'", name)
@ -417,0 +437,4 @@
}
if (!WM_operatortype_find(fh->export_operator, true)) {
return OPERATOR_CANCELLED;
Add
BKE_report(op->reports, RPT_ERROR, "File handler operator '%s' not found'", fh->export_operator)
@ -417,0 +446,4 @@
STRNCPY(data->fh_idname, fh->idname);
IDPropertyTemplate val{};
data->export_properties = IDP_New(IDP_GROUP, &val, "io_wmOpItemProp");
io_wmOpItemProp
is a bit of a strange name. Any reason it's notexport_properties
?@ -417,0 +462,4 @@
/* identifiers */
ot->name = "Add Exporter";
ot->description = "Add Exporter";
ot->idname = "COLLECTION_OT_io_handler_add";
Rename
COLLECTION_OT_io_handler_add
COLLECTION_OT_exporter_add
. I don't really see a reason to deviate from the user interface name here.@ -417,0 +466,4 @@
/* api callbacks */
ot->exec = collection_io_handler_add_exec;
ot->poll = WM_operator_winactive;
This needs a poll function that checks
CTX_data_collection(C) != nullptr
.@ -417,0 +501,4 @@
/* identifiers */
ot->name = "Remove Exporter";
ot->description = "Remove Exporter";
ot->idname = "COLLECTION_OT_io_handler_remove";
COLLECTION_OT_exporter_remove
@ -417,0 +505,4 @@
/* api callbacks */
ot->exec = collection_io_handler_remove_exec;
ot->poll = WM_operator_winactive;
Same comment about poll function.
@ -417,0 +510,4 @@
/* flags */
ot->flag = OPTYPE_REGISTER | OPTYPE_UNDO;
RNA_def_int(ot->srna, "index", 0, 0, INT_MAX, "Index", "IO Handler index", 0, INT_MAX);
IO Handler index -> Exporter index
@ -417,0 +568,4 @@
/* identifiers */
ot->name = "Export";
ot->description = "Invoke the export operation";
ot->idname = "COLLECTION_OT_io_handler_export";
COLLECTION_OT_export
perhaps.@ -417,0 +572,4 @@
/* api callbacks */
ot->exec = collection_io_handler_export_exec;
ot->poll = WM_operator_winactive;
Same comment about poll.
@ -417,0 +577,4 @@
/* flags */
ot->flag = OPTYPE_REGISTER | OPTYPE_UNDO;
RNA_def_int(ot->srna, "index", 0, 0, INT_MAX, "Index", "IO Handler index", 0, INT_MAX);
IO Handler index -> Exporter index
@ -417,0 +605,4 @@
/* identifiers */
ot->name = "Export All";
ot->description = "Invoke all configured exporters on this collection";
ot->idname = "COLLECTION_OT_io_export_all";
COLLECTION_OT_export_all
@ -417,0 +609,4 @@
/* api callbacks */
ot->exec = collection_io_export_all_exec;
ot->poll = WM_operator_winactive;
Same comment about poll.
@ -417,0 +651,4 @@
/* api callbacks */
ot->exec = wm_collection_export_all_exec;
ot->poll = WM_operator_winactive;
Same comment about poll.
@ -569,0 +571,4 @@
StructRNA *srna;
PropertyRNA *prop;
srna = RNA_def_struct(brna, "IOHandlerData", nullptr);
I wonder if we can just call this
CollectionExport
? I don't really see where we would generalize this. If we add import at some point I think it would be a separate data types, maybe sharing a base class if there is a reason.@ -569,0 +580,4 @@
RNA_def_property_ui_text(prop, "Is Open", "Whether the panel is expanded or closed");
RNA_def_property_flag(prop, PROP_NO_DEG_UPDATE);
RNA_def_property_update(prop, NC_SPACE | ND_SPACE_PROPERTIES, nullptr);
}
Can you add
exporter_properties
here, so scripts can modify the settings?I think you could add a pointer property with a custom get function (and not set function). The get function would then create a pointer with
RNA_pointer_create(owner_id, ot->srna, data->export_properties)
.If for some reason the operator type is not available, I think it can fall back to type
RNA_IDPropertyWrapPtr
.@ -653,1 +669,4 @@
"Children collections their parent-collection-specific settings");
/* IO Handlers. */
prop = RNA_def_property(srna, "io_handlers", PROP_COLLECTION, PROP_NONE);
Similarly, would call this just
exporters
.For the panel, I would suggest to remove IO Handler terminology since this is quite technical. And instead name the panel Exporters, with two buttons layed out horizontally.
@pablovazquez @Harley FYI, we are getting close to landing this. It would be great if the user interface team could have a look and check if they agree with the design decisions.
WIP: Enable file exporters to be specified on Collectionsto Collection IO: Enable file exporters to be specified on CollectionsToo early
Approved a bit too early, there is still this thing to optimize (and fixed so it takes into account which collections are actually in the current view layer).
As discussed, the plan is to detect any exporters in
BKE_view_layer_sync
and set a flag on the view layer (e.g.has_export_collections
). This ensures we are not looping over all collections in the UI code in more complex scenes.Everything else now looks good to me however.
One small thing. In the (unlikely) event that there are no file handlers available we get a popup notification:
We generally avoid informational popups. This situation could be checked for in COLLECTION_OT_exporter_add's poll function to disable the button in this case. Or if that is not desired (for performance reasons, etc) then just doing a BKE_reportf could be good enough - although our notifications do need some work.
Not an issue with this PR per se, but @pablovazquez I find the background color of these sections to be a bit dark. But that could just be me. This is just the theme Inner color for Box in case we want to change this. It looks fine to me in the Light Theme. Following with default dark theme:
I worry about the "Remove Exporter" button being so close to the "Export" button. It is really easy to hit that delete by accident and it just happens without warning, which could be frustrating. If they have to stay together then you could consider a simple confirmation for the deletion.
Yes, semi-performance related as it's not a simple boolean check. This is a Menu with a single Label as its only entry in the layout (see
collection_exporter_menu_draw
). I can change to a report though if that's better for now.This will change a bit, hopefully, soon after this lands as we'll stop using "box" layouts and switch to panels for the operator properties as much as possible. Each exporter needs to change so there will be several commits that trickle in. It would more typically look something like (only the garish
filepath
/preset boxes will remain):Ack. Will make your suggested change for an invoke confirmation.
Honestly not sure what to do with the "presets" button looking dark among its siblings.
We do that automatically for buttons that open menus and popovers (ui_item_menu, force_menu), but I find it odd here. Maybe just me though. Probably because everywhere else I see a "presets" button it isn't embossed.
But to remove the embossing on it might not be any better.
Oh, that does look nice!
There could be some problems with the selection of file paths. I'm testing using the Wavefront OBJ exporter.
By default the file path is a full path to a file, "//Collection.obj" or similar based on the collection name. So that works just fine.
You can also type in a full path to an export file, like "d:\temp\test.obj". Works great.
But typing in a full path is a pain so you might want to browse by clicking the folder icon. This works great if you select an existing file, but it seems unlikely that the export file will exist before you configure this.
The browsing process allows you to select a folder and that seems like a plausible way for this to work. As in the user might think that they specify an output folder and then this process creates an export file there.
But enter a path, like "D:\Temp" or "D:\Temp" and I get an error, "Cannot Open". This could check if the file path was a folder and append a file name?
More importantly though, if I use the "folder" icon to browse, but select a folder - which it allows - it will almost always select something nonsensical. For example, I can click on that D:\Temp folder, click Accept, and I have entered D:\Temp\Temp" or "D:\Temp\Documents" or some other combination. This is just the way we are appending what is in the file name input to the selected path. If I continue in this state and click "Export" I get an exception in MTLWriter::MTLWriter. In this particular case the obj_filepath being passed to that function is "D:\Temp\Logs\Documents" and neither "Logs" nor "Documents" exists there.
This probably just needs an earlier check to make sure the output path is valid.
@mont29
Can you take a look at commit
25407e6bbd
and see if it's correct? Brecht wanted this boolean so that we wouldn't have to iterate over the collections during UI draw. I tried to find the location which was already iterating over the collections and which did similar work. Not sure this covers all the cases I need to though or if it violates the design in some way.@Harley
For using Report instead of filing out the menu with "No file handlers available", I'm not sure the best place to put that as it would most likely mean duplicating the file handler search when filling out the menu. Maybe since this text will only be visible to devs who build "lite" builds, it is probably fine to leave as is. What do you think?
The directory / filename situation I'm still looking into. I'm surprised the browser is "helpfully" always putting a filename in place and it doesn't seem like I can influence it. The Render Output Properties filename is similarly affected though it will work because of how it appends to the resulting name. I'll instead see if I can provide the Exporters with a "more valid" filename when this happens instead.
Yikes, you're right. I was thinking these were enabled in Addons. So the way it is just fine. Sorry about that!
Yeah that was all bit odd and interesting, and probably just something you can't do anything about. It was mostly just an easy way to create an invalid path. I'm assuming you probably have one spot where you can test if the base directory is a valid directory, and then if there isn't a filename just append something generic.
Mainly answering @deadpin 's question here, plus a few other related notes found in same code. By no mean a full review.
@ -1222,2 +1222,4 @@
child_layer->runtime_flag |= LAYER_COLLECTION_VISIBLE_VIEW_LAYER;
}
if (!BLI_listbase_is_empty(&child_collection->exporters)) {
Just to be sure, it is intended that excluded collections are skipped from this test (see the
continue;
statement a few lines above)?Because afaict, below in export operators code, there is no check to skip such collection...
You're right. I do think they should be skipped when Excluded but I'll have to account for this in the export all operator.
@ -419,0 +465,4 @@
data->flag |= IO_HANDLER_PANEL_OPEN;
BLI_addtail(exporters, data);
Missing deg tagging of the modified collection (likely just using
ID_RECALC_SYNC_TO_EVAL
?).Same below in remove code.
@ -419,0 +468,4 @@
WM_event_add_notifier(C, NC_SPACE | ND_SPACE_PROPERTIES, nullptr);
WM_event_add_notifier(C, NC_SPACE | ND_SPACE_OUTLINER, nullptr);
BKE_layer_collection_sync(CTX_data_scene(C), CTX_data_view_layer(C));
This should not be called here (same for the remove op code below). Use instead
BKE_view_layer_need_resync_tag
, code that needs to get a valid viewlayer data is in charge of ensuring it itself (viaBKE_view_layer_synced_ensure
).Also, this should be called before deg tagging and notifiers are sent.
@ -419,0 +503,4 @@
BKE_collection_exporter_free_data(data);
BLI_remlink(exporters, data);
Most likely have no consequences here, but would rather remove the link from the listbase before starting to free its data?
Requesting changes
This is handling bad paths nicely now. But can wreck it with bad filenames.
I have an existing "D:\test" folder. Entering "d:\test\test.obj" works great.
Entering "d:\test\test.obi" (misspelling the extension), worked as expected, making a "test.obi" in that folder. And it works like this for any file that has an extension that is exactly three characters long.
But entering "d:\test\test.ob" results in a crash, as does "d:\test\test.o", "d:\test\test.objj", "d:\test\test.", etc. Looks to be an abort in MTLWriter::MTLWriter asserting that the extension be a particular length. Without that assert it will fail but just with a console error message because
BLI_path_extension_replace
fails to work.Might make sense to just check the extension or the length with your other filepath checks.
LGTM now, for what I checked.
Regarding extensions, I would not expect any expectation on length here? Even Blender files use
.blend
!The extension problem(s) were actually an existing bug in the OBJ exporter itself. I fixed it on Friday with another PR to behave better.
@Harley I think this is ready to commit now? As I mentioned on the call I'll address the emboss on the Presets button as a separate PR.
Yes, I agree. I'd rather get this functionality in so it is seen (and tested) by more people. After it is in we can deal with that button if need be, and maybe look at inconsistencies between the different exporters, etc.
@blender-bot build