[usdview] Split _updateOnFrameChange logic. Simplified the FrameSlider class. #770

ianlawson · 2019-02-21T04:10:30Z

The three PR's I've submitted tonight will allow me to submit a final PR for the current frame feature once they have been merged into dev!

Description of Change(s)

Split _updateOnFrameChange into _updateOnFrameChange and _updateOnFrameChangeFinish
- Allows us to differentiate between what we want to update whilst the frame is changing (playing/scrubbing) and what we want to update when the frame change has finished.
- Removes the need for the refreshUI variable.
Simplified the FrameSlider class
- Reverted to using Qt's logic for scrubbing/tracking which improves accuracy and performance.
- In doing so, fixed a minor bug that was causing extra UI updates whilst scrubbing.
  - If the action of scrubbing happens towards the start or end, isSliderDown returns False rather than True.
- Only using the class to override necessary mouse events.
- Removed unused _playbackFrameIndex variable.

pxr/usdImaging/lib/usdviewq/appController.py

spiffmon · 2019-02-25T02:49:13Z

On the second commit that rewrites the FrameSlider class... it looks like you have completely removed the "update when pausing scrubbing" behavior that kicks in when "Update on scrubbing" is turned off. I think we like that feature - can you work it back in? Thanks!

ianlawson · 2019-02-26T01:44:30Z

Thanks for your comments - they've been addressed and the PR has been updated!

jilliene · 2019-02-28T01:31:41Z

Filed as internal issue #USD-5096

spiffmon · 2019-03-08T23:57:22Z

pxr/usdImaging/lib/usdviewq/appController.py



    def _playClicked(self):
        if self._ui.playButton.isChecked():
+            # Enable tracking whilst playing
+            self._ui.frameSlider.setTracking(True)


Maybe I'll discover why later, but why do we want tracking on during playback?

This is to mirror the behavior that was already there! And...

We want mouse tracking to be enabled as the user might choose to click somewhere on the slider as it is playing. We want that new play point to be respected regardless of whether the frame scrub option is enabled/disabled!

Ah, that makes sense - thanks, @ianlawson !

spiffmon · 2019-03-09T00:19:33Z

pxr/usdImaging/lib/usdviewq/appController.py

+            value = self._ui.frameSlider.value() + 1
+            if value > self._ui.frameSlider.maximum():
+                value = self._ui.frameSlider.minimum()
+            self._ui.frameSlider.setValue(value)



If the goal here (which I'm slowly coming around to) is to simplify the slider widget/controller, and put logic in the appController, then shouldn't we be implementing advance/retreat in terms of the appController concepts of currentFrame and stepSize, and then just having the slider update in response to a signal that frame has changed? Slider values should correspond exactly to indices in _timeSamples, right?

For reference, I've been hoping that at some point we'll migrate all of the time/playback-related controller logic into a TransportController class to make transport be a more modular piece.

Some really good points again here - definitely can make these changes!

I've been taking a closer look and found some more inconsistencies with the advance/retreat frame, especially when looking at hotkey presses and active focus (between the slider and the window).

I'd like to address all of this in another pull request if possible, what do you think?

That sounds good to me, @ianlawson !

spiffmon · 2019-03-09T00:56:20Z

pxr/usdImaging/lib/usdviewq/frameSlider.py

-        super(FrameSlider, self).mousePressEvent(event)
+            # Set the slider down property.
+            # This tells the slider to obey the tracking state.
+            self.setSliderDown(True)


Why do you want to loft up logic from the base class here rather than just delegating to it? Same comment for mouseReleaseEvent()

Sorry I don't quite follow! What do you mean by just delegating to it?

as it was doing previously, calling super(FrameSlider, self).mousePressEvent(event)

Sure but what logic/functionality do you think we can remove?

Yeah, this is a good shout. I've taken another look at the logic in QSlider and I've simplified the implementation in our subclass.

The perfect solution here would be to create a QProxyStyle. Then we could modify the value of QStyle::SH_Slider_AbsoluteSetButtons (which is currently set to just the middle mouse button) to include the left mouse button. However, this is only available in PyQt >= 5.5!

spiffmon · 2019-03-09T00:57:50Z

pxr/usdImaging/lib/usdviewq/frameSlider.py

+        )
+        # Set the slider value.
+        self.setSliderPosition(value)
+


This logical block is shared with mousePressEvent() - factor it out?

