blender/blender
154
493
Fork 732
Code Issues 6.7k Pull Requests 896 Projects 18 Wiki Activity

Double click list renaming not working on heavy scenes #40009

New Issue
Closed
opened 2014-05-03 13:55:03 +02:00 by Konstantins Visnevskis · 36 comments
Konstantins Visnevskis commented 2014-05-03 13:55:03 +02:00
Copy Link

System Information
Operating system: Vista64
Graphics card: gtx470

Blender Version
Broken: 2.7a

Short description of error
When the scene is heavy, so that interface becomes slightly laggy, double click list rename stops working, especially for fast clicking.

Exact steps for others to reproduce the error

  • Add subdivision modifiers to any mesh until interface becomes laggy.
  • Add a vertex group or UV map.
  • Double click to rename - renaming is not activated.
**System Information** Operating system: Vista64 Graphics card: gtx470 **Blender Version** Broken: 2.7a **Short description of error** When the scene is heavy, so that interface becomes slightly laggy, double click list rename stops working, especially for fast clicking. **Exact steps for others to reproduce the error** - Add subdivision modifiers to any mesh until interface becomes laggy. - Add a vertex group or UV map. - Double click to rename - renaming is not activated.
Konstantins Visnevskis commented 2014-05-03 13:55:03 +02:00
Author
Copy Link

Changed status to: 'Open'

Changed status to: 'Open'
Konstantins Visnevskis commented 2014-05-03 13:55:03 +02:00
Author
Copy Link

Added subscriber: @KonstantinsVisnevskis

Added subscriber: @KonstantinsVisnevskis
blender-admin commented 2014-05-03 13:55:03 +02:00
Copy Link

#91921 was marked as duplicate of this issue

#91921 was marked as duplicate of this issue
blender-admin commented 2014-05-03 13:55:03 +02:00
Copy Link

#88973 was marked as duplicate of this issue

#88973 was marked as duplicate of this issue
blender-admin commented 2014-05-03 13:55:03 +02:00
Copy Link

#63650 was marked as duplicate of this issue

#63650 was marked as duplicate of this issue
blender-admin commented 2014-05-03 13:55:03 +02:00
Copy Link

#88380 was marked as duplicate of this issue

#88380 was marked as duplicate of this issue
Bastien Montagne commented 2014-05-03 15:03:13 +02:00
Owner
Copy Link

Added subscriber: @mont29

Added subscriber: @mont29
Bastien Montagne commented 2014-05-03 15:05:19 +02:00
Owner
Copy Link

Added subscribers: @ideasman42, @brecht

Added subscribers: @ideasman42, @brecht
Bastien Montagne commented 2014-05-03 15:05:19 +02:00
Owner
Copy Link

@KonstantinsVisnevskis: you can ctrl-click (or ctrl-enter), works fine.

Dev note: looks like in this case, a 'single click' event is processed before a double-one gets generated, which turns to selecting the listrow instead of editing the label (i.e. ui_do_but_LISTROW() never gets the KM_DBL_CLICK event, unless you triple-click, which will then select the item and then edit its label).

Not quite sure we consider this a bug, or a todo of our event system?

@KonstantinsVisnevskis: you can ctrl-click (or ctrl-enter), works fine. Dev note: looks like in this case, a 'single click' event is processed before a double-one gets generated, which turns to selecting the listrow instead of editing the label (i.e. `ui_do_but_LISTROW()` never gets the `KM_DBL_CLICK` event, unless you triple-click, which will then select the item and then edit its label). Not quite sure we consider this a bug, or a todo of our event system?
Sergey Sharybin commented 2014-05-16 12:06:12 +02:00
Owner
Copy Link

Added subscriber: @Sergey

Added subscriber: @Sergey
Sergey Sharybin commented 2014-05-16 12:06:12 +02:00
Owner
Copy Link

@mont29, ideally none of the RNA update should do heavy operations (look into shape key update function, i.e.). But that's also kind of separate issue because you'll need to tag update here anyway which would make first click tagging the object, then object update happens and only them second click is handled which doesn't really help.

What we can do instead is to not activate row if we're single-clicking an active one. Here's a real dirty hack for this http://www.pasteall.org/51656/diff Maybe you'll have ideas how to clean it up a bit.

@mont29, ideally none of the RNA update should do heavy operations (look into shape key update function, i.e.). But that's also kind of separate issue because you'll need to tag update here anyway which would make first click tagging the object, then object update happens and only them second click is handled which doesn't really help. What we can do instead is to not activate row if we're single-clicking an active one. Here's a real dirty hack for this http://www.pasteall.org/51656/diff Maybe you'll have ideas how to clean it up a bit.
Bastien Montagne was assigned by Sergey Sharybin 2014-05-19 18:11:24 +02:00
Bastien Montagne commented 2014-05-20 19:13:08 +02:00
Owner
Copy Link

