Collection IO: Enable file exporters to be specified on Collections #116646

Merged
Jesse Yurkovich merged 56 commits from deadpin/blender:collection-io into main 2024-04-08 22:10:52 +02:00

This implements the ability to have file exporters added and configured on Collections.

Exporting is reachable from several locations:

  • Individually on each exporter configuration: The Export button in each panel header
  • For all exporters on the collection: The Export All button in the main panel interface
  • For all exporters on all collections in the scene: The File->Export All Collections button

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


Start Single Multiple Outliner
coll-export-0.png coll-export-1.png coll-export-2.png coll-export-3.png

Ref #115690

This implements the ability to have file exporters added and configured on Collections. Exporting is reachable from several locations: - Individually on each exporter configuration: The `Export` button in each panel header - For all exporters on the collection: The `Export All` button in the main panel interface - For all exporters on all collections in the scene: The `File`->`Export All Collections` button Visibility 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. ---- | Start | Single | Multiple | Outliner | | --- | --- | --- | --- | | ![coll-export-0.png](/attachments/9617e278-f9e3-42be-a9c1-0ddfea1d8321) | ![coll-export-1.png](/attachments/1dbf63a7-fb72-4925-baed-b0ffe39d4a31) | ![coll-export-2.png](/attachments/d1bec267-a8b3-488c-bb65-68dd28157e63) | ![coll-export-3.png](/attachments/60633340-f8de-47d4-bda2-732f92b2107e) | Ref #115690
Contributor

