Stars drawing (speed and color)

[Khudyakov Alexey]

Hello

After recent fix in star drawing code I looked into it. I think usage of
QString as keys for cache isn't the best thing to do. I've tried to experiment
with other indexing schemes. So far I got ~10% speedup for map update with
maximum star density. Sadly code become harder to read.


Another problem I spotted is function StarObject::updateColors. It appears
only in commented code in the StarComponent which is marked "REMOVE" [1]
Moreover it doesn't know anything about color modes.

So question is whether it a historical remnant which should be removed?


[1] skycomponents/starcomponent.cpp:301
--
  Khudyakov Alexey
_______________________________________________
Kstars-devel mailing list
Kstars-devel@...
https://mail.kde.org/mailman/listinfo/kstars-devel

[Khudyakov Alexey]

> Follow-up on this subject
>
Another problematic place is StarObject::initImages. It uses pixmaps of size
15*SkyMap::Instance()->scale(). Then algorithm which actaully create pixmaps
assume that they are 15x15 pixels. Which behaviour is correct?


And last one is
> double StarObject::pmMagnitude()
> {
>    return sqrt( pmRA() * pmRA() + pmDec() * pmDec() );
> }
If this functionfion calculate absolute value of proper motion it should use
following formula:

sqrt( v_{RA}^2 * cos(dec)^2 + v_dec^2 )
_______________________________________________
Kstars-devel mailing list
Kstars-devel@...
https://mail.kde.org/mailman/listinfo/kstars-devel

[Khudyakov Alexey]

В сообщении от 02 ноября 2009 22:03:33 Akarsh Simha написал:

> > After recent fix in star drawing code I looked into it. I think usage of
> > QString as keys for cache isn't the best thing to do. I've tried to
> > experiment with other indexing schemes. So far I got ~10% speedup for map
> > update with maximum star density. Sadly code become harder to read.
>
> Wow! That's awesome. I think it is okay to sacrifice code readability
> for the speed increase, considering that it's annoyingly slow right
> now. I think it should be okay to commit it, as long as you have some
> comments explaining what's going on and what the indexing scheme is.
>

I think speed and readability rarely conflict with each other. Unless you go
into inner working of computers and even in that case it's not obvious.
Any algorithm could be written in the form it's reasonably easy to read
especially if one knows what he is doing.

BTW my experiment with profiling shown that about 8% of time KStars spent on
QString allocation/deallocation. This might be a cause.

> > Another problematic place is StarObject::initImages. It uses pixmaps of
> > size 15*SkyMap::Instance()->scale(). Then algorithm which actaully create
> > pixmaps assume that they are 15x15 pixels. Which behaviour is correct?
>
> I think they can be 15x15, since the star size is recomputed using the
> zoom factor anyway. If that's the screen scale, it still shouldn't
> matter. What we must preserve is the relative sizes of stars. I think
> it's okay to use 15x15, unless it looks awful on some very big
> displays. Which one will be faster?
>

Variant with fixed resolution. Star image is 15x15 pixels anyway. If scale > 1.
then star pixmap just have invisible margins. They are completely transparent.

> > Another problem I spotted is function StarObject::updateColors. It
> > appears only in commented code in the StarComponent which is marked
> > "REMOVE" [1] Moreover it doesn't know anything about color modes.
> >
> > So question is whether it a historical remnant which should be removed?
>
> I've noticed that too. I think it's okay to remove it, although I
> really don't know. In any case, we can always revert the change if
> required.
>

KStars compile without this function. I'll remove it. Unused code tends to rot
and accumulate bugs over time.
_______________________________________________
Kstars-devel mailing list
Kstars-devel@...
https://mail.kde.org/mailman/listinfo/kstars-devel

[Akarsh Simha-2]

> After recent fix in star drawing code I looked into it. I think usage of
> QString as keys for cache isn't the best thing to do. I've tried to experiment
> with other indexing schemes. So far I got ~10% speedup for map update with
> maximum star density. Sadly code become harder to read.

Wow! That's awesome. I think it is okay to sacrifice code readability
for the speed increase, considering that it's annoyingly slow right
now. I think it should be okay to commit it, as long as you have some
comments explaining what's going on and what the indexing scheme is.

> Another problem I spotted is function StarObject::updateColors. It appears
> only in commented code in the StarComponent which is marked "REMOVE" [1]
> Moreover it doesn't know anything about color modes.

> So question is whether it a historical remnant which should be removed?

I've noticed that too. I think it's okay to remove it, although I
really don't know. In any case, we can always revert the change if
required.

Regards
Akarsh
_______________________________________________
Kstars-devel mailing list
Kstars-devel@...
https://mail.kde.org/mailman/listinfo/kstars-devel

[Akarsh Simha-2]

On Mon, Nov 02, 2009 at 08:39:13PM +0300, Alexey Khudyakov wrote:
> > Follow-up on this subject
> >
> Another problematic place is StarObject::initImages. It uses pixmaps of size
> 15*SkyMap::Instance()->scale(). Then algorithm which actaully create pixmaps
> assume that they are 15x15 pixels. Which behaviour is correct?

I think they can be 15x15, since the star size is recomputed using the
zoom factor anyway. If that's the screen scale, it still shouldn't
matter. What we must preserve is the relative sizes of stars. I think
it's okay to use 15x15, unless it looks awful on some very big
displays. Which one will be faster?

> And last one is
> > double StarObject::pmMagnitude()
> > {
> >    return sqrt( pmRA() * pmRA() + pmDec() * pmDec() );
> > }
> If this functionfion calculate absolute value of proper motion it should use
> following formula:
>
> sqrt( v_{RA}^2 * cos(dec)^2 + v_dec^2 )

Hmm... you seem to be right, and that also means that I'll need to
regenerate USNO NOMAD! I'll work on this next weekend, hopefully. But
feel free to experiment around.

Regards
Akarsh
_______________________________________________
Kstars-devel mailing list
Kstars-devel@...
https://mail.kde.org/mailman/listinfo/kstars-devel