Page MenuHome

Build modifier: Adding "natural drawing" time & UI improvements.
ClosedPublic

Authored by Marc Chéhab (marcluzmedia) on Dec 13 2022, 12:06 PM.

Details

Summary

Hello dear GP team!

I'm Marc, I've contributed the "additive" build modifier in Blender 3.2. I've been working on an update which uses the recorded drawing speed to rebuild the strokes. This results in a way more natural feel of the animation. Here's a 1min video so you see the idea: https://youtu.be/PAAek5QjmHA

People told me I should approach the GP module because you're updating the underlying data architecture. I'll post a comment on the next module meeting.

Here's a short summary of existing data I use:

  • gps->points->time: This is a timestamp in seconds of when the point was created since the creation of the stroke. It's quite often 0 (I added a sanitization routine).
  • gpf->inittime: This is a timestamp in seconds when a stroke was drawn measured since some unknown point in time. I only ever use the difference between two strokes, so the absolute value is not relevant.

I added to DNA & RNA, as you see in the patch. I also wrote an update code in versioning_300.c.

I'd love to see this in master!

Kind regards,

Marc

Diff Detail

Repository
rB Blender

Event Timeline

Marc Chéhab (marcluzmedia) requested review of this revision.Dec 13 2022, 12:06 PM
Marc Chéhab (marcluzmedia) created this revision.

Made version code if condition slightly more accurate.

@Aleš Jelovčan (frogstomp) Could you do some tests? you know very well build modifier, so you know how to test it.

Greetings, I already built with it yesterday and will test more in detail.
For now I can consistently get a crash with a file not ment for that (the frames are not ment to be additive)

@Marc Chéhab (marcluzmedia) here is a test file, to get crash just let it run more than one full length of scene animation

@Marc Chéhab (marcluzmedia) here is a test file, to get crash just let it run more than one full length of scene animation

Thanks a lot for your testing efforts!! I found something that is definitively a bug in the sanitization routine for the zeropoints & made it very robust. Let's see if that is what caused your crash, too!

Great patch. In general the code looks good, just some style comments.

We need a full test to be sure the modifier is working as expected.

source/blender/gpencil_modifiers/intern/MOD_gpencilbuild.c
256

In the final patch you shouldn't include ToDos

268

Don't use C++ comments // ... use /* */ above the line.

I updated the following

  • I added a setting for users to define the time for geometric strokes in seconds. Default is 1 second. If all "time" values of a stroke are 0, it uses this setting.
  • I realised BLI_listbase_count(&gpf->prev->strokes) used cached strokes and gave unreliable stroke counts, changed it to BLI_listbase_count(&gpf->runtime.gpf_orig->prev->strokes)

tests look good, special cases are covered nicely

This revision is now accepted and ready to land.Dec 21 2022, 11:26 AM

I've hardcoded two values as discussed today on blender.chat, in particular I changed the time for geometric strokes from a UI exposed setting to a hardcoded value (because it's really a detail and the UI is cleaner this way).

Marc Chéhab (marcluzmedia) marked 2 inline comments as done.Dec 27 2022, 9:50 AM

@Matias Mendiola (mendio) Do you think the UI is correct? If you agree and @Aleš Jelovčan (frogstomp) has done a full range of testing, we can approve the patch.

@Marc Chéhab (marcluzmedia) Could you rebase the patch with master? We are trying to apply the patch and fails, so we cannot test it.

Dear @Antonio Vazquez (antoniov) and @Matias Mendiola (mendio) ! Thanks, yes, sure, I pulled "master" again, here's a fresh diff file.

@Marc Chéhab (marcluzmedia) great improvement for the Build mofdifier, nice work.

My notes:

IMO "Use natural writing speed" should be an option to be used in any of the Build Modifier Modes (Sequential, concurrent, additive) not only in Additive Mode.
The name could be just "Natural drawing" (avoiding the writing reference) and It should be behave like Fade subpanel with a checkbox to activate it and the settings

