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

Fixes #5343 Missing info #5439

Merged
merged 2 commits into from
Dec 21, 2022
Merged

Conversation

azieba
Copy link
Contributor

@azieba azieba commented Dec 10, 2022

Fixes #5343 - missing ship cabin capacity info and space station tech level.

I have only realized that @Web-eWorks is already assigned to this issue when I was about to create the PR, sorry about that..

The change for the ship cabin capacity was quite easy but I need some feedback if it is ok as it is or some rephrasing is needed.

With tech level it was a bit more complicated. Tech level was calculated only when real SpaceStation body was created and is not know in SystemBody objects. SystemBody knows only seed that can be used to calculate the techLevel but it is rather complex operation including RNG.
The lua windows work somewhat surprising to me, their contents are recalculated each time frame. So recalculating tech level of selected body each frame did not seem to be a good idea. So I added a state for the info window with data to display updated only if the selected body changes.

I also added a cmake target for calling scan_enums.py with update in data/meta/Constants.lua after running it.

And one more interesting thing. SolFed homeworld path was pointing to Shanghai instead of Earth. Was it intentional? I hope I do not offend any Chinese people with this correction.

@impaktor
Copy link
Member

And one more interesting thing. SolFed homeworld path was pointing to Shanghai instead of Earth. Was it intentional? I hope I do not offend any Chinese people with this correction.

Didn't @fluffyfreak move the SolFed home world to Mars? Or that was just the starting location, now that I think of it. Anyway, I'm not sure what the "pioneer lore" says on this, as long as it is consistent (e.g. either change wiki or change code). I don't think that that part of the lore is used in any missions / flavors.

@azieba
Copy link
Contributor Author

azieba commented Dec 10, 2022

Didn't @fluffyfreak move the SolFed home world to Mars?

The body index was set to be 4 like maybe intended for Mars, but Mars has much higher index like 16 or something like that.

EDIT
"After almost 25 years of chaos a new governmental body finally emerges from the anarchy and slowly assumes control of Mars, and then the inner Sol system."

Hm, maybe it should be Mars.

Copy link
Member

@Web-eWorks Web-eWorks left a comment

Choose a reason for hiding this comment

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

I have only realized that @Web-eWorks is already assigned to this issue when I was about to create the PR, sorry about that..

No worries, I'm assigned to a few different issues but rarely have the time to work on all of them. This was a good one to take over!

The lua windows work somewhat surprising to me, their contents are recalculated each time frame.

It's not immediately visible in the development resources we have, but pigui is an immediate-mode UI so it is indeed redrawn and recalculated each frame. It's also why there are a number of places in the code where we do a lot of manual size calculation etc. because it doesn't use a multi-pass architecture over predefined primitives to perform "automatic" layouts.

And one more interesting thing. SolFed homeworld path was pointing to Shanghai instead of Earth. Was it intentional? I hope I do not offend any Chinese people with this correction.

The homeworld is intended to point to a planet, not a starport so this was an accident. As @impaktor mentioned, that was probably intended to be Mars and not Shanghai.

Apologies on the long delay on reviewing your other PRs, I will hopefully be able to test and merge all of them (including this one) in the next few days. I've left one change request here that I'd like to see addressed before merge, and pending testing I'm otherwise happy with the code.

data/pigui/modules/system-view-ui.lua Show resolved Hide resolved
@azieba
Copy link
Contributor Author

azieba commented Dec 11, 2022

The homeworld is intended to point to a planet, not a starport so this was an accident. As @impaktor mentioned, that was probably intended to be Mars and not Shanghai.

I have corrected it to be Mars now.

@azieba
Copy link
Contributor Author

azieba commented Dec 20, 2022

@Web-eWorks I think I have implemented the requested change. Is there anything else?

@bszlrd
Copy link
Contributor

bszlrd commented Dec 20, 2022

An important note, so we don't forget: @azieba should be included in contributors.txt before the new release is out. (if not already)
How would you like to be in there? Github handle, full name, other nickname for example?

@azieba
Copy link
Contributor Author

azieba commented Dec 20, 2022

An important note, so we don't forget: @azieba should be included in contributors.txt before the new release is out. (if not already)

I'm already in the AUTHORS.txt, but thnx again 😃

@Web-eWorks
Copy link
Member

Thanks for the ping @azieba - I've had a very difficult week last week and unfortunately didn't get a chance to review your outstanding PRs. Most of your currently open PRs are ready to be merged in my eyes, but I haven't gotten a chance to clear my working tree from the Scout Mission code and test them recently.

This PR in particular I want to personally test before approving/merging, but will attempt to merge other outstanding PRs if I don't get enough time to do that tonight.

@Web-eWorks Web-eWorks merged commit abce979 into pioneerspacesim:master Dec 21, 2022
@azieba azieba deleted the missing_info branch December 21, 2022 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing info
4 participants