WIP: UI: dont run update functions if button values did not change #115831

Philipp Oeser · 2023-12-06T10:50:25+01:00

Philipp Oeser commented

2023-12-06 10:50:25 +01:00

Symptoms were recalculation in cases were these are not needed (e.g.
putting the same value as before on a heavy modifier property would give
a delay, same as selecting e.g. the same vertexgroup that was previously
selected). As shown in #40009, this delay could also end up swallowing
doublick-events, so on a heavy mesh e.g. a vertexgroup could not be
renamed by doublicking on it.

The doubleclick issue can be solved with event timestamps (see
efef709ec7 for this being done for linux), however the - unneccessary

delay would still take place.

This PR skip RNA property set/update functions in case a button value
did not change, thus eliminating the delay.

It is basically the second try at this (see eb06ccc324 which got
reverted), but this time leaving the button handling code untouched.

Fixes #87448
Fixes #40009

Symptoms were recalculation in cases were these are not needed (e.g. putting the same value as before on a heavy modifier property would give a delay, same as selecting e.g. the same vertexgroup that was previously selected). As shown in #40009, this delay could also end up swallowing doublick-events, so on a heavy mesh e.g. a vertexgroup could not be renamed by doublicking on it. The doubleclick issue can be solved with event timestamps (see efef709ec73c for this being done for linux), however the - unneccessary - delay would still take place. This PR skip RNA property set/update functions in case a button value did not change, thus eliminating the delay. It is basically the second try at this (see eb06ccc32462 which got reverted), but this time leaving the button handling code untouched. Fixes #87448 Fixes #40009

👍 2 👀 1 ❤️ 2

Philipp Oeser added 1 commit 2023-12-06 10:50:39 +01:00

67510b89d3 UI: dont run update functions if button values did not change

Symptoms were recalculation in cases were these are not needed (e.g.
putting the same value as before on a heavy modifier property would give
a delay, same as selecting e.g. the same vertexgroup that was previously
selected). As shown in #40009, this delay could also end up swallowing
doublick-events, so on a heavy mesh e.g. a vertexgroup could not be
renamed by doublicking on it.

The doubleclick issue can be solved with event timestamps (see
efef709ec7 for this being done for linux), however the - unneccessary
- delay would still take place.

This PR skip RNA property set/update functions in case a button value
did not change, thus eliminating the delay.

It is basically the second try at this (see eb06ccc324 which got
reverted), but this time leaving the button handling code untouched.

Fixes #87448
Fixes #40009

Philipp Oeser added this to the User Interface project 2023-12-06 10:50:51 +01:00

Philipp Oeser requested review from Brecht Van Lommel 2023-12-06 10:50:57 +01:00

Philipp Oeser requested review from Campbell Barton 2023-12-06 10:51:05 +01:00

Philipp Oeser requested review from Julian Eisel 2023-12-06 10:51:14 +01:00

Philipp Oeser added 1 commit 2023-12-06 10:54:24 +01:00

92def30fcb remove whitespace

Philipp Oeser referenced this pull request

2023-12-06 11:15:17 +01:00

User Interface unresponsive with High Poly scenes #114835

Campbell Barton requested changes 2023-12-19 03:11:42 +01:00

Campbell Barton left a comment

Testing this patch, it seems to work well for the most part, however it breaks PROP_ENUM_FLAG properties.

To reproduce the bug:

Add "Mesh Sequence Modifier" to a mesh.
Click on "Read Data" 's "Vertex" Button.
Shift click on "Faces" (now both are pressed)
Shift click on "Vertex" (now only Faces is pressed).
Shift click on "Vertex" (nothing happens)

In main the last step will toggle the button.
This problem occurs because the value of PROP_ENUM_FLAG buttons is the bit to toggle, not the final value.

Other potential issues:

UI_BUT_POIN_FLOAT buttons found value <= std::abs(0.00001) to zero, while it's odd to do this - it means this PR is making an unintended functional change.
For char/short/int types rounding & clamping is done on the value before it's assigned and after it's compared with the buttons original value, as this currently only skips RNA updates, non-RNA buttons shouldn't be an issue but it's not correct AFAICS.
but->editval is not set when it otherwise would be, while I couldn't make this result in any bugs, it seems error prone & could cause problems in the future.
UI_BUT_VALUE_UNSET could previously be set for unhandled types (not sure if this is important right now, but it's a funcitonal change).

Attached a patch which applies to this PR and resolves these issues, although I'm not all that keen on it (adds checks inline & tracks changed state).

A more general issue is this workaround is only for numbers, not colors, vectors, strings, anyone reading the code might ask why numbers are special cases - and why every other button doesn't attempt skipping unnecessary work. If we want to make number an exception, OK, then an explanation should be added to the code-comment explaining why this is a pain-point for numbers and why strings and other button types don't do this check.
This update-if-changed check also isn't applied for multi-button editing, Python API and other areas... so while skipping updates seems reasonable at first, I think this is more of a "special case" than it might first seem.

I'd suggest to investigate a solution that deals with clicking on already active UI-list item, but if that's not practical, a solution similar to the attached patch could work however it's hard to predict how skipping updates might cause problems.

While in principle it's logical to avoid unnecessary work, in practice users rarely set a button to it's exact same value (with the exception of UIList 's) and the drawback for failing to run update-callbacks is significant.

The UIList code could be changed so clicking on the current active item doesn't send through the index assignment (although this isn't very elegant either as it's a UI-specific logic that's applied in one place).

Testing this patch, it seems to work well for the most part, however it breaks `PROP_ENUM_FLAG` properties. To reproduce the bug: - Add "Mesh Sequence Modifier" to a mesh. - Click on "Read Data" 's "Vertex" Button. - Shift click on "Faces" (now both are pressed) - Shift click on "Vertex" (now only Faces is pressed). - **Shift click on "Vertex" (nothing happens)** In `main` the last step will toggle the button. This problem occurs because the value of `PROP_ENUM_FLAG` buttons is the bit to toggle, not the final value. --- Other _potential_ issues: - `UI_BUT_POIN_FLOAT` buttons found `value <= std::abs(0.00001)` to zero, while it's odd to do this - it means this PR is making an unintended functional change. - For char/short/int types rounding & clamping is done on the value before it's assigned and after it's compared with the buttons original value, as this currently only skips RNA updates, non-RNA buttons shouldn't be an issue but it's not *correct* AFAICS. - `but->editval` is not set when it otherwise would be, while I couldn't make this result in any bugs, it seems error prone & could cause problems in the future. - `UI_BUT_VALUE_UNSET` could previously be set for unhandled types (not sure if this is important right now, but it's a funcitonal change). Attached a patch which applies to this PR and resolves these issues, although I'm not all that keen on it (adds checks inline & tracks changed state). --- A more general issue is this workaround is only for numbers, not colors, vectors, strings, anyone reading the code might ask why numbers are special cases - and why every other button doesn't attempt skipping unnecessary work. If we want to make number an exception, OK, then an explanation should be added to the code-comment explaining why this is a pain-point for numbers and why strings and other button types don't do this check. This update-if-changed check also isn't applied for multi-button editing, Python API and other areas... so while skipping updates seems reasonable at first, I think this is more of a "special case" than it might first seem. ---- I'd suggest to investigate a solution that deals with clicking on already active UI-list item, but if that's not practical, a solution similar to the attached patch could work however it's hard to predict how skipping updates might cause problems. While in principle it's logical to avoid unnecessary work, in practice users rarely set a button to it's exact same value (with the exception of `UIList` 's) and the drawback for failing to run update-callbacks is significant. The UIList code could be changed so clicking on the current active item doesn't send through the index assignment (although this isn't very elegant either as it's a UI-specific logic that's applied in one place).

115831_update.diff

3.8 KiB

Philipp Oeser commented

2023-12-20 09:18:45 +01:00

Thx @ideasman42 for checking in such depth!

I agree the most pressing issue (UIList doubleclick) has been solved already by the timestamps commits, so the urgency decreased considerably. I'll still have it on the list, will check on the issues mentioned (but with a lower prio for now, so might take a bit to post an update...)

Thx @ideasman42 for checking in such depth! I agree the most pressing issue (UIList doubleclick) has been solved already by the timestamps commits, so the urgency decreased considerably. I'll still have it on the list, will check on the issues mentioned (but with a lower prio for now, so might take a bit to post an update...)

Philipp Oeser changed title from ~~UI: dont run update functions if button values did not change~~ to WIP: UI: dont run update functions if button values did not change

2024-01-25 12:00:27 +01:00

This pull request is marked as a work in progress.

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u 40009_b:lichtwerk-40009_b

git checkout lichtwerk-40009_b

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

WIP: UI: dont run update functions if button values did not change #115831

Checkout