I was trying to do this originally, but I couldn't think of a suitable name for the function.

I'll refactor the logic out into setSliderPositionFromEvent

Sounds good to me!

spiffmon · 2019-03-12T00:50:06Z

Without investigating, I figured it took care of the slider state?

On Mon, Mar 11, 2019 at 5:10 PM Ian Lawson ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pxr/usdImaging/lib/usdviewq/frameSlider.py <#770 (comment)> : > if event.button() == QtCore.Qt.LeftButton: - self._mousePressed = True - self.setValueFromEvent(event) - event.accept() - super(FrameSlider, self).mousePressEvent(event) + # Set the slider down property. + # This tells the slider to obey the tracking state. + self.setSliderDown(True) Sure but what logic/functionality do you think we can remove? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#770 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF7qaJZDmiFcjmWzvUtBOSqZFyXpPYpdks5vVvB_gaJpZM4bGnWi> .

-- --spiffiPhone

spiffmon

Looks ready to go, to me, @ianlawson - thanks so much for doing this!

spiffmon · 2019-03-14T22:15:21Z

Hi @ianlawson . After @c64kernal pulled the change into our tree for final review, I did some manual testing, and discovered a couple things:

In Pyside (and the Qt.py adapter), QStyle lives in QtWidgets, not QtGui . I fixed this locally to proceed with testing, and would have just committed that and proceeded, but
It introduces a regression that is a little tricky to repro, but happened to me often enough. It happens when you click-n-hold on the timeline, away from the slider-head. Without "Redraw On Frame Scrub" enabled, what you get sometimes is that clicking just has no effect - the playhead is not summoned to your mouse, and dragging produces no movement. With "Redraw On Frame Scrub" enabled, the playhead is always summoned, but sometimes it stops off-center from your cursor - sometimes completely outside of your cursor, and when this happens, you can no longer scrub, i.e. mouse movement produces no playhead movement.

I think it may have something to do with rounding and something not getting updated correctly, but I couldn't pick out an obvious line of code in five minutes. This is easiest for me to reproduce if I make the window really wide, so that the timeline is long and there's lots more pixels than there are frames... which is what leads me to think it has something to do with rounding/mapping. I was able to repro with both a heavier animation that only achieves 9fps playback, but also with a light one that gets over 200 fps - it reprod more easily with the heavier anim, but that may have been also because it only had 47 frames of anim, whereas the light one had 100 frames.

Let me know if you have trouble reproing, I can probably share either or both assets with you - they are both synthetic.

On the plus side, this change fixes a regression that got introduced sometime in the last little while: after summoning the playhead on the slider, the arrow keys would no longer actually cause the scene to update until you hit the first or last frame!

I also discovered a long-standing bug that I'll file that after summoning the playhead a single time, the arrow keys will no longer loop from end to beginning or beginning to end - playhead just gets stuck at beginning or end.

ianlawson · 2019-03-14T22:34:40Z

I also discovered a long-standing bug that I'll file that after summoning the playhead a single time, the arrow keys will no longer loop from end to beginning or beginning to end - playhead just gets stuck at beginning or end.

I also noticed this - this is what I meant with the inconsistencies in advance/retreat frames in my earlier comment! Feel free to assign that ticket to me as I was planning on putting in a fix when I get some time.

spiffmon · 2019-03-14T23:31:07Z

@ianlawson - if you could, would you also rebase to current dev, which should now have your earlier changes?

Thanks!

ianlawson · 2019-03-14T23:32:16Z

Yep - on it, do you also want me to squash the extra commits?

spiffmon · 2019-03-14T23:55:54Z

Yeah, squashing would be awesome!

On Thu, Mar 14, 2019 at 4:32 PM Ian Lawson ***@***.***> wrote: Yep - on it, do you also want me to squash the extra commits? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#770 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF7qaGIVyLmKk7-UKl4QbDOL98fZgSixks5vWtwDgaJpZM4bGnWi> .

-- --spiffiPhone

ianlawson · 2019-03-27T18:29:41Z

Hey @spiffmon!

Anything left for me to do here or are we okay to merge? I'm keen to get the final PR into review for the current frame behavior! 😁

Thanks!

ianlawson · 2019-03-27T18:58:58Z

Ah of course - sorry, I'll look into that today!

ianlawson · 2019-03-27T20:57:13Z

Managed to reproduce both of your issues - thanks again for spotting them!

When Redraw On Frame Scrub is disabled, the 'no-update' issue looks to appear when the mouse click event occurs slightly above or below the main visible slider area.

This is definitely a regression, I'll get this fixed!

When Redraw On Frame Scrub is enabled, the 'off-center' issue occurs when we have a small number of time samples on the slider. The slider finds the closest time sample to where the mouse press event occurred and jumps to it's respective index.

This doesn't seem like a regression, however...
I agree that if you hold the mouse press you should be able to continue to move the control around and scrub, I'll get this fixed.
I tested this case without my changes (on the latest dev) and the behavior is extremely erratic, it looks to jump around (either side of the mouse press event) the timeline as your scrub.

Let me know what you think!

spiffmon · 2019-03-27T21:10:56Z

Agreed the behavior before your change was jumpy and janky, but I couldn’t get it to refuse to interact. If possible, it would be great to get both modes working? We definitely didn’t test well enough on the previous round of changes, and I don’t want to make that mistake again!

On Wed, Mar 27, 2019 at 1:57 PM Ian Lawson ***@***.***> wrote: Managed to reproduce both of your issues - thanks again for spotting them! When Redraw On Frame Scrub is *disabled*, the 'no-update' issue looks to appear when the mouse click event occurs slightly above or below the main visible slider area. - This is definitely a regression, I'll get this fixed! When Redraw On Frame Scrub is *enabled*, the 'off-center' issue occurs when we have a small number of time samples on the slider. The slider finds the closest time sample to where the mouse press event occurred and jumps to it's respective index. - This doesn't seem like a regression, however... - I agree that if you hold the mouse press you should be able to continue to move the control around and scrub, I'll get this fixed. - I tested this case without my changes (on the latest dev) and the behavior is extremely erratic, it looks to jump around (either side of the mouse press event) the timeline as your scrub. Let me know what you think! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#770 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF7qaC3dij7bCog8zt1ZM0ueK6LXttMrks5va9stgaJpZM4bGnWi> .

-- --spiffiPhone

ianlawson · 2019-03-27T21:11:59Z

Yep of course, I'll get this all fixed up and post an update later today!

ianlawson · 2019-04-03T17:33:39Z

@spiffmon - what are your thoughts? 😄

spiffmon · 2019-04-08T18:24:59Z

Hi @ianlawson - sorry I either hadn't gotten or missed the notification that you'd pushed new changes up! Am focused on doc improvements for 19.05 till its release, but will have a look after it gets pushed (hopefully mid-week). Thanks!

ianlawson · 2019-04-08T18:45:17Z

No worries at all!

Actually, just took a quick look. :-) If we're synthesizing a MM press, do we also need to synthesize a MM release when LM is released?

Nope, we're fine on that front. Only the mousePressEvent is concerned with what button is pressed!

…pdateOnFrameChangeFinish * Allows us to differentiate between what we want to update whilst the frame is changing (playing/scrubbing) and what we want to update when the frame change has finished. * Removes the need for the refreshUI variable.

* Reverted to using Qt's logic for scrubbing/tracking which improves accuracy and performance. * In doing so, fixed a minor bug that was causing extra UI updates whilst scrubbing. * If the action of scrubbing happens towards the start or end, isSliderDown returns False rather than True. * Only using the class to override necessary mouse events. * Removed unused _playbackFrameIndex variable.

* Changed _updateOnFrameChangeFinish to _updateGUIForFrameChange * Added mousePauseTimer to FrameSlider class * Simplified FrameSlider class, using more functionality from the base class. * Fixed minor bug in the state of the Play Button. * If you change the `Step Size` value whilst playing, it sets the internal state of the play button to False. Which means you have to hit the button twice to stop the playback.

…ddleButton presses to allow the FrameSlider to be manipulated correctly.

spiffmon · 2019-04-16T23:01:38Z

Huzzah! Testing went well - I had to change the module from QtWidgets to QtGui for QMouseEvent, and we can just do that fixup internally and accept. I think we are good to go, @ianlawson !

ianlawson · 2019-04-16T23:06:49Z

Awesome - once that's merged I'll get the PR in for the --curentframe behaviour!

[usdview] Split _updateOnFrameChange logic. Simplified the FrameSlider class. (Internal change: 1959450)

spiffmon reviewed Feb 25, 2019

View reviewed changes

