acowley / GLUtil

Utility functions for working with OpenGL BufferObjects, GLSL shaders, and textures.
BSD 3-Clause "New" or "Revised" License
40 stars 16 forks source link

Inconsistent rotations for Camera3D #18

Open rabipelais opened 9 years ago

rabipelais commented 9 years ago

The panRad function pre-multiplies the new rotation to the old orientation, while tiltRad and rollRad post-multiply it. This leads, for example, to the following unexpected behaviour:

Is this behaviour desired?

acowley commented 9 years ago

No, that doesn't sound good. I'll merge a PR.

acowley commented 9 years ago

I suspect I made things inconsistent because I had a camera control scenario where I wanted panning relative to the global up axis. I think making things consistent as you've done is the right thing, but I'm tempted to add an alternative pan that does things the old way. One argument for offering an option unique to panning is that it is rotation with respect to a commonly implicit axis induced by gravity. Do you think it would make sense to offer alternatives to other camera rotations to offer developer-friendly camera controls?

rabipelais commented 9 years ago

I had the use case where the program read gyroscopic data, where roll, pitch, and yaw were always in global coordinates. I changed tilt and roll to match pan, and wrote ...Local and ...LocalRad versions. Since local rotations were the common case for me in the rest of the program, I swapped them and had ...Global... versions. So I suppose it is not a very uncommon use case. They are, however, simple functions to write. I don't know what you prefer with regards to having a larger, but possibly more complete, API. (I also wrote a reorientate :: Quaternion a -> Camera a -> Camera a function. Didn't like the name, though.)

acowley commented 9 years ago

I guess we should have global versions, too.

I don't love the expansion, but this seems like a pretty common distinction. I agree they're simple functions, but part of the point of the linear package was just to provide common definitions of simple things. I guess I'd call reorientate reorient just by virtue of the latter being slightly shorter.

My vote is to add them all in.

rabipelais commented 9 years ago

That makes sense. Then I also think it is a good idea to add them. Quick question, would you expect that reorient substitutes the orientation of the camera completely, or multiplies it to the correct orientation, i.e. reorient q c = c { orientation = orientation c * q } (then again the question about local vs. global axis)?

rabipelais commented 9 years ago

(The PR only adds the ...Global and ...GlobalRad versions, not the reorient function. I think better names are due.)

acowley commented 9 years ago

Yeah, I was thinking about that multiply vs. substitute question. The ambiguity demonstrates the problem with the name. I'd expect reorient to replace the orientation completely, while something like rotateCamera to do the multiplication, but I don't know if my reading of those names is representative. The former could be something like resetOrientation which is perhaps more clear.