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

Improve PanZoom camera performance when non-+z direction is used #1682

Merged
merged 3 commits into from Aug 26, 2019

Conversation

os-gabe
Copy link
Contributor

@os-gabe os-gabe commented Jul 29, 2019

Fixes #1681 by keeping a MatrixTransformation that gets modified on each iteration rather than overwritten

… on each iteration rather than overwritten
Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By removing the if statement aren't you now adding a penalty to the +z case when it isn't needed?

My other thought is, is it possible to modify either the rect or the main transform once whenever up is set and then apply that to all calculations from then on? Or would that require a Rect class that understands orientation? Just throwing ideas out there I haven't actually looked at the math very much.

@os-gabe
Copy link
Contributor Author

os-gabe commented Jul 29, 2019

I went back and forth on leaving the if statement and decided the computation was cheap enough that it was a good trade-off for simplifying the logic by removing the if statement.

I like where you are going with only computing the up transform whenever it changes but I'm not seeing a straightforward way to do that at the moment.

@djhoese
Copy link
Member

djhoese commented Jul 30, 2019

I'm having a hard time being ok with the if statement being removed. Could you add it back. I can see how only creating the MatrixTransform once is a huge performance gain (it stops shaders from being recompiled every time), but the other calculations in there aren't nothing. Especially when you consider they aren't needed at all for +z.

I spent 30 minutes looking in to a different approach to this and I think it could be done, but has so many possible points of failure I'm a little worried. The up property isn't actually used anywhere in the base camera and neither is self.transform. That should mean all of the changes can be limited to the panzoom.py module. I originally though self._real_rect was the camera's rectangle oriented for the self.up property, but it isn't and is only used in the _update_transform method in question. So besides this update method the other use of the transform for the camera is viewbox_mouse_event and it is only used to get the current distance the mouse moved or the current pointing position of the mouse. I'm a little confused at why the mouse wheel event uses the Scene's transform but a mouse move uses the camera's transform. We'd have to make sure that the transform isn't linked via event handling to some other piece of code. Otherwise I don't see why it can't be handling this orientation stuff. I'm sure I'm missing something. Just brainstorming, don't feel like you have to do anything about this.

@os-gabe
Copy link
Contributor Author

os-gabe commented Aug 16, 2019

What do you think about getting this merged @djhoese? I don't think my change is causing the failed CI test is it? I don't really understand the error there.

@djhoese
Copy link
Member

djhoese commented Aug 16, 2019

Darn, I forgot about this. I really wanted to come up with a better overall solution that didn't need the extra logic for changing up. I think I'm ok merging this, but am leaving for the weekend and have been kind of swamped with other projects' CIs failing. I restarted the failing job which was due to an HTTP timeout so that should pass. I'll plan on merging next week.

@djhoese djhoese merged commit 75d8bfb into vispy:master Aug 26, 2019
@djhoese
Copy link
Member

djhoese commented Aug 26, 2019

Sorry, forgot about this again. Thanks.

@djhoese djhoese changed the title Fixes #1681 by keeping a MatrixTransformation that gets modified on e… Improve PanZoom camera performance when non-+z direction Oct 13, 2019
@djhoese djhoese changed the title Improve PanZoom camera performance when non-+z direction Improve PanZoom camera performance when non-+z direction is used Oct 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PanZoom camera interaction becomes unusable if the up parameter is changed from "+z"
2 participants