Opened 15 years ago
Closed 15 years ago
#37 closed defect (fixed)
Orthographic (orth.js) Inverse Function is broken
| Reported by: | winwaed | Owned by: | madair |
|---|---|---|---|
| Priority: | major | Milestone: | 1.0.1 |
| Component: | core | Version: | trunk |
| Keywords: | orthographic, ortho, inverse | Cc: |
Description
Whilst implementing the Gnomonic (gnom) projection, I used the orthographic (ortho) implementation in ortho.js as a crib. I was able to successfully adapt the forward projection, and I have had no problems actually using the forward orthographic transformation in my development project.
However, the inverse transformation (which I do not use) appears to be broken in at least two places. First, the sine and cosine of the "z" intermediate angle are pre-calculated for efficiency. For some reason, the cosine is stored in the undefined variable "cosi" which is never declared or used. It should be stored in "cosz" (which is declared and used but never set!). Similarly in the line that starts "lat = Proj4js.common.asinz( ", the variable "y" is used, when it should probably be "p.y".
In fact, I have had a lot of trouble making sense of most of the code from this point onwards. The layout of the if statements is a little confusing, and it is difficult to tell if they are correct or not.
As I don't have the ability or unit tests to check this, I am reporting the first two problems rather than fixing them now. I am of the opinion that the entire inverse function should be checked and tested.
Attachments (1)
Change History (6)
follow-up: 2 comment:1 by , 15 years ago
| Milestone: | future → 1.0.1 |
|---|---|
| Status: | new → assigned |
| Version: | → trunk |
comment:2 by , 15 years ago
Replying to madair:
can you provide a patch for this? thanks.
I could quickly apply those two fixes outlined above, but it would continue to be untested and I would continue to distrust the inverse function.
comment:3 by , 15 years ago
That's ok, it's just sometimes easier to see the change you are proposing using a patch. If you have a test point we can add that to the test\testdata.js file.
by , 15 years ago
ortho.js with the "typos" fixed in the inverse function.
comment:4 by , 15 years ago
Note I would continue to distrust the inverse ortho function. It couldn't have worked in the first place with these "typos". It hasn't been tested properly. The "IF" statement structure is a little confusing but might be correct (I haven't analyzed it in detail).
comment:5 by , 15 years ago
| Resolution: | → fixed |
|---|---|
| Status: | assigned → closed |
those changes make sense so I'll make the change. (rev 1721)
I can't find a test point for this so it will stay in the non-validated list of projections on the wiki. I will be opening another ticket to implement something to use the MetaCRS test dataset.
can you provide a patch for this? thanks.