I think this can done with with ui_list and draw the active one bellow, as is being proposed to modifiers (#111538)

image

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 way

I think this can done with with ui_list and draw the active one bellow, as is being proposed to modifiers (https://projects.blender.org/blender/blender/issues/111538) ||| |---|---| |![](https://projects.blender.org/blender/blender/attachments/34fb6591-c03c-4d1b-924c-8ba283320949)|![image](/attachments/b43b3b8b-6f07-4797-a267-81384703d0af)| 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 https://projects.blender.org/blender/blender/pulls/116047#issuecomment-1092682 that gathers the `WMOperator` makes it work, but it is very rare to use a operator to do UI in that way
Contributor

You 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?

You 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've been calling and labeling many of these things "IO Handlers", in both UI and implementation, rather than using "Export". This is in case "Import" will be considered in the future.

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 this can done with with ui_list and draw the active one bellow, as is being proposed to modifiers (#111538)

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.

It could be nice(er) to have the Export and Remove buttons for a given exporter available directly on the header row of the panel instead of right beside the file path; which would allow users to directly invoke the operation without having to first expand the panel.

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.

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've been calling and labeling many of these things "IO Handlers", in both UI and implementation, rather than using "Export". This is in case "Import" will be considered in the future. 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 this can done with with ui_list and draw the active one bellow, as is being proposed to modifiers (https://projects.blender.org/blender/blender/issues/111538) 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. > It could be nice(er) to have the Export and Remove buttons for a given exporter available directly on the header row of the panel instead of right beside the file path; which would allow users to directly invoke the operation without having to first expand the panel. 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.

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?

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.

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

This patch creates a new FileHandlerType.draw_export callback which is responsible for laying out the UI

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.

Where and how to store all the open/closed states seems a bit problematic. The states need to live someplace long-lived and durable. They get written to disk.

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.

> This patch creates a new FileHandlerType.draw_export callback which is responsible for laying out the UI 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. > Where and how to store all the open/closed states seems a bit problematic. The states need to live someplace long-lived and durable. They get written to disk. 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.
Contributor

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?

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.

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.

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

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

image

Buttons can be added in header preset area like this. @deadpin

Or with emboss. But I like without them better. Looks cleaner

image

def draw_header_preset(self, context):
	layout = self.layout
    row = layout.row(align=True)
    row.operator('mesh.primitive_cube_add', text="", icon="EXPORT", emboss=False)
    row.operator('mesh.primitive_cube_add', text="", icon="PANEL_CLOSE", emboss=False)
![image](/attachments/d8c09e37-4deb-46bb-b6f8-19711a784797) Buttons can be added in header preset area like this. @deadpin Or with emboss. But I like without them better. Looks cleaner ![image](/attachments/4b845c7d-3f2e-483d-b71f-ddbf6c7d0136) ``` def draw_header_preset(self, context): layout = self.layout row = layout.row(align=True) row.operator('mesh.primitive_cube_add', text="", icon="EXPORT", emboss=False) row.operator('mesh.primitive_cube_add', text="", icon="PANEL_CLOSE", emboss=False) ```
7.2 KiB
7.2 KiB
Contributor

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.

This also would solve the issue of keeping the support to operators presets

>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. This also would solve the issue of keeping the support to operators presets
Author
Member

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

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

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

Ok, I can see the point of it, it seems reasonable to keep.

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.

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.

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.

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.

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.

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.

> @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. Ok, I can see the point of it, it seems reasonable to keep. > 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. 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. > 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. 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. > 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. 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.
Bastien Montagne added this to the USD project 2024-01-04 16:13:49 +01:00
Jesse Yurkovich force-pushed collection-io from 9a22bd2f25 to 4ea79578f4 2024-01-05 08:26:56 +01:00 Compare
Author
Member

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

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 inside `wm_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.
Jesse Yurkovich added 2 commits 2024-01-08 20:08:32 +01:00
Author
Member

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

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. ![image](/attachments/c43a5eae-1c99-4e2a-8fda-06fccdd0bf34)

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.

> 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.
Jesse Yurkovich added 4 commits 2024-01-27 01:09:02 +01:00
Jesse Yurkovich added 1 commit 2024-01-29 02:37:14 +01:00

Regarding undo not working for the properties. Combined with #117640, the following patch should fix it.

diff --git a/source/blender/editors/interface/interface_templates.cc b/source/blender/editors/interface/interface_templates.cc
index e0c5c05cdcd..23f4cd797b9 100644
--- a/source/blender/editors/interface/interface_templates.cc
+++ b/source/blender/editors/interface/interface_templates.cc
@@ -2375,16 +2375,17 @@ static void minimal_operator_free(wmOperator *op)
   MEM_freeN(op);
 }
 
-void draw_export_controls(uiLayout *layout, const char *label, int index)
+static void draw_export_controls(uiLayout *layout, const char *label, int index)
 {
   uiItemL(layout, label, ICON_NONE);
   uiItemIntO(layout, "", ICON_EXPORT, "COLLECTION_OT_io_handler_export", "index", index);
   uiItemIntO(layout, "", ICON_X, "COLLECTION_OT_io_handler_remove", "index", index);
 }
 
-void draw_export_properties(bContext *C, uiLayout *layout, wmOperatorType *ot, IOHandlerData *data)
+static void draw_export_properties(
+    bContext *C, uiLayout *layout, ID *id, wmOperatorType *ot, IOHandlerData *data)
 {
-  PointerRNA properties = RNA_pointer_create(nullptr, ot->srna, data->export_properties);
+  PointerRNA properties = RNA_pointer_create(id, ot->srna, data->export_properties);
 
   uiLayout *box = uiLayoutBox(layout);
   uiItemR(box, &properties, "filepath", UI_ITEM_NONE, nullptr, ICON_NONE);
@@ -2415,12 +2416,12 @@ void uiTemplateCollectionExporters(uiLayout *layout, bContext *C)
       continue;
     }
 
-    PointerRNA io_handler_ptr = RNA_pointer_create(nullptr, &RNA_IOHandlerData, data);
+    PointerRNA io_handler_ptr = RNA_pointer_create(&collection->id, &RNA_IOHandlerData, data);
 
     PanelLayout panel = uiLayoutPanelWithHeader(C, layout, &io_handler_ptr, "is_open");
     draw_export_controls(panel.header, fh->label, index);
     if (panel.body) {
-      draw_export_properties(C, panel.body, ot, data);
+      draw_export_properties(C, panel.body, &collection->id, ot, data);
     }
   }
 }
Regarding undo not working for the properties. Combined with #117640, the following patch should fix it. ```diff diff --git a/source/blender/editors/interface/interface_templates.cc b/source/blender/editors/interface/interface_templates.cc index e0c5c05cdcd..23f4cd797b9 100644 --- a/source/blender/editors/interface/interface_templates.cc +++ b/source/blender/editors/interface/interface_templates.cc @@ -2375,16 +2375,17 @@ static void minimal_operator_free(wmOperator *op) MEM_freeN(op); } -void draw_export_controls(uiLayout *layout, const char *label, int index) +static void draw_export_controls(uiLayout *layout, const char *label, int index) { uiItemL(layout, label, ICON_NONE); uiItemIntO(layout, "", ICON_EXPORT, "COLLECTION_OT_io_handler_export", "index", index); uiItemIntO(layout, "", ICON_X, "COLLECTION_OT_io_handler_remove", "index", index); } -void draw_export_properties(bContext *C, uiLayout *layout, wmOperatorType *ot, IOHandlerData *data) +static void draw_export_properties( + bContext *C, uiLayout *layout, ID *id, wmOperatorType *ot, IOHandlerData *data) { - PointerRNA properties = RNA_pointer_create(nullptr, ot->srna, data->export_properties); + PointerRNA properties = RNA_pointer_create(id, ot->srna, data->export_properties); uiLayout *box = uiLayoutBox(layout); uiItemR(box, &properties, "filepath", UI_ITEM_NONE, nullptr, ICON_NONE); @@ -2415,12 +2416,12 @@ void uiTemplateCollectionExporters(uiLayout *layout, bContext *C) continue; } - PointerRNA io_handler_ptr = RNA_pointer_create(nullptr, &RNA_IOHandlerData, data); + PointerRNA io_handler_ptr = RNA_pointer_create(&collection->id, &RNA_IOHandlerData, data); PanelLayout panel = uiLayoutPanelWithHeader(C, layout, &io_handler_ptr, "is_open"); draw_export_controls(panel.header, fh->label, index); if (panel.body) { - draw_export_properties(C, panel.body, ot, data); + draw_export_properties(C, panel.body, &collection->id, ot, data); } } } ```
Jesse Yurkovich added 4 commits 2024-01-30 01:14:20 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
49725766b2
Merge remote-tracking branch 'upstream/main' into collection-io
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR116646) when ready.
Jesse Yurkovich added 9 commits 2024-02-13 08:59:07 +01:00

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR116646) when ready.
Jesse Yurkovich added 2 commits 2024-02-16 20:31:53 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
6634ed0f1b
Temp fix for Release builds
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR116646) when ready.
Jesse Yurkovich added 6 commits 2024-03-07 06:07:24 +01:00
Jesse Yurkovich added 1 commit 2024-03-08 20:28:31 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
ec9051a2c8
Merge remote-tracking branch 'upstream/main' into collection-io
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR116646) when ready.
Jesse Yurkovich added 4 commits 2024-03-28 00:20:20 +01:00
Brecht Van Lommel requested changes 2024-03-28 20:28:33 +01:00
Dismissed
@ -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)

Add `BKE_report(op->reports, RPT_ERROR, "File handler '%s' not found'", name)`
deadpin marked this conversation as resolved
@ -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)

Add `BKE_report(op->reports, RPT_ERROR, "File handler operator '%s' not found'", fh->export_operator)`
deadpin marked this conversation as resolved
@ -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 not export_properties?

`io_wmOpItemProp` is a bit of a strange name. Any reason it's not `export_properties`?
deadpin marked this conversation as resolved
@ -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.

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.
deadpin marked this conversation as resolved
@ -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.

This needs a poll function that checks `CTX_data_collection(C) != nullptr`.
deadpin marked this conversation as resolved
@ -417,0 +501,4 @@
/* identifiers */
ot->name = "Remove Exporter";
ot->description = "Remove Exporter";
ot->idname = "COLLECTION_OT_io_handler_remove";

COLLECTION_OT_exporter_remove

`COLLECTION_OT_exporter_remove`
deadpin marked this conversation as resolved
@ -417,0 +505,4 @@
/* api callbacks */
ot->exec = collection_io_handler_remove_exec;
ot->poll = WM_operator_winactive;

Same comment about poll function.

Same comment about poll function.
deadpin marked this conversation as resolved
@ -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

IO Handler index -> Exporter index
deadpin marked this conversation as resolved
@ -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.

`COLLECTION_OT_export` perhaps.
deadpin marked this conversation as resolved
@ -417,0 +572,4 @@
/* api callbacks */
ot->exec = collection_io_handler_export_exec;
ot->poll = WM_operator_winactive;

Same comment about poll.

Same comment about poll.
deadpin marked this conversation as resolved
@ -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

IO Handler index -> Exporter index
deadpin marked this conversation as resolved
@ -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

`COLLECTION_OT_export_all`
deadpin marked this conversation as resolved
@ -417,0 +609,4 @@
/* api callbacks */
ot->exec = collection_io_export_all_exec;
ot->poll = WM_operator_winactive;

Same comment about poll.

Same comment about poll.
deadpin marked this conversation as resolved
@ -417,0 +651,4 @@
/* api callbacks */
ot->exec = wm_collection_export_all_exec;
ot->poll = WM_operator_winactive;

Same comment about poll.

Same comment about poll.
deadpin marked this conversation as resolved
@ -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.

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.
deadpin marked this conversation as resolved
@ -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.

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`.
deadpin marked this conversation as resolved
@ -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.