Here is a little mockup

For consistency we can get rid of "Time Mode" selector in Additive Mode, Please use what we have in the other modes for the same purpose (Start Delay - Frames / Manual factor)

Dear Mathias! Thanks for your review!

IMO "Use natural writing speed" should be an option to be used in any of the Build Modifier Modes (Sequential, concurrent, additive) not only in Additive Mode.

That would require a substantial rewrite of the underlying code. I decided against it because I didn't see the value of having a "natural drawing speed" animation for fundamentally "unnatural" build modes, especially with concurrent. If multiple lines are being drawn simultaneously, do you even notice their speed? Or in sequential if you "shrink" it the same. Do you see relevant use-cases that justify the effort?

The name could be just "Natural drawing" (avoiding the writing reference) and It should be behave like Fade subpanel with a checkbox to activate it and the settings

Good point, I'll rename it.

Here is a little mockup
For consistency we can get rid of "Time Mode" selector in Additive Mode, Please use what we have in the other modes for the same purpose (Start Delay - Frames / Manual factor)

The reason I added the selector is because in the "natural drawing speed" mode, all the inputs at the top would be deactivated (delay, frames, manual, object). That would make for a cluttered UI, don't you think?

I chose the selector because that way the UI separates what is a logical separation: Those are fundamentally different time modes and if you choose one, you shouldn't be distracted by the inactive options of the other. What do you think?

Matias Mendiola (mendio) requested changes to this revision.Jan 5 2023, 2:18 PM

That would require a substantial rewrite of the underlying code. I decided against it because I didn't see the value of having a "natural drawing speed" animation for fundamentally "unnatural" build modes, especially with concurrent. If multiple lines are being drawn simultaneously, do you even notice their speed? Or in sequential if you "shrink" it the same. Do you see relevant use-cases that justify the effort?

I know this require a more code work, but I really think is the way to go. You are right that concurrent will not benefit for this feature. But in Other modes have the same speed you used to draw the strokes is really nice to get rid of the unnatural and mechanical way the Build modifier is working now. Having both way for all modes, natural and mechanical, will be a really nice addition.

The reason I added the selector is because in the "natural drawing speed" mode, all the inputs at the top would be deactivated (delay, frames, manual, object). That would make for a cluttered UI, don't you think?
I chose the selector because that way the UI separates what is a logical separation: Those are fundamentally different time modes and if you choose one, you shouldn't be distracted by the inactive options of the other. What do you think?

This UI change is related with moving "Natural Drawing" for all modes.
That way we only keep Frames and Manual Factor in Additive Mode and we should follow the same UI and behaviour used in other modes.
In a different patch we can tweak the UI to improve this for all modes, but for now keeping the consistency is a must.

Other thing I forgot to mention: Why the object target origin is now longer available in this patch for Additive mode? Working well in the current version allowing the user to set the origin of the strokes building other than the original.

This revision now requires changes to proceed.Jan 5 2023, 2:18 PM

Dear @Matias Mendiola (mendio). It adapted the underlying code so it can handle going backwards on strokes.

Before I submit yet another patch, I want to ask about the UI: It'd be easier for me to adapt the UI now. Here's my proposal, which is based on the idea that we always show:

  • first the build option
  • second the time options

I think overall it makes for a less cluttered UI than if we show all time options all the time (frames, manual factor & drawing speed).

Here's a side-by-side comparison, at the bottom a video.

  • Sorry gnome screen recording hid my cursor

If I cannot get the new drawing speed to work with "concurrent" mode today, I'll have to hide the option from the dropdown in concurrent mode. (If I find out how to do that, hehe...)

About DELAY Option i don't see it in additive mode is it intentionally? because it's useful to make previous kf vanish and after a delay then the hand writing get working ,,so is it renamed by another thing ?

Thanks for the update, I'll try it ASAP, meanwhile please change the patch title and summary to reflect what you are really doing right now: Adding a new time option "Natural Drawing" to the different modes and UI improvements.

