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

[Rafal Kulaga]

> On Sept. 23, 2013, 5 p.m., Henry de Valence wrote:
> > 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 wrote:
>     Apologies, I hit return prematurely. Continuing:
>     
>     There's no good way to "recover" from the problem at this point; it's already too late.
>     
>     If we don't crash, we just propogate garbage through the program, making it harder to find the original error.
>     
>     Moreover, crashing immediately lets us detect all the errors that we would have picked up at compile time if the design wasn't so broken, so that we can fix these errors in the source.
>     
>     Of course, in the long term the best solution is to fix the SkyPoint class. But for now, I think that exposing broken behaviour is the best thing to do.
>     
>

I couldn't agree more with you as it comes to the design of the SkyPoint class. The exact scenario in which this "bug" happens is very simple:

1) During startup we read an object (represented by a SkyPoint subclass) which has incorrect coordinates from a file. Those coordinates are replaced with NaN values.
2) When any of those two methods is called for this object, the Q_ASSERT statements terminates KStars. This won't happen for the end user (unless he is running a debug build).

 


- Rafal


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


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/105a92e2/attachment.html>