Similarly, would call this just `exporters`.
deadpin marked this conversation as resolved

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.

> Exporters
|         Add ...             |  |      Export All      |
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. ``` > Exporters | Add ... | | Export All | ```

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

@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.
Jesse Yurkovich added 4 commits 2024-03-29 03:06:43 +01:00
Jesse Yurkovich changed title from WIP: Enable file exporters to be specified on Collections to Collection IO: Enable file exporters to be specified on Collections 2024-03-29 03:39:28 +01:00
Jesse Yurkovich added 1 commit 2024-03-29 08:21:13 +01:00
Jesse Yurkovich added 2 commits 2024-03-29 18:16:07 +01:00
Brecht Van Lommel approved these changes 2024-03-29 18:21:04 +01:00
Dismissed
Brecht Van Lommel dismissed brecht’s review 2024-03-29 18:22:04 +01:00
Reason:

Too early

Brecht Van Lommel requested review from Brecht Van Lommel 2024-03-29 18:22:13 +01:00

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

        row.enabled = any(len(coll.exporters) > 0 for coll in bpy.data.collections) 

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.

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). ``` row.enabled = any(len(coll.exporters) > 0 for coll in bpy.data.collections) ``` 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.
Member

One small thing. In the (unlikely) event that there are no file handlers available we get a popup notification:

