Apologies for delay, patch updated, review needed

@Aaron Carlisle (Blendify)
@Hans Goudey (HooglyBoogly)
@Julian Eisel (Severin)

Can we continue on this?, only final review is needed
@Aaron Carlisle (Blendify)
@Julian Eisel (Severin)

Final Review needed

This patch fixes the alignment issues in the Line set Panel, while it is a hack (empty panel) I think this is much better looking than using the custom UI spacing

In D10505#315071, @Hans Goudey (HooglyBoogly) wrote:

I still feel uncomfortable accepting this having never actually used Freestyle. The UI looks much better to me though, and by looking at the screenshots I only see these two small things:

Here you can omit "Value" from the second line. This is standard elsewhere this sort of aligned button is used.

This can look the same as above. Looks like maybe you forgot to do that when turning property split on.

agree will unify the style of these as you said

In D10505#315169, @Aaron Carlisle (Blendify) wrote:

This patch fixes the alignment issues in the Line set Panel, while it is a hack (empty panel) I think this is much better looking than using the custom UI spacing

P2314 (An Untitled Masterwork)

1	diff --git a/release/scripts/startup/bl_ui/properties_freestyle.py b/release/scripts/startup/bl_ui/properties_freestyle.py
2	index 9191c355c5e..6438acc0aea 100644
3	--- a/release/scripts/startup/bl_ui/properties_freestyle.py
4	+++ b/release/scripts/startup/bl_ui/properties_freestyle.py
5	@@ -301,10 +301,26 @@ class VIEWLAYER_PT_freestyle_lineset(ViewLayerFreestyleEditorButtonsPanel, Panel
6	layout.template_ID(lineset, "linestyle",
7	new="scene.freestyle_linestyle_new")
8
9	- layout.label(text="Selection By")
10	- row = layout.row(align=True)
11	- row.separator(factor=4.0)
12	- row.prop(lineset, "select_by_image_border", text="Image Border")
13	+ layout.separator()
14	+ layout.label(text="Select by:")
15	+
16	+
17	+class VIEWLAYER_PT_freestyle_lineset_image_border(ViewLayerFreestyleLineStyle, Panel):
18	+ bl_label = "Image Border"
19	+ bl_parent_id = "VIEWLAYER_PT_freestyle_lineset"
20	+ COMPAT_ENGINES = {'BLENDER_RENDER', 'BLENDER_EEVEE', 'BLENDER_WORKBENCH'}
21	+
22	+ def draw_header(self, context):
23	+ layout = self.layout
24	+
25	+ view_layer = context.view_layer
26	+ freestyle = view_layer.freestyle_settings
27	+ lineset = freestyle.linesets.active
28	+
29	+ layout.prop(lineset, "select_by_image_border", text="")
30	+
31	+ def draw(_self, _context):
32	+ pass
33
34
35	# Freestyle Lineset Sub Panels
36	@@ -1308,6 +1324,7 @@ classes = (
37	VIEWLAYER_PT_freestyle_edge_detection_options,
38	VIEWLAYER_PT_freestyle_style_modules,
39	VIEWLAYER_PT_freestyle_lineset,
40	+ VIEWLAYER_PT_freestyle_lineset_image_border,
41	VIEWLAYER_PT_freestyle_lineset_visibilty,
42	VIEWLAYER_PT_freestyle_lineset_edgetype,
43	VIEWLAYER_PT_freestyle_lineset_facemarks,

great!, i applied the patch, but yeah still the collapse button is a problem, should I add a new panel option "HIDE_COLLAPSE"?

In D10505#315170, @Aaron Carlisle (Blendify) wrote:

I think this is very close and will land in 3.0 a couple of comments, might take a deeper look in the morning.

Awesome!.

I see, did the change will upload soon

In D10505#315447, @Iyad Ahmed (iyadahmed2001) wrote:

In D10505#315169, @Aaron Carlisle (Blendify) wrote:

This patch fixes the alignment issues in the Line set Panel, while it is a hack (empty panel) I think this is much better looking than using the custom UI spacing

P2314 (An Untitled Masterwork)

1	diff --git a/release/scripts/startup/bl_ui/properties_freestyle.py b/release/scripts/startup/bl_ui/properties_freestyle.py
2	index 9191c355c5e..6438acc0aea 100644
3	--- a/release/scripts/startup/bl_ui/properties_freestyle.py
4	+++ b/release/scripts/startup/bl_ui/properties_freestyle.py
5	@@ -301,10 +301,26 @@ class VIEWLAYER_PT_freestyle_lineset(ViewLayerFreestyleEditorButtonsPanel, Panel
6	layout.template_ID(lineset, "linestyle",
7	new="scene.freestyle_linestyle_new")
8
9	- layout.label(text="Selection By")
10	- row = layout.row(align=True)
11	- row.separator(factor=4.0)
12	- row.prop(lineset, "select_by_image_border", text="Image Border")
13	+ layout.separator()
14	+ layout.label(text="Select by:")
15	+
16	+
17	+class VIEWLAYER_PT_freestyle_lineset_image_border(ViewLayerFreestyleLineStyle, Panel):
18	+ bl_label = "Image Border"
19	+ bl_parent_id = "VIEWLAYER_PT_freestyle_lineset"
20	+ COMPAT_ENGINES = {'BLENDER_RENDER', 'BLENDER_EEVEE', 'BLENDER_WORKBENCH'}
21	+
22	+ def draw_header(self, context):
23	+ layout = self.layout
24	+
25	+ view_layer = context.view_layer
26	+ freestyle = view_layer.freestyle_settings
27	+ lineset = freestyle.linesets.active
28	+
29	+ layout.prop(lineset, "select_by_image_border", text="")
30	+
31	+ def draw(_self, _context):
32	+ pass
33
34
35	# Freestyle Lineset Sub Panels
36	@@ -1308,6 +1324,7 @@ classes = (
37	VIEWLAYER_PT_freestyle_edge_detection_options,
38	VIEWLAYER_PT_freestyle_style_modules,
39	VIEWLAYER_PT_freestyle_lineset,
40	+ VIEWLAYER_PT_freestyle_lineset_image_border,
41	VIEWLAYER_PT_freestyle_lineset_visibilty,
42	VIEWLAYER_PT_freestyle_lineset_edgetype,
43	VIEWLAYER_PT_freestyle_lineset_facemarks,

great!, i applied the patch, but yeah still the collapse button is a problem, should I add a new panel option "HIDE_COLLAPSE"?

great!, i applied the patch, but yeah still the collapse button is a problem, should I add a new panel option "HIDE_COLLAPSE"?

Yes, I was unaware of this option.

One other thing that I noticed:

In D10505#315472, @Aaron Carlisle (Blendify) wrote:

great!, i applied the patch, but yeah still the collapse button is a problem, should I add a new panel option "HIDE_COLLAPSE"?

Yes, I was unaware of this option.

One other thing that I noticed:

ah sorry the option doesn't exist yet I coded it in.

ok will remove them now

Ah okay, can you submit that as a separate patch to not clutter up this patch.

In D10505#315491, @Aaron Carlisle (Blendify) wrote:

Ah okay, can you submit that as a separate patch to not clutter up this patch.

Okay 👌

In D10505#315491, @Aaron Carlisle (Blendify) wrote:

Ah okay, can you submit that as a separate patch to not clutter up this patch.

Done ✅, https://developer.blender.org/D12185, should be approved I think before we can use option here

I think it is better,

@Pablo Vazquez (pablovazquez) what do you say about the Select by options (see picture just above)? In D12185: Add option to hide collapse button for panels you suggested a different solution.
I don't see a great solution to this, so I think we'll have to accept some trade-off.

Also, should Chain Count be called Chain Segment Count?

Besides that I don't see any bigger issue.

In D10505#320409, @Julian Eisel (Severin) wrote:

@Pablo Vazquez (pablovazquez) what do you say about the Select by options (see picture just above)? In D12185: Add option to hide collapse button for panels you suggested a different solution.
I don't see a great solution to this, so I think we'll have to accept some trade-off.

Also, should Chain Count be called Chain Segment Count?

Besides that I don't see any bigger issue.

I think yeah it is a tradeoff and the solution Pablo suggested is better imo because it is simple and doesn’t add special code

On chain count I asked Light from BNPR:
“ Chain segment count is wrong actually, feels like cutting lines into segments
Chain count is correct”

Chain count was the original name too so I guess it can stay

In D10505#320734, @Iyad Ahmed (iyadahmed2001) wrote:

In D10505#320409, @Julian Eisel (Severin) wrote:

@Pablo Vazquez (pablovazquez) what do you say about the Select by options (see picture just above)? In D12185: Add option to hide collapse button for panels you suggested a different solution.
I don't see a great solution to this, so I think we'll have to accept some trade-off.

Also, should Chain Count be called Chain Segment Count?

Besides that I don't see any bigger issue.

I think yeah it is a tradeoff and the solution Pablo suggested is better imo because it is simple and doesn’t add special code

Chain count was the original name so I guess it can stay

Yes, Chain Count is the precise term here.

In D10505#320734, @Iyad Ahmed (iyadahmed2001) wrote:

I think yeah it is a tradeoff and the solution Pablo suggested is better imo because it is simple and doesn’t add special code

I'm not sure if what this patch does is what Pablo suggested. E.g. instead of having the checkboxes in the panel headers you could have a list of checkboxes and the panels separately (e.g. grayed out if the relevant checkbox is disabled). I'll let @Pablo Vazquez (pablovazquez) decide, I don't have a strong opinion.

In D10505#321580, @Julian Eisel (Severin) wrote:

In D10505#320734, @Iyad Ahmed (iyadahmed2001) wrote:

I think yeah it is a tradeoff and the solution Pablo suggested is better imo because it is simple and doesn’t add special code

I'm not sure if what this patch does is what Pablo suggested. E.g. instead of having the checkboxes in the panel headers you could have a list of checkboxes and the panels separately (e.g. grayed out if the relevant checkbox is disabled). I'll let @Pablo Vazquez (pablovazquez) decide, I don't have a strong opinion.

Hmm not sure if separating panels from checkboxes is used heavily in Blender, it is unlikely Pablo meant that

@Julian Eisel (Severin), quote from @Pablo Vazquez (pablovazquez) DM
"Hey! this is looking much better now! (the Select By)
the only thing left in this panel I would say is to unify Visibility with it's range. No need to have 1 subpanel with only 1 setting. Or will it ever show more settings in that subpanel?"

"where it says Bounds it'd say Visibility, and where it says Box it'd say Visible (from the dropdown)"

I tired so but it creates more problems

screenshot from local work not in patch

important thing is select by as is now is good

@Julian Eisel (Severin) can we get this approved?, Pablo sees no issue in the checkbox layout

In D10505#322942, @Julian Eisel (Severin) wrote:

I talked with Pablo, we agree that this is ready now. Just requesting one little code cleanup and one error fix.

The responsive is_narrow behavior is not an existing pattern. But it's not intrusive and behaves quite nice IMO, so I'm fine with it. It would be easy to support this on the C level I think and have it as a general feature that way.

very great, will work on it right away

In D10505#322942, @Julian Eisel (Severin) wrote:

I talked with Pablo, we agree that this is ready now. Just requesting one little code cleanup and one error fix.

The responsive is_narrow behavior is not an existing pattern. But it's not intrusive and behaves quite nice IMO, so I'm fine with it. It would be easy to support this on the C level I think and have it as a general feature that way.

scale_x and y gives 0 all time

@Julian Eisel (Severin) changes done

In D10505#323798, @Julian Eisel (Severin) wrote:

This new responsive layout logic kicks in much earlier than the previous version. In fact, for me the enums spawn multiple rows now by default for me. I much preferred the previous one, this new one is too aggressive in my opinion.
But I also don't see a decent way to handle this from Python that works with zooming.

To move forward I'd say lets see if we want this as a general layout feature, so I created D12440: UI: Spread expanded enums over rows if space is narrow. And then we either do it properly everywhere, or not at all.
Meanwhile, you can remove the responsive layout for this patch, so we can accept it for merging.

Ok will do so