-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Adding lock for Viser Element callbacks #3067
Adding lock for Viser Element callbacks #3067
Conversation
What kinds of crash did you see? I also experienced crashes (#3064 ) in the viser viewer. |
This PR fixes a bug with a race condition in custom viewer element callbacks (eg a ViewerButton which renders the current view), previously this might clash with the standard rendering thread and crash the callback or main renderer. |
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.
lgtm!
@@ -209,12 +211,19 @@ def __init__( | |||
# Keep track of the pointers to generated GUI folders, because each generated folder holds a unique ID. | |||
viewer_gui_folders = dict() | |||
|
|||
def prev_cb_wrapper(prev_cb): | |||
def cb_lock(element): |
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.
add a docstring describing why this is being done
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.
and maybe explain the logic a bit since functors can be confusing
* add callback lock, prepare for testing Co-authored-by: Justin Kerr <justin.g.kerr@gmail.com>
@kerrj I found this PR impacts the experience of ns-viewer when training gaussian splatting. The delay when I rotate the scene is so long that basically disallow me to interact with the scene. Do you observe similar issue? |
That's odd I don't notice any difference. In fact it shouldn't affect things since the only code that was added to the lock was viser element callbacks, which splatfacto doesn't have any of. |
I added a wrapper around the viser element callback so the render thread is blocked while the callback is called to avoid crashing. I also added a lock in run_viewer.py so this issue is addressed for both ns-train and ns-viewer.
Test: I added time.sleep() in the callback and confirmed the render thread was blocked.