Code review comment for lp:~alexwolf/stellarium/pulsars

Revision history for this message
Matthew Gates (matthew-porpoisehead) wrote :

0. Overall, nice clean plugin - I like it a lot! Good job Alex.

Specific points:

1. You might want to add a ChangeLog file in the plugins/Pulsars directory, but it's up to you. I do not consider it a requirement.

2. The Perl program plugins/Pulsars/util/tsv2json.pl, which processes the raw data into the catalog.json file could do with being written to more gracefully omit the comments and blank lines in the file - right now it just ignores the top 31 lines of the file, which might prove unreliable if the comments in the source data change. Personally, I would read the file from the top, with a skip flag set to 1 by default, and then un-set this flag when the ------- line is seen.

3. I changed the plugin description a little for English language readability.

4. I'm not sure about how it works, but I feel that is is prudent to empty the psa list before the pulsar texture is deleted, else there can be ::draw calls which happen between deinit and the destructor which will try to use the texture.

5. Some of the files don't have a newline on the last non-blank line of the file. I added this - it can cause problems with some build systems.

I made some changes which are attached in the form of a patch - mainly to the Perl utility program.

http://porpoisehead.net/misc/pulsars_20120212_0.patch

review: Approve

« Back to merge proposal