pxr/usdImaging/lib/usdviewq/appController.py Outdated Show resolved Hide resolved

spiffmon requested changes Mar 9, 2019

View reviewed changes

spiffmon approved these changes Mar 13, 2019

View reviewed changes

ianlawson force-pushed the dev_frame_slider branch from e8d736d to cb17781 Compare March 15, 2019 01:04

ianlawson force-pushed the dev_frame_slider branch from cb17781 to 6a13f29 Compare March 27, 2019 23:35

Ian Lawson added 4 commits April 11, 2019 11:34

[usdview] Adjusted mousePressEvent to handle LeftButton presses as Mi…

fc4c1cd

…ddleButton presses to allow the FrameSlider to be manipulated correctly.

ianlawson force-pushed the dev_frame_slider branch 3 times, most recently from 3575c2f to fc4c1cd Compare April 12, 2019 03:23

c64kernal added the pending push label Apr 17, 2019

pixar-oss merged commit fc4c1cd into PixarAnimationStudios:dev Apr 25, 2019

pixar-oss added a commit that referenced this pull request Apr 25, 2019

Merge pull request #770 from ianlawson/dev_frame_slider

4e5054d

[usdview] Split _updateOnFrameChange logic. Simplified the FrameSlider class. (Internal change: 1959450)

ianlawson deleted the dev_frame_slider branch April 25, 2019 17:30

AlexSchwank mentioned this pull request Apr 29, 2019

[usdview] slider jitters #837

Closed

sunyab removed pending push labels Jun 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[usdview] Split _updateOnFrameChange logic. Simplified the FrameSlider class. #770

[usdview] Split _updateOnFrameChange logic. Simplified the FrameSlider class. #770

ianlawson commented Feb 21, 2019

spiffmon commented Feb 25, 2019

ianlawson commented Feb 26, 2019

jilliene commented Feb 28, 2019

spiffmon Mar 8, 2019

ianlawson Mar 11, 2019

spiffmon Mar 11, 2019

spiffmon Mar 9, 2019

ianlawson Mar 13, 2019

spiffmon Mar 13, 2019

spiffmon Mar 9, 2019

ianlawson Mar 11, 2019

spiffmon Mar 11, 2019

ianlawson Mar 12, 2019

ianlawson Mar 12, 2019

spiffmon Mar 9, 2019

ianlawson Mar 11, 2019

spiffmon Mar 11, 2019

spiffmon commented Mar 12, 2019 via email

spiffmon left a comment

spiffmon commented Mar 14, 2019

ianlawson commented Mar 14, 2019

spiffmon commented Mar 14, 2019

ianlawson commented Mar 14, 2019

spiffmon commented Mar 14, 2019 via email

ianlawson commented Mar 27, 2019

ianlawson commented Mar 27, 2019

ianlawson commented Mar 27, 2019

spiffmon commented Mar 27, 2019 via email

ianlawson commented Mar 27, 2019

ianlawson commented Apr 3, 2019 •

edited

Loading

spiffmon commented Apr 8, 2019

ianlawson commented Apr 8, 2019

spiffmon commented Apr 16, 2019

ianlawson commented Apr 16, 2019

[usdview] Split _updateOnFrameChange logic. Simplified the FrameSlider class. #770

[usdview] Split _updateOnFrameChange logic. Simplified the FrameSlider class. #770

Conversation

ianlawson commented Feb 21, 2019

Description of Change(s)

spiffmon commented Feb 25, 2019

ianlawson commented Feb 26, 2019

jilliene commented Feb 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spiffmon commented Mar 12, 2019 via email

spiffmon left a comment

Choose a reason for hiding this comment

spiffmon commented Mar 14, 2019

ianlawson commented Mar 14, 2019

spiffmon commented Mar 14, 2019

ianlawson commented Mar 14, 2019

spiffmon commented Mar 14, 2019 via email

ianlawson commented Mar 27, 2019

ianlawson commented Mar 27, 2019

ianlawson commented Mar 27, 2019

spiffmon commented Mar 27, 2019 via email

ianlawson commented Mar 27, 2019

ianlawson commented Apr 3, 2019 • edited Loading

spiffmon commented Apr 8, 2019

ianlawson commented Apr 8, 2019

spiffmon commented Apr 16, 2019

ianlawson commented Apr 16, 2019

ianlawson commented Apr 3, 2019 •

edited

Loading