General Improvements to star field#5124
Conversation
|
You only need to edit en.json, so please remove all other {lang}.json files. |
src/Background.cpp
Outdated
| namespace { | ||
| static const Uint32 BG_STAR_MAX = 500000; | ||
| static const Uint32 BG_STAR_MIN = 50000; | ||
| static const Uint32 BG_STAR_MIN = 1; |
There was a problem hiding this comment.
The reason this was 50,000 was so that there are always some stars generated. With it at 1 you could literally have a starfield with just 1 star.
There was a problem hiding this comment.
maybe there's someone who wants a pitch black sky :)
|
Looks nice! |
src/Background.cpp
Outdated
|
|
||
| const double size = 1.0; | ||
| const Sint32 visibleRadius = MathUtil::mix(BG_STAR_RADIUS_MIN, Clamp(m_visibleRadiusLy, BG_STAR_RADIUS_MIN, BG_STAR_RADIUS_MAX), Pi::GetAmountBackgroundStars()); // lyrs | ||
| constexpr float starsPerSector = 2.0; /* experimentally determined (already ajusted for that we are |
There was a problem hiding this comment.
The potential sectors used are selected in a square, because that was easy, but are then rejected by distance leaving only stars within a spherical region.
There was a problem hiding this comment.
Also why pick only 2 stars per sectors when we have many more?
There was a problem hiding this comment.
The problem I encountered was that if BG_STAR_RADIUS_MAX was increased we'd run again in the issue that only half of the sky has stars. I also found that we can increase the visibleRadius far over the hard-coded 220 LY when selecting the maximum density, which I thought is nicer than filling up the rest with fake stars.
My idea was that we want to know, how big we need to make the visibleRadius to get approximately the number of stars we want (NUM_BG_STARS).
4/3 * pi * visibleRadius^3 = numSectors * sectorSize^3, i.e. I want the sum of the volume of all sectors be approximately the same as the volume of the sphere with radius visibleRadius. We can the numSectors by experimentally checking how many stars on average are in one sector. And I found that on average there are 2 stars in one sector (or rather 2 solar systems per sector). If we'd change the size of a sector this would break that's why I'm not sure if it's actually ok.
There was a problem hiding this comment.
Ah no, the fake stars are there because we never want to run the sector generator too much. It's incredibly expensive, so the populated stars are really only a shell close to the player, and anything further away is just ... padding to make it look more believable.
An extension to this idea is to eventually have the stars within jump distance be displayed on the night sky somehow, so again, that's only going to be stars close enough that the ship can jump too them.
There was a problem hiding this comment.
Oh ok, didn't think about that. What about if I cap the visible radius after calculating to BG_STAR_RADIUS_MAX or a parameter that can be changed? To be honest it takes on my notebook only half a second to generate the star field at maximum density, but this way will be easy to change it if its performance becomes more important.
Won't they then also get deleted here if this pull request would merge? |
Don't remove the entire files, just your changes to them. In other words, just edit the en.json file and leave the rest unchanged. |
tsoj
changed the title
|
@tsoj your |
|
Checked the skybox in-game, looks nice. |
@nozmajner That's how it was named before yes, I used the more descriptive name to make it easier for myself. But it doesn't really matter, if it is better I can rename it to I think it would be generally a good idea to make the skybox filename not hard coded, at least using come config file or even making it an in-game option, but I think that can be done in another pull request. |
|
I'm fine with the name, if the game picks it up. |
|
Not having read the code, but if it has a new name, the original could stay, as optional choice, or? |
|
I removed it, but I'll add it again. However, this doesn't make much sense at the moment, as the filename is hard-coded, so you would need to rename the skybox you want to use to the filename that is in the code. |
|
OK, hold off on any changes, I was just trying out the idea. |
|
Okay, I think I fixed the conflicts. |
|
@tsoj sorry this has taken so long to get reviewed and merged. I'd appreciate it if you could do before-and-after profiles of the time taken by the background setup so we can measure what kind of performance impact this has, given that you mentioned it was slower than the current code. |
|
@Web-eWorks What I could add is another option for the number of real stars that is independent of the star density, which could then be used to adjust the performance. The rest of the stars that would need to be added to reach the chosen star density would be fake stars (randomly generated). Though, I am wondering if that would be really necessary for this kind of performance penalty. I didn't do proper benchmarks (I had other programs running at the same time, and I didn't run all version of pioneer at the same time), but I think these numbers are roughly correct. For all tests, I did the same thing in-game: launched (a new game) from Mars, flew a bit around, jumped to another system and quit. By the way, I didn't manage to properly compile pioneer with cmake and make with the option to use the profiler. How is this supposed to be done? I just replace all |
Excellent, thank you! Wanted to get a "real world" example of what kind of performance difference we're talking about here; looks like it's negligible enough to not be a concern. Regarding building with profiler, |
|
Merged! Thank you for the work and the patience with the review process! |
6 participants
Star density at 25%, star size at 70%¹
Star density at 100%, star size at 70%¹
Star density at 50%, star size at 100%¹
¹ Edited: I decreased the maximum possible star size by half, before it was at 35%, 35%, 50% see 302e8b0
This skybox will of course not be realistic once you move away from space near our own solar system, but I personally think that this is no big deal, the old version wasn't better in that regard.
I made two skybox versions, one exactly like the original photo (
skybox.dds), the other without stars (skyboxNoStars.dds, default), because I thought that the stars should be generated.brightnessApparentSizeFactor(star size) is now a configurable parameter that can also be changed with a slider in the option-menubrightnessApparentSizeOffset,brightnessColorFactor,brightnessColorOffsetmade no real difference in appearancebrightnessPoweris to an extent the same asbrightnessApparentSizeFactor, but I am unsure whether it should be a configurable parameter, and if yes where? InGameConifg.cppor rather like before inStarfield.ini. Same withmedianPositionvisibleRadiusLyis now computed fully based on the selected star densityTODO: