cryos / avogadro

Avogadro 1 is not under active development, the repository was archived in September 2021. Development of Avogadro 2 is being done at https://github.com/openchemistry/avogadrolibs. Avogadro is an advanced molecular editor designed for cross-platform use in computational chemistry, molecular modeling, bioinformatics, materials science, and related areas.
http://avogadro.cc/
GNU General Public License v2.0
334 stars 156 forks source link

projection problem #89

Closed cryos closed 8 years ago

cryos commented 17 years ago

The camera's project function is projecting into a co-ordinate system with 0,0 at the bottom left and the y axis going up instead of having 0,0 at the top left and the y axis going down.

Reported by: rbraith

cryos commented 17 years ago

Logged In: NO

Why do you regard this as a problem? There's no universal convention as to which coordinate system is better. 2D systems as in Qt tend indeed to have the y-axis pointing downwards, but in mathematics and in OpenGL it is more common to have the y axis pointing upwards. Here's why: in OpenGL, in absence of a modelview transformation, the z axis is pointing toward the camera. Hence, if you want the x-axis to point rightward, and you want (x,y,z) to be a direct orthonormal basis (meaning that z=cross(x,y)), then the y-axis must point upwards, not downwards. Of course, this is only because of the arbitrary convention of what's called a "direct" basis.

Anyway, if you see a reason to, feel free to change that. Just note that if you do so, the code will have to be adapted at several places.

Original comment by: nobody

cryos commented 17 years ago

Logged In: YES user_id=1557018 Originator: NO

I was not logged in when I posted the previous comment.

Original comment by: benoitjacob

cryos commented 17 years ago

Logged In: YES user_id=1781194 Originator: YES

There is one very good reason for changing it. Internal consistency. The cameras unProject fucntion expects a coordinate with 0,0 at top left hand corner and y going down, it does this to work easily with the mouse. The project function should reverse this transformation perfectly but it doesn't it changes the y coordinate. For this reason alone it needs to be changed.

Original comment by: rbraith

cryos commented 17 years ago

Logged In: YES user_id=1557018 Originator: NO

D'Oh! I didn't realize there was this inconsistency between project() and unProject(), and that's indeed a very good reason. Whatever convention is adopted, project() and unProject() should indeed agree on the same convention!

So you're very welcome to make that change (I can't hack on avogadro currently).

Re-tagging this issue as a 'bug'.

Original comment by: benoitjacob

cryos commented 17 years ago

Logged In: YES user_id=1500648 Originator: NO

Marked as pending for testing. Fixed in commit 594. We should stick with QT coordinate system which has 0,0 at the top left.

Original comment by: @milkypostman

cryos commented 17 years ago

Logged In: YES user_id=1557018 Originator: NO

Hey, I just did svn up and tested, and there's a bug: the label engine now paints labels at wrong positions. As I said, this change must be done together with changes in the various places in the code where project() is called.

Original comment by: benoitjacob

cryos commented 17 years ago

Logged In: YES user_id=1500648 Originator: NO

benoit, i have fixed the problem. The only place it currently uses project is to render the labels on the front of atoms, and it simply needed to reorient the coordinates because there is a glTranslatef using the screen coordinates. Which seems weird but i think i know why it's done.

Original comment by: @milkypostman

cryos commented 17 years ago

Logged In: YES user_id=1312539 Originator: NO

This Tracker item was closed automatically by the system. It was previously set to a Pending status, and the original submitter did not respond within 14 days (the time period specified by the administrator of this Tracker).

Original comment by: sf-robot