Changed status from 'Open' to: 'Archived'

Changed status from 'Open' to: 'Archived'
Bastien Montagne commented 2014-05-20 19:13:08 +02:00
Owner
Copy Link

This is in fact an “issue” with current event system, you just can’t avoid getting a click first before getting a doubleclick event… More a TODO/internal limitation than a bug, so closing as such (see also our TODO list).

This is in fact an “issue” with current event system, you just can’t avoid getting a click first before getting a doubleclick event… More a TODO/internal limitation than a bug, so closing as such (see also [our TODO list](http://wiki.blender.org/index.php/Dev:2.5/Source/Development/Todo/UserInterface#Events)).
Philipp Oeser commented 2021-05-19 13:00:18 +02:00
Member
Copy Link

Added subscriber: @lichtwerk

Added subscriber: @lichtwerk
Philipp Oeser commented 2021-05-19 13:00:18 +02:00
Member
Copy Link

Changed status from 'Archived' to: 'Confirmed'

Changed status from 'Archived' to: 'Confirmed'
Philipp Oeser commented 2021-05-19 13:00:18 +02:00
Member
Copy Link

Since the TODO list is dead, the issue is still there (has been reported again), will set to Known Issue to have something as reference.

Since the TODO list is dead, the issue is still there (has been reported again), will set to `Known Issue` to have something as reference.
Philipp Oeser commented 2021-05-19 13:00:51 +02:00
Member
Copy Link

Added subscriber: @PMA33

Added subscriber: @PMA33
Philipp Oeser commented 2021-06-09 19:20:41 +02:00
Member
Copy Link

In #63650#661781, @brecht wrote:
This is a task for a developer to fix, not a UI design issue I think.

Changing the vertex group name should update the dependency graph, because bones and vertex groups are linked by name. I think the problem is that the first click is for changing the active vertex group, which can affect the viewport in weight paint mode for example.

There are a few things that could be improved here:

  1. If a vertex group is already active, then clicking it again should not cause any depsgraph update. (probably simple)
  2. Ideally changing the active vertex group would have more fine-grained tagging so it doesn't affect anything when it doesn't affect any viewport. (may be simple if it's a matter of checking if the object is in weight paint mode in the RNA update function, would need to be checked)
  3. Events coming from GHOST should ideally have a timestamp, and that timestamp should be used to determine the double clicking time. (too much work to handle as a bug report)

In #63650#686049, @brecht wrote:
The issue is that on double click two different things are done:

  • On the first click, it makes the item active which require a dependency graph update.
  • On the second click, it starts renaming.

It's not the renaming that's the problem, but the making active that happens right before that.

> In #63650#661781, @brecht wrote: > This is a task for a developer to fix, not a UI design issue I think. > > Changing the vertex group name should update the dependency graph, because bones and vertex groups are linked by name. I think the problem is that the first click is for changing the active vertex group, which can affect the viewport in weight paint mode for example. > > There are a few things that could be improved here: > 1) If a vertex group is already active, then clicking it again should not cause any depsgraph update. (probably simple) > 2) Ideally changing the active vertex group would have more fine-grained tagging so it doesn't affect anything when it doesn't affect any viewport. (may be simple if it's a matter of checking if the object is in weight paint mode in the RNA update function, would need to be checked) > 3) Events coming from GHOST should ideally have a timestamp, and that timestamp should be used to determine the double clicking time. (too much work to handle as a bug report) > In #63650#686049, @brecht wrote: > The issue is that on double click two different things are done: > * On the first click, it makes the item active which require a dependency graph update. > * On the second click, it starts renaming. > > It's not the renaming that's the problem, but the making active that happens right before that.
Philipp Oeser commented 2021-06-09 19:21:51 +02:00
Member
Copy Link

Added subscribers: @Hucky, @mano-wii, @finnb, @JacquesLucke, @ZedDB, @WilliamReynish, @Dspazio

Added subscribers: @Hucky, @mano-wii, @finnb, @JacquesLucke, @ZedDB, @WilliamReynish, @Dspazio
Philipp Oeser commented 2021-06-09 19:22:40 +02:00
Member
Copy Link

Added subscriber: @kursadk

Added subscriber: @kursadk
Richard Antalik commented 2021-10-04 00:23:19 +02:00
Member
Copy Link

Added subscriber: @Jens.Ne

Added subscriber: @Jens.Ne
Vladimir commented 2021-10-04 05:55:11 +02:00
Copy Link

Added subscriber: @Wovchick

Added subscriber: @Wovchick
Philipp Oeser removed the
Interest
User Interface
 label 2023-02-10 09:26:33 +01:00
Philipp Oeser commented 2023-11-14 13:08:20 +01:00
Member
Copy Link

Sameis true for UVMaps, see #114835

Sameis true for UVMaps, see #114835
Philipp Oeser commented 2023-11-14 13:09:45 +01:00
Member
Copy Link
  • If a vertex group is already active, then clicking it again should not cause any depsgraph update. (probably simple)
  • Ideally changing the active vertex group would have more fine-grained tagging so it doesn't affect anything when it doesn't affect any viewport. (may be simple if it's a matter of checking if the object is in weight paint mode in the RNA update function, would need to be checked)

Will check on these.

>- If a vertex group is already active, then clicking it again should not cause any depsgraph update. (probably simple) >- Ideally changing the active vertex group would have more fine-grained tagging so it doesn't affect anything when it doesn't affect any viewport. (may be simple if it's a matter of checking if the object is in weight paint mode in the RNA update function, would need to be checked) Will check on these.
❤️ 1
Philipp Oeser self-assigned this 2023-11-14 13:09:55 +01:00
Bastien Montagne was unassigned by Philipp Oeser 2023-11-14 13:10:07 +01:00
Philipp Oeser removed their assignment 2023-11-14 13:10:07 +01:00
Philipp Oeser self-assigned this 2023-11-14 13:10:10 +01:00
Ludvik Koutny commented 2023-11-14 13:54:17 +01:00
Contributor
Copy Link

Will check on these.

I would just make sure that #106903 is not the ultimate fix for this. If it is, then maybe it may not be worth for you to spend time on this now, and instead maybe add this subset of issues to that task as well, to bump its priority.

> Will check on these. I would just make sure that https://projects.blender.org/blender/blender/pulls/106903 is not the ultimate fix for this. If it is, then maybe it may not be worth for you to spend time on this now, and instead maybe add this subset of issues to that task as well, to bump its priority.
👍 1
Philipp Oeser commented 2023-11-24 11:19:19 +01:00
Member
Copy Link

I do have a local solution that is specific to e.g. vertexgroups (tweaking their RNA property set and property update functions), however, I would first like to check on a more general solution (and if this is known to cause problems):

The following diff just showcases this for INT properties and listrows, but it could be broadened to a wider set of cases:
(it save us from the property's PropIntSetFunc and the UpdateFunc to be executed if the value did not actually change -- in the case of this report: when clicking on the same vertexgroup in the list)

diff --git a/source/blender/editors/interface/interface.cc b/source/blender/editors/interface/interface.cc
index 6d9e216e50b..1ef3b42665f 100644
--- a/source/blender/editors/interface/interface.cc
+++ b/source/blender/editors/interface/interface.cc
@@ -2610,7 +2610,7 @@ double ui_but_value_get(uiBut *but)
   return value;
 }
 
-void ui_but_value_set(uiBut *but, double value)
+bool ui_but_value_set(uiBut *but, double value)
 {
   /* Value is a HSV value: convert to RGB. */
   if (but->rnaprop) {
@@ -2631,7 +2631,13 @@ void ui_but_value_set(uiBut *but, double value)
             RNA_property_int_set_index(&but->rnapoin, prop, but->rnaindex, int(value));
           }
           else {
-            RNA_property_int_set(&but->rnapoin, prop, int(value));
+            if (RNA_property_int_get(&but->rnapoin, prop) == int(value)) {
+              /* Value did not change. */
+              return false;
+            }
+            else {
+              RNA_property_int_set(&but->rnapoin, prop, int(value));
+            }
           }
           break;
         case PROP_FLOAT:
@@ -2704,6 +2710,8 @@ void ui_but_value_set(uiBut *but, double value)
   }
 
   ui_but_update_select_flag(but, &value);
+
+  return true;
 }
 
 int ui_but_string_get_maxncpy(uiBut *but)
diff --git a/source/blender/editors/interface/interface_handlers.cc b/source/blender/editors/interface/interface_handlers.cc
index e0a1fd189c3..9f9ac35d635 100644
--- a/source/blender/editors/interface/interface_handlers.cc
+++ b/source/blender/editors/interface/interface_handlers.cc
@@ -1160,7 +1160,10 @@ static void ui_apply_but_TOG(bContext *C, uiBut *but, uiHandleButtonData *data)
 
 static void ui_apply_but_ROW(bContext *C, uiBlock *block, uiBut *but, uiHandleButtonData *data)
 {
-  ui_but_value_set(but, but->hardmax);
+  if (!ui_but_value_set(but, but->hardmax)) {
+    /* Value did not change. */
+    return;
+  }
 
   ui_apply_but_func(C, but);
 
diff --git a/source/blender/editors/interface/interface_intern.hh b/source/blender/editors/interface/interface_intern.hh
index abf1facc7ec..bfc23aa085a 100644
--- a/source/blender/editors/interface/interface_intern.hh
+++ b/source/blender/editors/interface/interface_intern.hh
@@ -690,7 +690,10 @@ void ui_block_add_dynamic_listener(uiBlock *block,
 uiBut *ui_but_change_type(uiBut *but, eButType new_type);
 
 double ui_but_value_get(uiBut *but);
-void ui_but_value_set(uiBut *but, double value);
+/**
+ * Returns false if the value did not change.
+ */
+bool ui_but_value_set(uiBut *but, double value);
 /**
  * For picker, while editing HSV.
  */

@filedescriptor : I recall you had a try at this as well (that got reverted because there were issue with drivers? Undo?) Was this done along the lines of the above? Sill remeber which commit this was?

@brecht @JulianEisel : does the above sound like it is worth pursuing? (otherwise I might still put up a PR with the more localized solution for just a few properties...)

Thx for checking in advance!

I do have a local solution that is specific to e.g. vertexgroups (tweaking their RNA property set and property update functions), however, I would first like to check on a more general solution (and if this is known to cause problems): The following diff just showcases this for INT properties and listrows, but it could be broadened to a wider set of cases: (it save us from the property's `PropIntSetFunc` and the `UpdateFunc` to be executed if the value did not actually change -- in the case of this report: when clicking on the same vertexgroup in the list) ``` diff diff --git a/source/blender/editors/interface/interface.cc b/source/blender/editors/interface/interface.cc index 6d9e216e50b..1ef3b42665f 100644 --- a/source/blender/editors/interface/interface.cc +++ b/source/blender/editors/interface/interface.cc @@ -2610,7 +2610,7 @@ double ui_but_value_get(uiBut *but) return value; } -void ui_but_value_set(uiBut *but, double value) +bool ui_but_value_set(uiBut *but, double value) { /* Value is a HSV value: convert to RGB. */ if (but->rnaprop) { @@ -2631,7 +2631,13 @@ void ui_but_value_set(uiBut *but, double value) RNA_property_int_set_index(&but->rnapoin, prop, but->rnaindex, int(value)); } else { - RNA_property_int_set(&but->rnapoin, prop, int(value)); + if (RNA_property_int_get(&but->rnapoin, prop) == int(value)) { + /* Value did not change. */ + return false; + } + else { + RNA_property_int_set(&but->rnapoin, prop, int(value)); + } } break; case PROP_FLOAT: @@ -2704,6 +2710,8 @@ void ui_but_value_set(uiBut *but, double value) } ui_but_update_select_flag(but, &value); + + return true; } int ui_but_string_get_maxncpy(uiBut *but) diff --git a/source/blender/editors/interface/interface_handlers.cc b/source/blender/editors/interface/interface_handlers.cc index e0a1fd189c3..9f9ac35d635 100644 --- a/source/blender/editors/interface/interface_handlers.cc +++ b/source/blender/editors/interface/interface_handlers.cc @@ -1160,7 +1160,10 @@ static void ui_apply_but_TOG(bContext *C, uiBut *but, uiHandleButtonData *data) static void ui_apply_but_ROW(bContext *C, uiBlock *block, uiBut *but, uiHandleButtonData *data) { - ui_but_value_set(but, but->hardmax); + if (!ui_but_value_set(but, but->hardmax)) { + /* Value did not change. */ + return; + } ui_apply_but_func(C, but); diff --git a/source/blender/editors/interface/interface_intern.hh b/source/blender/editors/interface/interface_intern.hh index abf1facc7ec..bfc23aa085a 100644 --- a/source/blender/editors/interface/interface_intern.hh +++ b/source/blender/editors/interface/interface_intern.hh @@ -690,7 +690,10 @@ void ui_block_add_dynamic_listener(uiBlock *block, uiBut *ui_but_change_type(uiBut *but, eButType new_type); double ui_but_value_get(uiBut *but); -void ui_but_value_set(uiBut *but, double value); +/** + * Returns false if the value did not change. + */ +bool ui_but_value_set(uiBut *but, double value); /** * For picker, while editing HSV. */ ``` @filedescriptor : I recall you had a try at this as well (that got reverted because there were issue with drivers? Undo?) Was this done along the lines of the above? Sill remeber which commit this was? @brecht @JulianEisel : does the above sound like it is worth pursuing? (otherwise I might still put up a PR with the more localized solution for just a few properties...) Thx for checking in advance!
Falk David commented 2023-11-24 11:35:32 +01:00
Member
Copy Link

@lichtwerk Yes, this looks very familiar. I combed through the history on the archive to find all the related tasks and changes.

Quote from @brecht: "I left a code comment [see below] suggesting to implement this at the RNA set/update level, it's too fragile to reliably which updates may or may not be required at this level of the code."

Here was the code comment:

/* This is intended to avoid unnecessary updates when the value stays the same, however there
 * are issues with the current implementation. It does not work with multi-button editing
 * (T89996) or operator popups where a number button requires an update even if the value is
 * unchanged (T89996).
 *
 * Trying to detect changes at this level is not reliable. Instead it could be done at the
 * level of RNA update/set, skipping RNA update if RNA set did not change anything, instead
 * of skipping all button updates. */
@lichtwerk Yes, this looks very familiar. I combed through the history on the archive to find all the related tasks and changes. * Original fix: [Fix T87448: Avoid uiBut update if value was same](https://archive.blender.org/developer/differential/0010/0010976/index.html) * Caused issues: * [Fix T87637: Dragging button value cancel not working](https://archive.blender.org/developer/differential/0011/0011021/index.html) * [Fix T87688: Crash entering valid text into number field](https://archive.blender.org/developer/differential/0011/0011049/index.html) * [Fix T88058: Hover+return doesn't accept 0 as input](https://archive.blender.org/developer/differential/0011/0011168/index.html) * [Fix T89265: Crash when tabbing through num inputs](https://archive.blender.org/developer/differential/0011/0011679/index.html) * [Fix T89996: Resetting multi-button doesn't work ](https://archive.blender.org/developer/differential/0012/0012000/index.html) (was a proposed fix, but got rejected, because it seemed too unstable) * And commit that reverted the changes (with the reasoning): 3bf10e5d0ac297a70a9bc294fff0448e6b11fc20 Quote from @brecht: "I left a code comment [see below] suggesting to implement this at the RNA set/update level, it's too fragile to reliably which updates may or may not be required at this level of the code." Here was the code comment: ```cpp /* This is intended to avoid unnecessary updates when the value stays the same, however there * are issues with the current implementation. It does not work with multi-button editing * (T89996) or operator popups where a number button requires an update even if the value is * unchanged (T89996). * * Trying to detect changes at this level is not reliable. Instead it could be done at the * level of RNA update/set, skipping RNA update if RNA set did not change anything, instead * of skipping all button updates. */ ```
❤️ 1
Philipp Oeser commented 2023-11-24 12:36:51 +01:00
Member
Copy Link

Thx for the info @filedescriptor !

I tweaked this a bit, so now the whole button handling is basically untouched, it is now really just the property's PropIntSetFunc and the UpdateFunc to be skipped from execution if the value did not actually change:

diff --git a/source/blender/editors/interface/interface.cc b/source/blender/editors/interface/interface.cc
index 6d9e216e50b..1ef3b42665f 100644
--- a/source/blender/editors/interface/interface.cc
+++ b/source/blender/editors/interface/interface.cc
@@ -2610,7 +2610,7 @@ double ui_but_value_get(uiBut *but)
   return value;
 }
 
-void ui_but_value_set(uiBut *but, double value)
+bool ui_but_value_set(uiBut *but, double value)
 {
   /* Value is a HSV value: convert to RGB. */
   if (but->rnaprop) {
@@ -2631,7 +2631,13 @@ void ui_but_value_set(uiBut *but, double value)
             RNA_property_int_set_index(&but->rnapoin, prop, but->rnaindex, int(value));
           }
           else {
-            RNA_property_int_set(&but->rnapoin, prop, int(value));
+            if (RNA_property_int_get(&but->rnapoin, prop) == int(value)) {
+              /* Value did not change. */
+              return false;
+            }
+            else {
+              RNA_property_int_set(&but->rnapoin, prop, int(value));
+            }
           }
           break;
         case PROP_FLOAT:
@@ -2704,6 +2710,8 @@ void ui_but_value_set(uiBut *but, double value)
   }
 
   ui_but_update_select_flag(but, &value);
+
+  return true;
 }
 
 int ui_but_string_get_maxncpy(uiBut *but)
diff --git a/source/blender/editors/interface/interface_handlers.cc b/source/blender/editors/interface/interface_handlers.cc
index e0a1fd189c3..ee8506d6567 100644
--- a/source/blender/editors/interface/interface_handlers.cc
+++ b/source/blender/editors/interface/interface_handlers.cc
@@ -497,6 +497,8 @@ struct uiAfterFunc {
 
   PointerRNA rnapoin;
   PropertyRNA *rnaprop;
+  /* If the value did not change we can skip the update. */
+  bool rna_skip_update;
 
   void *search_arg;
   uiFreeArgFunc search_arg_free_fn;
@@ -826,7 +828,7 @@ static bool ui_afterfunc_check(const uiBlock *block, const uiBut *but)
  * handling is done, i.e. menus are closed, in order to avoid conflicts
  * with these functions removing the buttons we are working with.
  */
-static void ui_apply_but_func(bContext *C, uiBut *but)
+static void ui_apply_but_func(bContext *C, uiBut *but, const bool rna_skip_update=false)
 {
   uiBlock *block = but->block;
   if (!ui_afterfunc_check(block, but)) {
@@ -875,6 +877,7 @@ static void ui_apply_but_func(bContext *C, uiBut *but)
 
   after->rnapoin = but->rnapoin;
   after->rnaprop = but->rnaprop;
+  after->rna_skip_update = rna_skip_update;
 
   if (but->type == UI_BTYPE_SEARCH_MENU) {
     uiButSearch *search_but = (uiButSearch *)but;
@@ -1048,7 +1051,7 @@ static void ui_apply_but_funcs_after(bContext *C)
       WM_operator_properties_free(&opptr);
     }
 
-    if (after.rnapoin.data) {
+    if (after.rnapoin.data && !after.rna_skip_update) {
       RNA_property_update(C, &after.rnapoin, after.rnaprop);
     }
 
@@ -1160,9 +1163,9 @@ static void ui_apply_but_TOG(bContext *C, uiBut *but, uiHandleButtonData *data)
 
 static void ui_apply_but_ROW(bContext *C, uiBlock *block, uiBut *but, uiHandleButtonData *data)
 {
-  ui_but_value_set(but, but->hardmax);
+  const bool changed = ui_but_value_set(but, but->hardmax);
 
-  ui_apply_but_func(C, but);
+  ui_apply_but_func(C, but, !changed);
 
   /* states of other row buttons */
   LISTBASE_FOREACH (uiBut *, bt, &block->buttons) {
diff --git a/source/blender/editors/interface/interface_intern.hh b/source/blender/editors/interface/interface_intern.hh
index abf1facc7ec..bfc23aa085a 100644
--- a/source/blender/editors/interface/interface_intern.hh
+++ b/source/blender/editors/interface/interface_intern.hh
@@ -690,7 +690,10 @@ void ui_block_add_dynamic_listener(uiBlock *block,
 uiBut *ui_but_change_type(uiBut *but, eButType new_type);
 
 double ui_but_value_get(uiBut *but);
-void ui_but_value_set(uiBut *but, double value);
+/**
+ * Returns false if the value did not change.
+ */
+bool ui_but_value_set(uiBut *but, double value);
 /**
  * For picker, while editing HSV.
  */

@brecht : this seems to follow your advise from the quoted comment above?

Thx for the info @filedescriptor ! I tweaked this a bit, so now the whole button handling is basically untouched, it is now really just the property's `PropIntSetFunc` and the `UpdateFunc` to be skipped from execution if the value did not actually change: ```diff diff --git a/source/blender/editors/interface/interface.cc b/source/blender/editors/interface/interface.cc index 6d9e216e50b..1ef3b42665f 100644 --- a/source/blender/editors/interface/interface.cc +++ b/source/blender/editors/interface/interface.cc @@ -2610,7 +2610,7 @@ double ui_but_value_get(uiBut *but) return value; } -void ui_but_value_set(uiBut *but, double value) +bool ui_but_value_set(uiBut *but, double value) { /* Value is a HSV value: convert to RGB. */ if (but->rnaprop) { @@ -2631,7 +2631,13 @@ void ui_but_value_set(uiBut *but, double value) RNA_property_int_set_index(&but->rnapoin, prop, but->rnaindex, int(value)); } else { - RNA_property_int_set(&but->rnapoin, prop, int(value)); + if (RNA_property_int_get(&but->rnapoin, prop) == int(value)) { + /* Value did not change. */ + return false; + } + else { + RNA_property_int_set(&but->rnapoin, prop, int(value)); + } } break; case PROP_FLOAT: @@ -2704,6 +2710,8 @@ void ui_but_value_set(uiBut *but, double value) } ui_but_update_select_flag(but, &value); + + return true; } int ui_but_string_get_maxncpy(uiBut *but) diff --git a/source/blender/editors/interface/interface_handlers.cc b/source/blender/editors/interface/interface_handlers.cc index e0a1fd189c3..ee8506d6567 100644 --- a/source/blender/editors/interface/interface_handlers.cc +++ b/source/blender/editors/interface/interface_handlers.cc @@ -497,6 +497,8 @@ struct uiAfterFunc { PointerRNA rnapoin; PropertyRNA *rnaprop; + /* If the value did not change we can skip the update. */ + bool rna_skip_update; void *search_arg; uiFreeArgFunc search_arg_free_fn; @@ -826,7 +828,7 @@ static bool ui_afterfunc_check(const uiBlock *block, const uiBut *but) * handling is done, i.e. menus are closed, in order to avoid conflicts * with these functions removing the buttons we are working with. */ -static void ui_apply_but_func(bContext *C, uiBut *but) +static void ui_apply_but_func(bContext *C, uiBut *but, const bool rna_skip_update=false) { uiBlock *block = but->block; if (!ui_afterfunc_check(block, but)) { @@ -875,6 +877,7 @@ static void ui_apply_but_func(bContext *C, uiBut *but) after->rnapoin = but->rnapoin; after->rnaprop = but->rnaprop; + after->rna_skip_update = rna_skip_update; if (but->type == UI_BTYPE_SEARCH_MENU) { uiButSearch *search_but = (uiButSearch *)but; @@ -1048,7 +1051,7 @@ static void ui_apply_but_funcs_after(bContext *C) WM_operator_properties_free(&opptr); } - if (after.rnapoin.data) { + if (after.rnapoin.data && !after.rna_skip_update) { RNA_property_update(C, &after.rnapoin, after.rnaprop); } @@ -1160,9 +1163,9 @@ static void ui_apply_but_TOG(bContext *C, uiBut *but, uiHandleButtonData *data) static void ui_apply_but_ROW(bContext *C, uiBlock *block, uiBut *but, uiHandleButtonData *data) { - ui_but_value_set(but, but->hardmax); + const bool changed = ui_but_value_set(but, but->hardmax); - ui_apply_but_func(C, but); + ui_apply_but_func(C, but, !changed); /* states of other row buttons */ LISTBASE_FOREACH (uiBut *, bt, &block->buttons) { diff --git a/source/blender/editors/interface/interface_intern.hh b/source/blender/editors/interface/interface_intern.hh index abf1facc7ec..bfc23aa085a 100644 --- a/source/blender/editors/interface/interface_intern.hh +++ b/source/blender/editors/interface/interface_intern.hh @@ -690,7 +690,10 @@ void ui_block_add_dynamic_listener(uiBlock *block, uiBut *ui_but_change_type(uiBut *but, eButType new_type); double ui_but_value_get(uiBut *but); -void ui_but_value_set(uiBut *but, double value); +/** + * Returns false if the value did not change. + */ +bool ui_but_value_set(uiBut *but, double value); /** * For picker, while editing HSV. */ ``` @brecht : this seems to follow your advise from the quoted comment above?
👍 1 ❤️ 1
Bastien Montagne commented 2023-11-24 14:49:45 +01:00
Owner
Copy Link

Looks like a good solution yes, but imho the whole ui_but_value_set could use some cleanup (do same 'skip update' for other types, harmonize handling of values between RNA and 'raw' data, etc.).

Looks like a good solution yes, but imho the whole `ui_but_value_set` could use some cleanup (do same 'skip update' for other types, harmonize handling of values between RNA and 'raw' data, etc.).
Brecht Van Lommel commented 2023-11-24 15:31:21 +01:00
Owner
Copy Link

It seems fine, the value can be compared against ui_but_value_get at the start of ui_but_value_set to make it work for all types.

It seems fine, the value can be compared against `ui_but_value_get` at the start of `ui_but_value_set` to make it work for all types.
❤️ 1
Campbell Barton commented 2023-11-30 11:54:20 +01:00
Owner
Copy Link

While avoiding additional updates is good, it doesn´t solve the problem that slow event handling breaks double-click test on WIN32.

We could resolve this by improving event time-stamps for WIN32.

The double-click bug isn´t an issue on Linux/Wayland and from what I can tell macOS wont have this problem either - since both platforms event time-stamps aren´t delayed by Blender refreshing slowly.

On macOS the system events look to have a time-stamp AFAICS this information isn´t available on WIN32 though, so we could consume events in a thread as is done on Wayland.

While adding threaded event handling is a fairly large change only to resolve double-clicking, it has some potential up-sides such as allowing actions to be cancelled that are currently blocking.

While avoiding additional updates is good, it doesn´t solve the problem that slow event handling breaks double-click test on WIN32. We could resolve this by improving event time-stamps for WIN32. The double-click bug isn´t an issue on Linux/Wayland and from what I can tell macOS wont have this problem either - since both platforms event time-stamps aren´t delayed by Blender refreshing slowly. On macOS the system events look to have a time-stamp AFAICS this information isn´t available on WIN32 though, so we could consume events in a thread as is done on Wayland. While adding threaded event handling is a fairly large change only to resolve double-clicking, it has some potential up-sides such as allowing actions to be cancelled that are currently blocking.
Philipp Oeser commented 2023-11-30 12:13:31 +01:00
Member
Copy Link

The double-click bug isn´t an issue on Linux/Wayland and from what I can tell macOS wont have this problem either - since both platforms event time-stamps aren´t delayed by Blender refreshing slowly.

I sure have the issue on linux.
@ideasman42 : have you tried this with large enough meshes (e.g. just follow ##114835)

> The double-click bug isn´t an issue on Linux/Wayland and from what I can tell macOS wont have this problem either - since both platforms event time-stamps aren´t delayed by Blender refreshing slowly. I sure have the issue on linux. @ideasman42 : have you tried this with large enough meshes (e.g. just follow ##114835)
Philipp Oeser commented 2023-11-30 12:30:57 +01:00
Member
Copy Link

I will try to get out the PR the coming days (atm., triaging has a bit of priority)

I will try to get out the PR the coming days (atm., triaging has a bit of priority)
Campbell Barton commented 2023-12-03 05:13:19 +01:00
Owner
Copy Link

The double-click bug isn´t an issue on Linux/Wayland and from what I can tell macOS wont have this problem either - since both platforms event time-stamps aren´t delayed by Blender refreshing slowly.

I sure have the issue on linux.
@ideasman42 : have you tried this with large enough meshes (e.g. just follow ##114835)

Ah, I tested on a heavy scene but not heavy enough, confirmed this issue (even with threaded event handling).
Committed a fix that uses the time-stamps from Wayland events & resolves the issue: 4a735b1d05.
The same solution can be used for X11 (which supplies time-stamps), which would make WIN32 the only remaining platform with the bug, if we can find a way to support a event time-stamps there, this bug could be closed.

> > The double-click bug isn´t an issue on Linux/Wayland and from what I can tell macOS wont have this problem either - since both platforms event time-stamps aren´t delayed by Blender refreshing slowly. > > I sure have the issue on linux. > @ideasman42 : have you tried this with large enough meshes (e.g. just follow ##114835) Ah, I tested on a heavy scene but not heavy enough, confirmed this issue (even with threaded event handling). Committed a fix that uses the time-stamps from Wayland events & resolves the issue: 4a735b1d05af64711873ea531cc6d30e4f33e016. The same solution can be used for X11 (which supplies time-stamps), which would make WIN32 the only remaining platform with the bug, if we can find a way to support a event time-stamps there, this bug could be closed.
❤️ 1
Brecht Van Lommel commented 2023-12-04 20:45:32 +01:00
Owner
Copy Link

There seems to be GetMessageTime for Windows. As used in various UI toolkits and described here.
https://devblogs.microsoft.com/oldnewthing/20041018-00/?p=37543

Resolution is only 16ms, but should be ok for this. Also need to take into account wraparound somehow.

There seems to be `GetMessageTime` for Windows. As used in various UI toolkits and described here. https://devblogs.microsoft.com/oldnewthing/20041018-00/?p=37543 Resolution is only 16ms, but should be ok for this. Also need to take into account wraparound somehow.
Campbell Barton commented 2023-12-06 09:52:03 +01:00
Owner
Copy Link

Committed fix for X11 (efef709ec7).
Is anyone on WIN32 able to investigate the solution Brecht suggests?

Committed fix for X11 (efef709ec73ce95817a82e4e26cf7ec0bad8eca0). Is anyone on WIN32 able to investigate the solution Brecht suggests?
❤️ 1
Blender Bot added
Status
Resolved
 and removed
Status
Confirmed
 labels 2023-12-08 19:00:53 +01:00
Brecht Van Lommel added the
Interest
User Interface
 label 2023-12-11 16:09:12 +01:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels   
Interest
Alembic

  
Interest
Animation & Rigging

  
Interest
Asset System
Asset datablocks, libraries, browser and shelf

  
Interest
Audio

  
Interest
Automated Testing

  
Interest
Blender Asset Bundle

  
Interest
BlendFile

  
Interest
Collada

  
Interest
Compatibility
Backward and forward compatibility

  
Interest
Compositing

  
Interest
Core

  
Interest
Cycles

  
Interest
Dependency Graph

  
Interest
Development Management

  
Interest
EEVEE

  
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
Viewport & EEVEE

  
Interest
Virtual Reality

  
Interest
Vulkan

  
Interest
Wayland
Wayland windowing on Unix

  
Interest
Workbench

  
Interest: X11

Xorg/X11 windowing

  
Legacy
Asset Browser Project

Archived

  
Legacy
Blender 2.8 Project

Archived

  
Legacy
Milestone 1: Basic, Local Asset Browser

Archived

  
Legacy
OpenGL Error

Archived

  
Meta
Good First Issue

  
Meta
Papercut

  
Meta
Retrospective

  
Meta
Security
Related to security, https://wiki.blender.org/wiki/Process/Vulnerability_Reports

  
Module
Animation & Rigging

  
Module
Core

  
Module
Development Management

  
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

  
Module
Viewport & EEVEE

  
Platform
FreeBSD

  
Platform
Linux

  
Platform
macOS

  
Platform
Windows

  
Severity
High

  
Severity
Low

  
Severity
Normal

  
Severity
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

Archived

  
Type
Report

Archived

  
Type
To Do

No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
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
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
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
Philipp Oeser
11 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#40009
No description provided.
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?