Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Load stars from the HYG Star Database #4879

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

sturnclaw
Copy link
Member

This PR closes #4045 - with many thanks to @ecraven for the initial implementation in Scheme!

This is a long PR with many changes along the way, considering it's been in development for half a year now (cough due to my own laziness cough). I'll summarize quickly here, but the commit descriptions are where the meat of the PR is:

  • Moved some functionality to the core/ module (compression, IniConfig, etc.)
  • Added external libraries to contrib/: csv-parser to simplify reading the HYG database, nonstd::string_view for an easier way to pass around strings without copying them, argh to replace our ad-hoc and ugly command line parsing
  • Added some more performance information and restructured the way it's displayed
  • Made the performance info widget available in the main menu
  • Moved the modelcompiler run step into its own target - make -C build build-data invokes this as well as the HYG database fetch and parse steps

A couple of notes:

  • Fetching the HYG database requires a curl instance to be available on the command line (sorry Windows users!)
  • About 1200 stars from the DB aren't imported into the game; this is probably an oversight on my part, if someone wants to add logic to bind the remaining spectral classes to Pioneer's star types, feel free!
  • There's tons of data in the DB that we're not using; the absolute magnitude and star color temperature are two that I've added to the parser but don't have anything in-game to hook them up to.
  • The new modelcompiler step isn't currently run in the build-travis.sh script (easy to fix).
  • It's not really feasible to remove 02_local_stars.lua, as you wind up with an empty bubble about ~80ly around Sol. Whether this is intended behavior is up for discussion (as well as whether the galaxy should be a lot less dense), but for now it's staying.
  • System name generation is really bad. I did some cursory searching and within about 200ly we have 10+ duplicate systems all named "Solia". That's something to improve on next time though...

@impaktor
Copy link
Member

Quick first note: first commit aught to also add an entry to AUTHORS due to the csv import lib.

Last issue is already tracked in #4866

Will test later.

@sturnclaw
Copy link
Member Author

Thanks for catching that, I didn't even notice that I forgot to add entries to AUTHORS for csv-parser and string_view_lite!

@sturnclaw
Copy link
Member Author

Because I'm my own worst enemy, I pushed the logging changes to this branch as well. I'll split that off if people don't want to review both of them at the same time...

@impaktor
Copy link
Member

Have compiled, confirm that custom systems went from 500 100,000 in debug info, after make -C build build-data 1. (that needs to be documented in COMPILING), 2. Confed faction has moved so that needs fixing. i.e. now only the 3(?) custom CIW worlds are left, in a sea of Federation systems, + swarm of CIW systems very far away from CIW homeworld.

Nice progress!

@impaktor
Copy link
Member

I don't think we need to support procedural assignment of systems to factions. We decided some time ago to not have procedural factions, at which point the code generating them was removed, but only after its output had been dumped as +100 faction files in data/factions.

This step was the first taken in the attempt to reduce factions from hundreads, down to only three: SolFed, CIW, and Haber, (plus "independent"). The idea is that factions should be hand written, with lore, illegal-goods rules, positions, wiki, ship manufacturers, etc.

In summary: I wouldn't mind if each faction explicitly listed which systems (or sectors) it controlled, and then that's what's used in sector view. Either way, currently with this PR it sort of breaks CIW. I'll hold off on further review until some dicision / action on how to fix that has been taken.

@sturnclaw
Copy link
Member Author

@impaktor as far as I know, nothing in this PR has touched the faction code; if loading custom systems is causing issues with faction alignment, the best solution I see is to temporarily disable the build-hyg-data step until the faction code can be brought up to spec.

While it's certainly an issue that the factions are not showing up as intended, I think that's out of scope for this PR.

@impaktor
Copy link
Member

Also: starting positions are messed up. See for instance New Hope (in orbit), or Bernard Star (crashes the game)

sturnclaw added 7 commits May 15, 2020 16:40
Use csv-parser to parse the HYG database in an efficient manner
All of the parsing code is implemented except Bayer-Flamsteed names
Still need to arrange writing JSON / CBOR data and applying compression

Implement everything but LZ4 compression

Friendly reminder that CBOR is network byte order encoded, AKA big-endian
Validation, parsing, JSON and CBOR all working
Only thing remaining is to handle LZ4 de/compression

Split cbor writing to its own header
Finish database parser, generate long B-F names
- Don't divide parsecs by 3.5, multiply them instead
- Better logging and error catching for custom system loading
- Warn when overwriting a custom system; need better method to manually override stars
- Log memory used by custom systems
@sturnclaw
Copy link
Member Author

Sigh... Looks like this needs some more work then. I'll split the QOL changes into its own PR and this can bitrot for a bit while I work on something else.

@sturnclaw sturnclaw marked this pull request as draft May 15, 2020 20:50
@sturnclaw sturnclaw force-pushed the hyg-database-stars branch from 0441d9e to 2208638 Compare May 15, 2020 21:52
@impaktor
Copy link
Member

impaktor commented May 16, 2020

Sorry if I've come off as negative, I really like moving to using the HYG* star catalogue, so I really appreciate the work you've done. I agree that merging the code - but leaving it inactive - could be a good idea, to prevent bitrot.

As a side note, from the forum thread on using star catalogue, @Brianetta wrote:

I've processed star catalogues into custom systems before, and the one big thing that kept tripping devs up back then was the fact that star catalogues contain stars, not systems. There's a lot of work to be done on a star catalogue to combine binary, ternary and so forth systems from their component stars in the catalogue.

*EDIT: just reminding myself that HYG=Hipparcos Catalog + Yale Bright Star Catalog (5th Edition), + Gliese Catalog of Nearby Stars (3rd Edition)

@Brianetta
Copy link
Contributor

I stand by every word. Alpha Centaurus is s great example; Toliman (as was; the stars' names have been moved around IRL since the Pioneer project began) is at such a threshold distance from Proxima that it is unclear whether to call it a separate system. In the galactic core distances of that order are the norm.

Incidentally, I'm no sunk cost fallacist. If something is better or more fun, use it, even if what it replaced cost a lot of time and effort.

@sturnclaw
Copy link
Member Author

Yeah, this PR was mostly an effort to finish up ecraven's work and get it going; I'm going to take a break from this and work on another section of the code instead while I figure out the best way to handle it.

From what I can see, there are a few major tasks that need to be done before this can be merged:

  • Process star coordinates to match Pioneer's coordinate system
  • Merge close (how close?) stars into binary / trinary stars
  • Add an override / modify mode to CustomSystem to allow easier editing of imported stars
  • Add a "star blacklist" to the parser / importer to filter out specific cases of incorrect stars

#include "Galaxy.h"
#include "Json.h"
#include "LZ4Format.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line causes me a compilation error

@impaktor impaktor force-pushed the master branch 2 times, most recently from 876b6fc to b5bc47c Compare July 6, 2022 13:11
@impaktor impaktor force-pushed the master branch 2 times, most recently from 90e4530 to 6a3a044 Compare January 26, 2023 21:29
@impaktor
Copy link
Member

impaktor commented Aug 1, 2024

Probably not of interest, but I imagine this has (pytong) code for parsing HYG data base (although projecting to 2D sky, I assume),
https://github.com/eleanorlutz/worldstars_atlas_of_space

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants