Page MenuHome

Only brushes with pinned materials have materials.
AbandonedPublic

Authored by Antonio Vazquez (antoniov) on Mar 11 2019, 7:50 PM.

Details

Summary

Instead of repeatedly assigning one gpencil material to all brushes, every brush will use the active material unless a material was previously pinned to a brush.

This avoids T62465.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
matc (matc) edited the summary of this revision. (Show Details)

Using GP_BRUSH_MATERIAL_PINNED to switch between active material and brush material, instead of updating all brushes on active material changes. This will allow brushes to have no material and therefore to not inflate the user count.

The material property in the topbar now switches between active material and brush material depending on whether or not the pin is activated.

The brush material is not yet updated when the pin state changes.

Removed an accidental printf statement.

matc (matc) retitled this revision from Adjusting the user count for materials assigned to gpencil brushes. to Only brushes with pinned materials have materials..
matc (matc) edited the summary of this revision. (Show Details)
matc (matc) added a reviewer: Grease Pencil.
matc (matc) set the repository for this revision to rB Blender.

Using the active material to pin to a brush.

Keeping the different material properties in sync (topbar, properties->active tool, properties->material).

Switching in properties->active tool between active material and brush material.

Notifying the material slots to update when a material had to be added before it was used.

I have tested and looks that all is working, but there are something I don't like. When you pin a material, the topbar changes and the look of the material is different. Also we have buttons for fake user, delete, etc and this is not a good solution for topbar.

We need keep the same UI design when the material is pinned. I did a change to add the option "hide_buttons" in template_ID_preview.

Maybe, we need something similar for template_ID (we would need this change in a separated patch and the review of @Brecht Van Lommel (brecht) and/or @Bastien Montagne (mont29) ) or maybe we need keep the template_list to keep the same look in the topbar.

EDIT: Also the design of the list changes

Antonio Vazquez (antoniov) requested changes to this revision.Mar 14 2019, 10:29 AM
This revision now requires changes to proceed.Mar 14 2019, 10:29 AM

I see another possible problem. If you remove all materials and try to draw, the cursor moves, but nothing happens. Also, the topbar material is empty.

One of the goals of grease pencil is to make the drawing simple, so if no material assigned, a new basic material must be created automatically.

I think lib_link_brush needs to leave the material NULL if it's not pinned, so existing files do not have materials assigned without pinning.

The reason behind the UI changes is unclear to me too.

source/blender/editors/gpencil/gpencil_paint.c
1237–1242

Maybe this logic can go entirely into BKE_gpencil_get_material_index(), since BKE_gpencil_use_material() does the same.

Renamed BKE_gpencil_use_material to BKE_gpencil_handle_brush_material.

Made BKE_gpencil_get_material_index return a zero based index and adapted other code to use the handle functions instead.

Added BKE_gpencil_get_material_for_brush and BKE_gpencil_get_material_index_for_brush to have easy access to the actual material.

I did not yet touch the UI again. I will have a look at it tomorrow and will try to make it always possible to draw.

matc (matc) edited the summary of this revision. (Show Details)
matc (matc) set the repository for this revision to rB Blender.

Moved active_material popover to the right side of the topbar.

Using enabled = False for the material property on brushes that have no
pinned material. It will show the active material while not pinned.

Fixed a really small (unrelated) problem in GPENCIL_UL_layer

Added BKE_gpencil_current_input_material (with variations for toolsettings and brush) to obtain a material under all cirumstances.

Yesterday I forgot to mention that lib_link_brush now drops materials for brushes without pinned flag. And unpins brushes without materials.

When the material properties panel is open, brush materials appear to be created really slowly. This leads to a long delay when starting to draw without any material selected. As a result the first stroke begins with a straight line.

I accidentally removed all reviewers yesterday and therefore am adding all back. Sorry for that.

When the material properties panel is open, brush materials appear to be created really slowly. This leads to a long delay when starting to draw without any material selected. As a result the first stroke begins with a straight line.

This is the result of the preview process and we need it to see how it's the material.

@matc (matc) Could you update the patch with the last master source code? I can't apply it to test because something has changed since you generated the patch file.

Updated diff to work with current master.

Fixed user count for automatically created and assigned materials.

Antonio Vazquez (antoniov) requested changes to this revision.Mar 18 2019, 7:06 PM

I have found several compiler issues with VS2017.

source/blender/blenkernel/intern/gpencil.c
1083