image

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.

One small thing. In the (unlikely) event that there are no file handlers available we get a popup notification: ![image](/attachments/5e1bc3e9-4016-4484-8627-0ea82b3eb2ce) 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.
7.0 KiB
Member

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:

image

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: ![image](/attachments/d44fa93d-25dd-454f-8442-ef898ab5b8ca)
Member

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.

static int collection_exporter_remove_invoke(bContext *C,
                                             wmOperator *op,
                                             const wmEvent * /*event*/)
{
  return WM_operator_confirm_ex(
      C, op, IFACE_("Remove exporter?"), nullptr, IFACE_("Delete"), ALERT_ICON_NONE, false);
}

void COLLECTION_OT_exporter_remove(wmOperatorType *ot)
{
  /* identifiers */
  ot->name = "Remove Exporter";
  ot->description = "Remove Exporter";
  ot->idname = "COLLECTION_OT_exporter_remove";

  /* api callbacks */
  ot->invoke = collection_exporter_remove_invoke;
...
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. ``` static int collection_exporter_remove_invoke(bContext *C, wmOperator *op, const wmEvent * /*event*/) { return WM_operator_confirm_ex( C, op, IFACE_("Remove exporter?"), nullptr, IFACE_("Delete"), ALERT_ICON_NONE, false); } void COLLECTION_OT_exporter_remove(wmOperatorType *ot) { /* identifiers */ ot->name = "Remove Exporter"; ot->description = "Remove Exporter"; ot->idname = "COLLECTION_OT_exporter_remove"; /* api callbacks */ ot->invoke = collection_exporter_remove_invoke; ... ```
Author
Member

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.

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.

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.

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):
coll-export-4.png