Marc Chéhab (marcluzmedia) retitled this revision from Update for "additive" build modifier to Build modifier: Adding "natural drawing" time & UI improvements..Jan 7 2023, 5:42 PM

Current version to look at the UI.

The patch is working really well

  1. Same name changes to simplify and make less redundant (please see the uppercase treatment):
    • Build Mode > Mode (We are already in the Build Modifier the word Build is redundant here)
    • Time Mode > Time Method (to avoid the reuse of word Mode)
    • Time Method options:
      • Natural Drawing Speed
      • Number of Frames
      • Percentage Factor
  1. The tooltip of Time selector should be generic to cover all cases. Right now is only related to Natural Drawing Speed.
  1. Now it is not possible to select Natural Drawing Speed in Concurrent Mode. You should hide the option if is not working with this time method or allow it to be used with this method.

Thanks a lot for your review & tests! I'll change the UI with your changes. Other than that, there's only the bug with "shrink" left, I think.

In my test Shrink is working well even with and object source target, could you upload a video about the bug you mention?

It looks a bit like a rounding error to me... The speed is erratic.

@Matias Mendiola (mendio) I fixed the bug, will update patch today

All that is left is hiding one option in the dropdown menu, but I can't get it to work. Would you mind having a look? My logic starts in rna_gpencil_modifier.c where I use RNA_def_property_enum_funcs to get to prop_gpencil_build_time_mode_filter .

All that is left is hiding one option in the dropdown menu, but I can't get it to work. Would you mind having a look? My logic starts in rna_gpencil_modifier.c where I use RNA_def_property_enum_funcs to get to prop_gpencil_build_time_mode_filter .

@Antonio Vazquez (antoniov) could you please help with this? How could be the best way to solve it?

Hans Goudey (HooglyBoogly) added inline comments.
source/blender/makesrna/intern/rna_gpencil_modifier.c
2565

Usually instead of defining the min and max at 0 and 100, we use 0 and 1. The UI should take care of drawing that as a percentage on its own, and there's even a preference for displaying percentages or factors. You might have to set PROP_PERCENTAGE for that to work, I forget.

@Marc Chéhab (marcluzmedia) To fix the problem with the list of options, apply this diff

diff --git a/source/blender/gpencil_modifiers/intern/MOD_gpencilbuild.c b/source/blender/gpencil_modifiers/intern/MOD_gpencilbuild.c
index 2303a76ec4c..a929b302de0 100644
--- a/source/blender/gpencil_modifiers/intern/MOD_gpencilbuild.c
+++ b/source/blender/gpencil_modifiers/intern/MOD_gpencilbuild.c
@@ -881,12 +881,11 @@ static void panel_draw(const bContext *UNUSED(C), Panel *panel)
   PointerRNA ob_ptr;
   PointerRNA *ptr = gpencil_modifier_panel_get_property_pointers(panel, &ob_ptr);
 
-  int mode = RNA_enum_get(ptr, "mode");
+  const int mode = RNA_enum_get(ptr, "mode");
   int time_mode = RNA_enum_get(ptr, "time_mode");
   const bool use_percentage = RNA_boolean_get(ptr, "use_percentage");
 
   uiLayoutSetPropSep(layout, true);
-  RNA_def_enum_funcs(RNA_enum_get(ptr, "time_mode"), prop_gpencil_build_time_mode(mode));
 
   /* First: Build mode and build settings. */
   uiItemR(layout, ptr, "mode", 0, NULL, ICON_NONE);
@@ -923,6 +922,8 @@ static void panel_draw(const bContext *UNUSED(C), Panel *panel)
     case GP_BUILD_TIMEMODE_PERCENTAGE:
       uiItemR(layout, ptr, "percentage_factor", 0, NULL, ICON_NONE);
       break;
+    default:
+      break;
   }
   uiItemS(layout);
   uiItemR(layout, ptr, "object", 0, NULL, ICON_NONE);
