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

Benjee10_SharedAssets RO Configs #2966

Merged
merged 7 commits into from
Jun 19, 2024

Conversation

SkyPhoenix999
Copy link
Contributor

Used in multiple mods and needs to be standardized as such.

Covers the CBM, NDS/APAS/IDA, SSPV, and AJ10

Used in (almost) all Benjee10 Mods such as HabTech2, ACK, SOCK, MSSEV, etc

SkyPhoenix999 and others added 4 commits May 29, 2024 08:54
It's about time these got put in. Used in multiple mods and needs to be standardized as such.

Covers the CBM, NDS/APAS/IDA, SSPV, and AJ10

Used in (almost) all Benjee10 Mods such as HabTech2, ACK, SOCK, MSSEV, etc
@Capkirk123
Copy link
Member

Everything should now be scaled to the correct size. However, the scale for these parts is being set elsewhere in the mod configs (notably in the Habtech 2 configs), which means the settings from these configs are not being applied. Configs other than this one should probably be removed since these parts live in Benjee10 shared assets

@SkyPhoenix999
Copy link
Contributor Author

This line is what overrides the rescale factor in Benjee's other mods, idk why he put two separate rescaleFactors in some of his parts but this fixes it
!rescaleFactor,* = DEL

I can verify your NDS rescale values but not your CBM values, the 1.8x values were properly sized (and additionally are properly sized for HabTech2 at that value)

The outer ring diameter is 82 inches (2.083m) not 79 inches, Per This Document, 79 inches refers to the inner diameter, this 1.6737x rescale value is far too small. Should be between 1.7437x and 1.8x. depending on if you want to scale for inner diameter or outer. I chose 1.8x for consistency with HabTech parts while being within the correct size range.

@Capkirk123
Copy link
Member

I used Freedom-era documents for reference, so it's possible that they were changed.

The issue is not Benjee10s other mods, it is other RO configs. The rescaleFactors being set in this config are being changed by other rescale factors in other RO configs. They should be removed.

@SkyPhoenix999
Copy link
Contributor Author

The ISS CBM did grow between Freedom and the ISS so that makes sense.

The only other config that is rescaling these parts is HabTech2, and those configs are currently outdated given the upcoming HT2 update.

In addition there are two new CBM parts that the HT2 configs don't cover, so it is my opinion that anything concerning the CBM parts should be moved to SharedAssets.

In the meantime, my suggestion is leaving this PR on hold or push it without the one CBM part until the official HT2 update is out and new HT2 configs are ready to support it.

@Capkirk123
Copy link
Member

Well if the Habtech 2 configs are outdated, that's all the more reason to delete them in favor of the not-outdated configs here

Copy link
Member

@Capkirk123 Capkirk123 left a comment

Choose a reason for hiding this comment

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

Removing competing configs for Benjee10 shared assets parts (namely, those in Habtech2)

@SkyPhoenix999
Copy link
Contributor Author

I have a new set of HabTech configs but I still have some B9 errors to sort out, and I might wait until the new update with the large modules comes out, I'm undecided on that.

@Capkirk123
Copy link
Member

I have decided for you

@SkyPhoenix999
Copy link
Contributor Author

One Last thing, NDS Passive Port. Good to go now.

@Capkirk123 Capkirk123 merged commit 7a37e20 into KSP-RO:master Jun 19, 2024
3 checks passed
@SkyPhoenix999 SkyPhoenix999 deleted the Benjee10SharedAssets-Configs branch June 22, 2024 14:28
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.

2 participants