Warning C4047 'return': 'int *' differs in levels of indirection from 'int' bf_blenkernel D:\MyBlender\BlenderDEV\blender\source\blender\blenkernel\intern\gpencil.c 1083

1086

Warning C4047 'return': 'int *' differs in levels of indirection from 'int' bf_blenkernel D:\MyBlender\BlenderDEV\blender\source\blender\blenkernel\intern\gpencil.c 1086

1093

Warning C4101 'ma': unreferenced local variable bf_blenkernel D:\MyBlender\BlenderDEV\blender\source\blender\blenkernel\intern\gpencil.c 1093

source/blender/editors/gpencil/gpencil_data.c
2083

Warning C4101 'idx': unreferenced local variable bf_editor_gpencil

source/blender/editors/gpencil/gpencil_edit.c
3957

Compiler issue:
Warning C4024 'BKE_gpencil_handle_material': different types for formal and actual parameter 2 bf_editor_gpencil D:\MyBlender\BlenderDEV\blender\source\blender\editors\gpencil\gpencil_edit.c 3957

3958

Warning C4189 'totadd': local variable is initialized but not referenced bf_editor_gpencil D:\MyBlender\BlenderDEV\blender\source\blender\editors\gpencil\gpencil_edit.c 3910
Warning C4189 'totslots': local variable is initialized but not referenced bf_editor_gpencil D:\MyBlender\BlenderDEV\blender\source\blender\editors\gpencil\gpencil_edit.c 3909

This revision now requires changes to proceed.Mar 18 2019, 7:06 PM

fixed warnings and errors

fixed user count for automatically created materials on user input

I don't see why we need to have two times the material in the topbar.

It's looks very weird.

We need fix the bugs, but need keep the UI as it was.

@Matias Mendiola (mendio) What do you think?

One material is a property of the brush, the other is a property of the object.

@matc (matc) Yes, but we try to design a interface simple for new 2D animators. For long time Blender users the concept of Brush material and Object material can be easy to understand, but this is the type of thing we must avoid to get a "new user" friendly UI.

I agree with @Antonio Vazquez (antoniov). The top bar is for tool settings related to the tool you are using. As such, just show the material for the active brush / tool.

I agree with @Antonio Vazquez (antoniov). The top bar is for tool settings related to the tool you are using. As such, just show the material for the active brush / tool.

Exactly @William Reynish (billreynish) , material is related to the actual tool, and as @Antonio Vazquez (antoniov) said, we should keep the UI simple as possible.

@William Reynish (billreynish) @Matias Mendiola (mendio) With this patch the material for a brush is NULL by default and the active material of a object is used instead, unless a brush has a pinned material. The material property on the right side changes the active material for a object, the same way as the layer property changes the current layer. Active layer and active materials affect all brushes.

Before this patch changing the material for one brush would cause other burshes to update their material and change the objects active material. Changing the active material for a object would cause all brushes to update their materials as well. Brushes had essentially one button to change their own material as well as those of all other brushes.

@Antonio Vazquez (antoniov) If we want to have only one material selector in the topbar, i would suggest to not display the material for brushes, unless a material was pinned from the properties panel or the brush popover.

yes @matc (matc) , I know the goal of this the patch, and agree, but we can still keep the actual UI.

The material selector in the top-bar would change the object active material and if it pinned, the brush material should be used instead (like an override).

@Matias Mendiola (mendio) If we fake it to look the same in both cases, we would have to remove the slot related controls (move up, down, lock). The list of materials in each case would be of different length. (You can pin any existing grease pencil material, but you can only select between slots if unpinned.)

Do we have another button somewhere that operates on one property of two different types? Or would this be a first?

Moved to minimal changes in UI.

Topbar

Toolbar
I had to disable the property when not pinned. It would otherwise allow to change the material for the active slot.

This should be as requested by @Antonio Vazquez (antoniov) @Matias Mendiola (mendio) and @William Reynish (billreynish) for the topbar. I'm not sure about the toolbar.

I have tested and it looks this last version could work. We need the opinion of more reviewers.

I tested this latest patch. For me the UI is ok, but I wasn't able to change the material on the active brush when Material Pin is enabled.

I seem to have to un-pin, then change the material, then pin again. Can people confirm this?

@William Reynish (billreynish) I can confirm that changing a pinned material in the topbar is not possible. We have the same problem currently on master. I tried to implement this before, but failed to get a consistent look for both cases. @Antonio Vazquez (antoniov) previously posted pictures of how it did look like.

...

...