I worry about the "Remove Exporter" button being so close to the "Export" button.

Ack. Will make your suggested change for an invoke confirmation.

> 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. 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. > 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. 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): ![coll-export-4.png](/attachments/4c483d65-1e18-4746-9c07-bfde673a750d) > I worry about the "Remove Exporter" button being so close to the "Export" button. Ack. Will make your suggested change for an invoke confirmation.
Member

Honestly not sure what to do with the "presets" button looking dark among its siblings.

image

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.

image

image

Honestly not sure what to do with the "presets" button looking dark among its siblings. ![image](/attachments/93654c57-b93f-435d-9ad8-d301624c8af1) 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. ![image](/attachments/2f43ba1e-7b5f-4f58-9828-50463a3ef63c) ![image](/attachments/e88cba3c-2f20-4f54-b6c6-19cb76d5b2fd)
5.5 KiB
5.5 KiB
5.5 KiB
Member

@deadpin - It would more typically look something like...

Oh, that does look nice!

> @deadpin - It would more typically look something like... Oh, that does look nice!
Member

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.

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.
Jesse Yurkovich added 3 commits 2024-04-02 21:24:01 +02:00
Author
Member

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

@mont29 Can you take a look at commit 25407e6bbd0695e2a1c65eff4be8f1028aa9633b 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.
Member

@deadpin - this text will only be visible to devs who build "lite"

Yikes, you're right. I was thinking these were enabled in Addons. So the way it is just fine. Sorry about that!

I'm surprised the browser is "helpfully" always putting a filename in place...

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.

> @deadpin - this text will only be visible to devs who build "lite" Yikes, you're right. I was thinking these were enabled in Addons. So the way it is just fine. Sorry about that! > I'm surprised the browser is "helpfully" always putting a filename in place... 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.
Bastien Montagne reviewed 2024-04-03 06:37:59 +02:00
Bastien Montagne left a comment
Owner

Mainly answering @deadpin 's question here, plus a few other related notes found in same code. By no mean a full review.

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

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

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.

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.
deadpin marked this conversation as resolved
@ -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.

Missing deg tagging of the modified collection (likely just using `ID_RECALC_SYNC_TO_EVAL`?). Same below in remove code.
deadpin marked this conversation as resolved
@ -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 (via BKE_view_layer_synced_ensure).

Also, this should be called before deg tagging and notifiers are sent.

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 (via `BKE_view_layer_synced_ensure`). Also, this should be called before deg tagging and notifiers are sent.
deadpin marked this conversation as resolved
@ -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?

Most likely have no consequences here, but would rather remove the link from the listbase before starting to free its data?
deadpin marked this conversation as resolved
Bastien Montagne requested changes 2024-04-03 06:38:45 +02:00
Dismissed
Bastien Montagne left a comment
Owner

Requesting changes

Requesting changes
Jesse Yurkovich added 2 commits 2024-04-04 03:35:09 +02:00
Member

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.

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.
Bastien Montagne approved these changes 2024-04-05 05:13:29 +02:00
Bastien Montagne left a comment
Owner

LGTM now, for what I checked.

Regarding extensions, I would not expect any expectation on length here? Even Blender files use .blend!

LGTM now, for what I checked. Regarding extensions, I would not expect any expectation on length here? Even Blender files use `.blend`!
Author
Member

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.

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

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

> @deadpin - 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.
Jesse Yurkovich added 1 commit 2024-04-08 20:31:51 +02:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
634ee5600b
Merge remote-tracking branch 'upstream/main' into collection-io
Author
Member

@blender-bot build

@blender-bot build
Jesse Yurkovich merged commit 509a7870c3 into main 2024-04-08 22:10:52 +02:00
Jesse Yurkovich deleted branch collection-io 2024-04-08 22:10:55 +02:00
Sign in to join this conversation.
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
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#116646
No description provided.