Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a grace period between detecting a change and triggering generation in live preview #1412

Merged
merged 4 commits into from
Nov 29, 2024

Conversation

modelflat
Copy link
Contributor

As per discussion in #1248 this PR adds logic to delay the live preview generation upon change (without the extensions mentioned in this comment).

@Acly
Copy link
Owner

Acly commented Nov 19, 2024

This is more of an absolute timer though, which doesn't refresh if there are more changes after the first change, right?
I understood it as a (fairly short) period where there are no more changes instead.

It might be easier to implement that by tracking the timestamp of the last change. Since it's already a loop with a sleep it could be quite simple, something like

if self._last_input != new_input:
    current_time = time.monotonic()
    if current_time - self._last_change < grace_period:
        await self._model._generate_live(new_input)
        self._last_input = new_input
    self._last_change = time.monotonic()

I think this would make it unnecessary to pass _last_input to _generate_live and call _prepare there since that part is already handled. The just_got_here would also not be needed, since the _last_change timestamp would naturally be quite old in this case.

@modelflat modelflat force-pushed the feat/live-mode-event-buffering branch from 00e14fa to 81030fd Compare November 26, 2024 12:19
…on in live preview

* This will prevent some of the "useless" generations, e.g. from the very start of
  the brush stroke
* Period is configurable in settings; setting the default to 0 to
  preserve the existing behaviour

This at least partially addresses/follows the discussions in Acly#628 and Acly#1248
@modelflat modelflat force-pushed the feat/live-mode-event-buffering branch from 81030fd to 5d5e7a8 Compare November 26, 2024 12:20
@modelflat modelflat force-pushed the feat/live-mode-event-buffering branch from 5d5e7a8 to 98e64d8 Compare November 26, 2024 12:43
@modelflat
Copy link
Contributor Author

@Acly I like this idea - I updated the code to use simple timer as you suggested.

I also noticed that _continue_generating will exit for inactive documents, effectively turning off live mode for them. I don't think that should be the case, so I updated the loop to exit only when explicitly disabled.

@Acly
Copy link
Owner

Acly commented Nov 28, 2024

I also noticed that _continue_generating will exit for inactive documents, effectively turning off live mode for them. I don't think that should be the case, so I updated the loop to exit only when explicitly disabled.

That was intentional, the poll loop is fairly expensive and per document, to generate may cost money. Feels prudent to stop it when you work on something else. It might be annoying if you quickly switch documents all the time, but I don't think that's common.

@modelflat
Copy link
Contributor Author

modelflat commented Nov 28, 2024

the poll loop is fairly expensive

I don't think that's the case, because in my implementation, the loop does any actual work only if the document is active. If the document is inactive, it goes back to sleep again right after checking a single property, which can hardly be considered expensive. I'm pretty sure that's not gonna impact performance unless a user opens a lot of (100s?) documents and explicitly turns on live mode in all of them - in which case they probably wouldn't want live mode to turn off by itself when switching from/to documents.

@Acly
Copy link
Owner

Acly commented Nov 29, 2024

fair enough, I think either behavior makes sense, but don't have a super strong opinion on it.

I will merge & give it a try later, and also check if the setting can be automatic based on generation time (I like to avoid user settings where possible). What do you feel is a good value to set as grace period?

@Acly Acly merged commit 81f0195 into Acly:main Nov 29, 2024
2 checks passed
@Acly
Copy link
Owner

Acly commented Nov 29, 2024

Hmm it didn't really work like I expected, but not 100% sure what you intended.

I made small changes which make it work like I understood it: e12d241

@modelflat
Copy link
Contributor Author

didn't really work like I expected, but not 100% sure what you intended.

Checked out your branch; it is not the behaviour I intended, but it is interesting in its own way.

The intent of my solution is to block changes only for a short window of time, and after this window start generating anyway - even though the change may not be "complete" yet. The reasoning for this is:

  1. if a user makes a change, and then undoes it quickly enough, they don't want to see the intermediate state
  2. users want to see the "meaningful" changes. Here, "meaningful" != "complete" - because to complete the change may take long time, see (3). The generation can still start in the middle of the change, just not at the very beginning of it, because in order to be "meaningful" to the user, it should capture more than just a very first few pixels of it.
  3. if a user makes continuous changes that take a long time, they want to see the preview updating (e.g. making a slow, long brush stroke, or making lots of quick ones without any pauses). This is the base purpose of live mode.

Before my changes, only behaviour (3) was present. With my change, behaviours (1) and (2) are controllable (to an extent) with the grace period, while (3) is preserved.

As I can see, your solution instead waits for a "complete" change to happen, and start generating only when a no-change window occurs. Depending on which grace period value is chosen, this addresses either (2) or (3) from the list above, but not simultaneously.

I believe my implementation will provide better experience on both fast and average setups - based on the reasoning above - so I'd say we should keep it.

@Acly
Copy link
Owner

Acly commented Nov 30, 2024

Okay that clears things up, I just thought you implemented one thing but you did the other.

The downside to having only a maximum wait time since the first change is that you have to set it to a reasonabl large value to be useful, and for quick/instant changes (like an undo) it will take the full time to react. So my idea from the start was to just combine both.

See 842108b
It doesn't make it a lot more complex, and you get the best of both worlds (I hope). Also allows setting some decent fixed values, I think 250ms grace period and 3s max wait time feels good.

I'll try to disable the mechanism if generation is fast (less than 1s maybe) to get the minimum latency. Then it should be possible to enable this without setting for everyone, and have it adapt to the situation (resolution, model, gpu power...) at hand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants