[Kstars-devel] Review Request 112879: Setting altitude and azimuth to default in skypoint when not set from catalog

[Henry de Valence]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112879/#review40591
-----------------------------------------------------------


Apologies in advance for the somewhat lengthy comment.

It's not clear to me either
a) what the problem we're attempting to solve is
or
b) whether this patch does indeed solve the problem.

First, a note about the SkyPoint class. This class has IMO a fatal design flaw, in that it is impossible for code to properly use a SkyPoint without having extra information about the point.

A SkyPoint object, contrary to the name, does not store a point in the sky. It stores three different points in the sky, in three different coordinate systems. Users of the SkyPoint have to magically know what the correct coordinate system is, or they will use the class incorrectly. There are some parts of KStars that rely on the three stored coordinate sets storing the *same* point in different coordinate systems, while other parts rely on the three stored coordinate sets storing *different* points. For example, the code that does the horizontal coordinate grid creates SkyPoints that use horizontal coordinates only.

The best solution to this, and many other, related, problems, is to get rid of the SkyPoint entirely and replace it with a different type for each coordinate system. This bug, as far as I can see, is of this form: we have a SkyPoint which is only initialized in coordinate system A, and try to use it with coordinate system B. There are also many other bugs of the type where we change the coordinates in system A, and forget to update the stored coordinates in system B. If we had different C++ types for different types of coordinate systems, all of these bugs would turn into compile-time errors and go away.

However, the problem is compounded by the way that KStars' inheritance hierarchy complects the SkyPoint with nearly every other sky object class, so it's very difficult to try to change the way that the SkyPoint works. This is one major problem that I ran into with my GSoC project. It's also very tricky to change the behaviour of the SkyPoint internally: how can you know whether your change breaks any of the 500+ existing uses of the class? Writing unit tests for the SkyPoint class is also not going to work, because to have a unit test, you need to know what the correct behaviour of the class is supposed to be. I'm not even sure that it's possible for it to behave correctly: I suspect that different parts of the code rely on SkyPoint in contradictory ways.

So, there's a deeper, more fundamental problem here.

Going back to the questions a) and b), this patch might fix the referenced bug. 

But it certainly doesn't fix the bigger problem with the SkyPoint, which IMO requires removing the SkyPoint class entirely and rewriting everything that uses it. It's also not clear to me what impact this patch has on the behavour of all the other uses of the SkyPoint class -- can we be sure that it doesn't break anything else?

I think that a better solution would be as follows:

-- When SkyPoint::HorizontalToEquatorial is called without proper AltAz coordinates, KStars should crash immediately.

I don't think that there is any other acceptable solution. It is a programming error to convert from coordinates which have not been set. There'



- Henry de Valence


On Sept. 23, 2013, 2:20 a.m., Vijay Dhameliya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112879/
> -----------------------------------------------------------
> 
> (Updated Sept. 23, 2013, 2:20 a.m.)
> 
> 
> Review request for KStars, Akarsh Simha and Rafal Kulaga.
> 
> 
> Description
> -------
> 
> If Alt/Az or RA/Dec are not finite before using them in further calculation in HorizontalToEquatorial and nutate method then it leads to error in further calculation. 
> 
> 
> This addresses bug 322848.
>     http://bugs.kde.org/show_bug.cgi?id=322848
> 
> 
> Diffs
> -----
> 
>   kstars/skyobjects/skypoint.cpp f634075 
> 
> Diff: http://git.reviewboard.kde.org/r/112879/diff/
> 
> 
> Testing
> -------
> 
> Tested - Runs well
> Bug resolved permanently 
> 
> 
> Thanks,
> 
> Vijay Dhameliya
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kstars-devel/attachments/20130923/2a66e933/attachment.html>