diff --git a/source/blender/makesrna/intern/rna_gpencil_modifier.c b/source/blender/makesrna/intern/rna_gpencil_modifier.c
index a0c9c656459..7f00fffeaef 100644
--- a/source/blender/makesrna/intern/rna_gpencil_modifier.c
+++ b/source/blender/makesrna/intern/rna_gpencil_modifier.c
@@ -158,6 +158,25 @@ const EnumPropertyItem rna_enum_object_greasepencil_modifier_type_items[] = {
     {0, NULL, 0, NULL, NULL},
 };
 
+static const EnumPropertyItem gpencil_build_time_mode_items[] = {
+    {GP_BUILD_TIMEMODE_DRAWSPEED,
+     "DRAWSPEED",
+     0,
+     "Natural Drawing Speed",
+     "Use recorded speed multiplied by a factor"},
+    {GP_BUILD_TIMEMODE_FRAMES,
+     "FRAMES",
+     0,
+     "Number of Frames",
+     "Set a fixed number of frames for all build animations"},
+    {GP_BUILD_TIMEMODE_PERCENTAGE,
+     "PERCENTAGE",
+     0,
+     "Percentage Factor",
+     "Set a manual percentage to build"},
+    {0, NULL, 0, NULL, NULL},
+};
+
 #ifndef RNA_RUNTIME
 static const EnumPropertyItem modifier_modify_color_items[] = {
     {GP_MODIFY_COLOR_BOTH, "BOTH", 0, "Stroke & Fill", "Modify fill and stroke colors"},
@@ -925,6 +944,33 @@ static void rna_EnvelopeGpencilModifier_material_set(PointerRNA *ptr,
   rna_GpencilModifier_material_set(ptr, value, ma_target, reports);
 }
 
+const EnumPropertyItem *gpencil_build_time_mode_filter(bContext *UNUSED(C),
+                                                       PointerRNA *ptr,
+                                                       PropertyRNA *prop,
+                                                       bool *r_free)
+{
+
+  GpencilModifierData *md = ptr->data;
+  BuildGpencilModifierData *mmd = (BuildGpencilModifierData *)md;
+  const bool is_concurrent = (mmd->mode == GP_BUILD_MODE_CONCURRENT);
+
+  EnumPropertyItem *item_list = NULL;
+  int totitem = 0;
+
+  for (const EnumPropertyItem *item = gpencil_build_time_mode_items; item->identifier != NULL;
+       item++) {
+    if (is_concurrent && item->identifier == "DRAWSPEED") {
+      continue;
+    }
+    RNA_enum_item_add(&item_list, &totitem, item);
+  }
+
+  RNA_enum_item_end(&item_list, &totitem);
+  *r_free = true;
+
+  return item_list;
+}
+
 #else
 
 static void rna_def_modifier_gpencilnoise(BlenderRNA *brna)
@@ -2395,32 +2441,6 @@ static void rna_def_modifier_gpencilarray(BlenderRNA *brna)
 
   RNA_define_lib_overridable(false);
 }
-const EnumPropertyItem *prop_gpencil_build_time_mode_filter(bContext *UNUSED(C),
-                                                            PointerRNA *ptr,
-                                                            PropertyRNA *prop,
-                                                            bool *r_free)
-{
-  GpencilModifierData *md = ptr->data;
-  BuildGpencilModifierData *mmd = (BuildGpencilModifierData *)md;
-
-  EnumPropertyItem *time_mode_all = (EnumPropertyRNA *)prop;
-  if (mmd->mode != GP_BUILD_MODE_CONCURRENT) {
-    return time_mode_all;
-  }
-
-  EnumPropertyItem *time_mode_filtered = NULL;
-  int items_len = 0;
-  for (const EnumPropertyItem *item = time_mode_all; item->identifier != NULL; item++) {
-    if (item->identifier != "DRAWSPEED") {
-      RNA_enum_item_add(&time_mode_filtered, &items_len, item);
-    }
-  }
-
-  RNA_enum_item_end(&time_mode_filtered, &items_len);
-  *r_free = true;
-
-  return time_mode_filtered;
-}
 
 static void rna_def_modifier_gpencilbuild(BlenderRNA *brna)
 {
@@ -2479,25 +2499,6 @@ static void rna_def_modifier_gpencilbuild(BlenderRNA *brna)
       {0, NULL, 0, NULL, NULL},
   };
 
