[Kstars-devel] NOMAD 1e8 - the ugly way.

[James Bowlin]

On Sat August 9 2008, Akarsh Simha wrote:
> I managed to implement, [...], the handling of multiple meshes in
> KStars. 

Wow!  That's great!

> I don't like the way I do it now, but don't know enough OOP to decide
> of a better way to do things that will still be easy to implement. So
> I thought I should post here and get some feedback.

I agree with the approach of getting it working ugly first and then
cleaning up the code.

> Here's a summary of the changes:
> 1. DeepStarComponent is a copy of StarComponent that has a lot of
> methods and properties removed or modified.

Sounds good.

> 2. StarBlockList has an additional constructor that is called only
> from DeepStarComponent, that sets a StarBlockList::deepStars to true.
> I don't like the name, because there's also StarComponent::deepStars
> and DeepStarComponent::deepStars and they mean different things.

I agree that it is not a good idea to have the same name mean more
than one thing.  It sounds like your subclassing ideas below will
fix this problem, but if not, I'd suggest making the names more 
specific.  For example, "DeepStars_1e8" or "DeepStars_NOMAD", or
"DeepStars_16".

> 3. StarBlockList::fillToMag reads data using a 16-byte deepStarData
> structure or a 32-byte starData structure, depending on whether
> StarBlockList::deepStars is true or not.

You're probably right that we want to deal with this using subclassing
if we can.

> 4. SkyMapComposite has a QList that stores pointers to any
> DeepStarComponent classes. The constructor of SkyMapComposite looks
> for the catalogs and adds DeepStarComponents for each of them to this
> QList.

This sounds good.

> Notice that:
> 1. The Tycho-2 catalog is still handled by StarComponent

H'mm. IMO maybe the data structures should drive the class design.
I was thinking that StarComponent would only deal with named stars 
(pretty much the old pre-GSoC behavior) and DeepStarComponent
would deal with stars that get dynamically loaded.  I'm not down in
the trenches, so I don't know if there is a reason not to do it this
way, but I think this would make the overall structure more clean.

This might be the single best way to beautify the code.  

It also opens up the door for migrating the QList of DeepStarComponents
out of SkyMapComposite and into StarComponent.  I'm not sure this
migration is the best thing to do but it does make sense to have 
SkyMapComposite delegate as much component specific work as possible.

> 2. We do not have flexibility to go and use a 10-bytes per record
> data file (USNO A?)
> 3. What if we get enough data for fainter stars and wish to use a
> 32-byte data structure for them?
>
> I think I should:
> 1. Create a StarComponentSkeleton (or whatever) class which has all
> the methods that are common to StarComponent and DeepStarComponent
> and let the latter two subclass from the former.

Removing the dynamic star code from StarComponent (as suggested above) 
would remove the need for doing this. 

IMO the named stars component and the dynamic stars components are 
apples and oranges so they should be totally separate classes.  The
different dynamic meshes only differ in the size of the StarData record
so they should be subclasses (or the same class with function pointers).

This will also make it trivial to migrate the fainter stars to 32-bytes.


> 2. Create a StarBlockListSkeleton (or whatever) class which has a
> virtual fillToMag and let StarBlockList and DeepStarBlockList
> subclass from it.

This subclassing makes lots of sense.  But I've run into problems with 
relying on polymorphism to change behavior. You might run into a 
problem when you try to keep different subclasses in the same QList.  
One solution is to do it the "old fashioned" way and use a function 
pointer instead of subclassing.  But subclassing is preferred if you 
can get it to do what you want.

Another thing to consider is moving the code that reads in the star
data from StarBlock to DeepStarComponent.  Sometimes moving the code up 
one level like this makes things cleaner.  That way the choice of which 
filereading routine to use is made once per DeepStarComponent instead
of once per StarBlock.  But there might be much more compelling reasons
to leave the code in StarBlock and its subclasses.


> 3. Better than above, have a StarFileReader class that subclasses
> from BinFileHelper and defines a way of reading one record from the
> file and creating a StarObject out of it. This way, we could simply
> have different versions of StarFileReader, like DeepStarFileReader,
> USNOStarFileReader that override the StarFileReader::readStar() (or
> whatever) method to handle different file formats. This will give us
> enough flexibility to support USNO A or any other format (say, ones
> from Stellarium or Cartes du Ciel). Maybe we could even have a file
> that tells the program how to understand various data files.

I like the idea of making it easy to add support for other file formats,
but I think having a data file to explain the formats of the other data
files might be a bridge too far because it sounds like some significant
work that might never get used.  

> I need some suggestions here, on what to do. Let me know what changes
> I need to make before commiting this.

I think Jason is the best person to make this call.  You might want to
consider asking him to make you another branch so that you can feel free
to leave things in a state of disrepair.  It's up to you (and Jason) and
depends on your upcoming schedule.  In some ways, it is much easier to
work directly on the trunk.  I'm just trying to give you options.


-- 
Peace, James