@matc (matc) Do you know if it's possible when change the material in topbar for pinned brush (current), unpin internally, change material and pin again? This would be only for the active brush if it's pinned. It's only an idea, maybe it's impossible.

@Antonio Vazquez (antoniov) I don't think it's possible to distinguish between topbar and properties panel. I can only think of rna_MaterialIndex_update to do this.

Antonio Vazquez (antoniov) requested changes to this revision.Mar 21 2019, 4:58 PM

@matc (matc) review the bug I put above.

release/scripts/startup/bl_ui/space_view3d_toolbar.py
1559

In properties panel, the template_ID is not working because context.object is None. If you unpin, you will see the topbar brush panel is working, but the properties desn't.

If you change to context.active_object works. I guess the context of properties panel is not handling the object,

This revision now requires changes to proceed.Mar 21 2019, 4:58 PM

I have tested to disable the topbar list when the brush is pinned,. It's weird to clik on the list and don't change anything.

This is the patch:

diff --git a/release/scripts/startup/bl_ui/properties_grease_pencil_common.py b/release/scripts/startup/bl_ui/properties_grease_pencil_common.py
index 5f2ac7e7123..bb059d6befc 100644
--- a/release/scripts/startup/bl_ui/properties_grease_pencil_common.py
+++ b/release/scripts/startup/bl_ui/properties_grease_pencil_common.py
@@ -832,6 +832,11 @@ class GreasePencilMaterialsPanel:
         layout = self.layout
         show_full_ui = (self.bl_space_type == 'PROPERTIES')
 
+        is_view3d = (self.bl_space_type == 'VIEW_3D')
+        tool_settings = context.scene.tool_settings
+        gpencil_paint = tool_settings.gpencil_paint
+        brush = gpencil_paint.brush
+
         ob = context.object
         row = layout.row()
 
@@ -841,6 +846,12 @@ class GreasePencilMaterialsPanel:
 
             row.template_list("GPENCIL_UL_matslots", "", ob, "material_slots", ob, "active_material_index", rows=rows)
 
+            # if topbar popover and brush pinned, disable
+            if is_view3d and brush is not None:
+                gp_settings = brush.gpencil_settings
+                if gp_settings.use_material_pin:
+                    row.enabled = False
+
             col = row.column(align=True)
             if show_full_ui:
                 col.operator("object.material_slot_add", icon='ADD', text="")
This revision now requires review to proceed.Mar 21 2019, 5:14 PM
  • Fixed error in properties panel
  • Disable Materiasl popover when brush is pinned

@matc (matc) I updated the diff, command again the patch to update the diff (if you need to do it).

@Antonio Vazquez (antoniov) The only thing remaining is the slow creation of new materials. But I guess this will be a new task. Looks otherwise good to be me.

matc (matc) added inline comments.Mar 21 2019, 11:23 PM
release/scripts/startup/bl_ui/space_view3d_toolbar.py
1682

Is this intentionally twice?

  • Remove duplicated line
Antonio Vazquez (antoniov) marked an inline comment as done.Mar 22 2019, 8:55 AM

The creation of preview materials is something not related to this. To generate the preview, a render task is started on background, so to solve this we would need to start this background tasks before the user open the control, but this change affects other areas of Blender.

For me, the patch is ready, but we will wait for more opinions, before applying it.

@Matias Mendiola (mendio) @William Reynish (billreynish) @Daniel Martinez Lara (pepeland) If you hve time, please check the last version of the patch. If all agree, we could move to master these changes next week.

I think this is ok. It's a shame you cannot change materials if it is pinned to the brush, but I suppose that can be handled later.

This revision is now accepted and ready to land.Mar 22 2019, 9:38 PM

@matc (matc) Do you have commit rights? if not, you can ask for them or give me your mail to set the authorship of the commit to you.

@matc (matc) does not have commit rights.

To set the author of the commit we don't need an email address, but we do need a real name, and then you can set `"Firstname Lastname <matc>".

@matc (matc) Can you give me your name to push the commit?

@Antonio Vazquez (antoniov) @Brecht Van Lommel (brecht) Would it be ok to do it like for 2cc303700b65?

I don't like the idea of being googlable by real name. Making one up does not seem right either. I would otherwise have no problem with @Antonio Vazquez (antoniov) claiming authorship.

No, I'd rather not commit without a real name for the author.

What Antonion can do is commit with himself as the author, and then in the commit message say "Patch contributed by matc".

Ok, I will commit myself and I will add a reference to @matc (matc)

This was included in commit 7021bd527380