-  static EnumPropertyItem prop_gpencil_build_time_mode[] = {
-      {GP_BUILD_TIMEMODE_DRAWSPEED,
-       "DRAWSPEED",
-       0,
-       "Natural Drawing Speed",
-       "Use recorded speed multiplied by a factor"},
-      {GP_BUILD_TIMEMODE_FRAMES,
-       "FRAMES",
-       0,
-       "Number of Frames",
-       "Set a fixed number of frames for all build animations"},
-      {GP_BUILD_TIMEMODE_PERCENTAGE,
-       "PERCENTAGE",
-       0,
-       "Percentage Factor",
-       "Set a manual percentage to build"},
-      {0, NULL, 0, NULL, NULL},
-  };
-
   StructRNA *srna;
   PropertyRNA *prop;
 
@@ -2550,8 +2551,8 @@ static void rna_def_modifier_gpencilbuild(BlenderRNA *brna)
   /* Which time mode to use: Current frames, manual percentage, or drawspeed.  */
   prop = RNA_def_property(srna, "time_mode", PROP_ENUM, PROP_NONE);
   RNA_def_property_enum_sdna(prop, NULL, "time_mode");
-  RNA_def_property_enum_items(prop, prop_gpencil_build_time_mode);
-  RNA_def_property_enum_funcs(prop, NULL, NULL, "prop_gpencil_build_time_mode_filter");
+  RNA_def_property_enum_items(prop, gpencil_build_time_mode_items);
+  RNA_def_property_enum_funcs(prop, NULL, NULL, "gpencil_build_time_mode_filter");
   RNA_def_property_ui_text(
       prop,
       "Timing",

The diff file:

source/blender/makesrna/intern/rna_gpencil_modifier.c
2565

I do need the upper limit to be substantially more than a factor of one. A factor of 100 should suffice, that's why I put it to 100. You're right that in many other places it's like this

RNA_def_property_range(prop, 0.0f, 1.0f);

I adapted the code to better reflect that formatting and put in 0.0f, 100.0f

Thanks for the input!

Thanks a lot @Antonio Vazquez (antoniov) for your time and effort, you saved my sanity today! I'll review your changes to learn from them tomorrow. With your addition and a small formatting change, I now updated the patch to a version which I think is ready for a final review!

Thanks to @Matias Mendiola (mendio), the function of this patch - to use a factor of the recorded drawing speed - is now available across all functionality in all modes except concurrent. With Antonio's help, Natural Drawing Speed does not appear in the dropdown.

And @Aleš Jelovčan (frogstomp) : Besides your tests, thanks for our chats and your pointers in the right direction to the right people!

If you're doing any tests, here is a test file I use - just press play, it's quite self explanatory.

Again thank you all for adding your effort to this new functionality and inviting me to the team meeting :) It's great to know you all!

(Obviously I'm still here, hehe, now it all weirdly took on a departing quality...)

source/blender/makesrna/intern/rna_gpencil_modifier.c
2565

Looking again, it's possible that my comment was completely wrong, I should have actually built this and looked at the UI. Seeing the 100 made me assume the patch was manually building the percentage.

Well done!
Tested, I think it is ready

Marc Chéhab (marcluzmedia) marked an inline comment as done.Jan 10 2023, 12:38 PM
Marc Chéhab (marcluzmedia) added inline comments.
source/blender/makesrna/intern/rna_gpencil_modifier.c
2565

Ah ok, thanks for clarifying!

This revision is now accepted and ready to land.Jan 10 2023, 1:18 PM
Marc Chéhab (marcluzmedia) marked an inline comment as done.

rebased patch