-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix redoPlacement blocking main thread, fix #3727 #3731
Conversation
// Don't start a new placement request when the current one hasn't completed yet, or when | ||
// we are parsing buckets. | ||
if (workRequest) return; | ||
|
||
workRequest.reset(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that line, and two others removed in fa522e02fe8fe58382a82643f9e4884136c59b76
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok to remove the other two because workRequest =
will destruct/cancel the old one, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's safe to remove this one; if (workRequest) return;
guarantees that reset()
would be a no-op. For the other ones, there's a difference:
- reset, create new: Ensures the current task is ended and the callback for it will not be called, then starts a new one.
- create new, assign: The callback for the previous task could still be executed, which might put things into an unexpected state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, thanks! I should've checked. changed.
fa522e0
to
53dda0f
Compare
👍 |
I had the impression that by doing this we could potentially make text placement non deterministic. I.e. if you rotate from angle A to B fast enough, in some cases we could skip the last My solution was in a situation like there is a task ongoing, queue the |
If you're rotating fast enough it could skip some of the intermediate Whenever the map is rotated |
If a workRequest exists, don't cancel it and start an new request. It's ok to just do nothing if the request exists because a new call to redoPlacement will be triggered after the existing request finishes. this fixes a regression in e44db93
53dda0f
to
0439d00
Compare
💯 thanks a lot for explaining, |
fix #3727
If a workRequest exists, don't cancel it and start an new request. It's ok to just do nothing if the request exists because a new call to redoPlacement will be triggered after the existing request finishes.
This fixes a regression in #3543
@tmpsantos can you review?