-
-
Notifications
You must be signed in to change notification settings - Fork 375
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
Refactor and rework random system generator #5748
base: master
Are you sure you want to change the base?
Conversation
58addc4
to
dfa9676
Compare
This is good work! Glad to see someone showing this code some care and attention |
da4be00
to
1406c63
Compare
removed WIP: sign, third task would be completed through separate PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't spot any coding issues, although the ULGY HACK part could be nice to cleanup
I don't know how to make it pretty, it generates an i-th uppercase letter |
Does this change the universe generation at all? |
In what terms? |
Naming of systems, numbers of stars/planets/etc, anything like that or is it purely a code reorganisation? |
Number of planets could vary. Number of stars for each system is done in sector generator. Only change here is how star types are generated |
@Mc-Pain would you mind if I pushed a small change to your PR to cleanup the |
+, go ahead |
@fluffyfreak Wait with asking until after you've done it. (looks like mcpain gave thumbs up on your post, but you're not notified by that) EDIT: looks like me and mcpain posted at the same time |
@Web-eWorks I'm happy with this as-is but as I've (tried to) contribute to the code I'm not going to judge whether it should be merged, also with the changes it may have changed the generated universe which requires a savegame bump? |
I'd recommend holding off on merge until after 3.14, when we intend to release a bug-fix zuper-ztable Tauoneer (tau = 2 x pi, Tauoneer = Pi-Pioneer - yes, I'll keep proposing that). Players of the 3.14 release are Tauonauts. Only bug-fixes and PRs that are unlikely to introduce bugs before 3.14, and no save-bumps, according to the Board of Directors of Pioneer. |
|
Apologies on the delay officially responding here. I'll be reviewing this PR soon after the 3.14 hotfix release, as I'd prefer not to introduce a savegame bump in the upcoming release if it's not absolutely necessary. |
if body is root, parent == 0
Do not rely on syste layout, it could be random now
a365298
to
4ec5008
Compare
|
Looking at the result of system generation here - I do like that systems are much more flexible with their arrangements of binary stars, but I suspect the order of bodies generated in this system does not match what they'd be named if the system was discovered by astronomers today. We have an informal pattern of naming objects in a system according to their distance to the system center, and as you can see Qutiio A breaks that pattern (in system Qutiio (-1, 5, 0)). I suspect some of the arrangements here would be much better served by introducing the ability for stars to orbit other stars, rather than requiring they be children of a gravpoint. Additionally, I suspect it'd be quite trivial under the current model for Qutiio D to overlap with the orbits of Qutiio A or Qutiio B,C, compounding the existing issues with body orbits being generated "inside" other bodies. |
Please only read this when you are interested in getting systems who are in accordance with reality...! Web-eWorks, you are wrong... Stars and exoplanets get their names by the order they have been discovered. So, in general, the brightest component gets the letter A, and the brightest star is the most massive one and therefore closest to the gravitational center of the system. The same for exoplanets: if a gas giant is discovered first, it gets the letter b (not a = star itself) and keeps this letter even when there are more planets detected inside its orbit. I don't know how pioneer generates systems, gravpoints are a good help if you have only two components, but with more, this won't work (unless you get a quasi-stable system by accident). Imagine all stars in this Quitio system move to one side - where is the gravpoint? Where the gravitational center? For save system generation, you need
Video by Harald Lesch (german physician and philosopher, auto-generated english subtitles available) |
I'd argue that from a cartographic, navigation perspective, an inside-out naming order is better. Like if there's a new street, you won't number houses in the order they were built. |
This is of course all right for systems completely generated by a computer or human imagination - and without real or fictional discovery history. |
Indeed, I should have specified that I was speaking in the context of the nomenclature that would be used for a system that had been discovered in its entirety in an instantaneous fashion, rather than a system in which bodies had been discovered piecemeal over tens or even several hundred years.
While this is true in real life, every single system you visit in Pioneer has planets and moons named and ordered by their distance to the primary. Pioneer does not currently attempt to generate any kind of history for random systems, partially because no one's implemented it yet, and partially because the runtime performance for system generation is a primary concern of the system.
...Which is what I was attempting to say originally. The first star generated in a system is always the most massive one. Thus it should be closest to the center. However, the code in question can - as evidenced by the screenshot - place the first, most massive star significantly farther from the combined gravitational center than the remaining successive bodies.
This is a good point. I retract my comment regarding the pair-of-binary-pairs - it may produce implausible looking systems to the average player's eye but is a decent enough arrangement with regards to the orbits of the associated bodies. |
- Ensure eccentricity and semi-major axis values are properly written back to individual bodies (cherry picked from commit 2de6265)
Current implementation allows up to four stars in the system with only layout of "two binary systems orbiting each other"