Fix issue with orbitControl rotating inversely on the X axis#2956
Fix issue with orbitControl rotating inversely on the X axis#2956dekmm wants to merge 2 commits intoprocessing:masterfrom dekmm:master
Conversation
|
Not sure if the inverted rotation was intented, but I would like to add an optional argument so the user can choose between normal and inverted rotation. |
There was a problem hiding this comment.
thanks for this! let's file a separate issue for adding a parameter to orbitControl.
I think I understand the impulse to change this.width to this.height in the rotateX case (it does feel like it's more correct, technically), but what this means is that the user will rotate at different speeds in different directions. Is that the intent? How does it feel as a user?
|
I guess I never noticed the different rotation speeds since I tested the change on a canvas of equal width and height. Must've slipped my mind that I made the change. I'm assuming the change isn't that drastic unless a user uses some weird resolution for the canvas such as 1000x100 or has OCD and these little inconsistencies drive him crazy :P but I'll test it out and change it back if necessary. |
|
@dekmm one context I was thinking of was using the browser's developer console, which usually results in a screen much wider than tall because the dev panel takes up the bottom half of the screen. I'm often doing this to run tests or debug. |
|
@kjhollen good point. I probably would've missed this since my dev console is attached to the side and creates a square-like canvas. I'll revert the change. |
|
I wonder if it behaves differently if the canvas is large vs if it is small? I think users might expect the turning rate to be related to the 'size' of the canvas, but also in a way that the horizontal & vertical turning rates are the same. (I have a feeling that getting this exactly right would depend on inspecting the camera matrix, but on the other hand, I think that assuming a square pixel aspect ratio would probably be sufficient.) |
|
thanks for updating this. I just merged #2962 which included a similar fix + more features, so I'm going to close this, but the update was right on. :) |
Addressing issue explained in #2930