Skip to content

Main menu button, and some scrolling#29

Merged
feldoh merged 5 commits intofeldoh:mainfrom
keyz182:ui_overhaul_#6
Aug 20, 2024
Merged

Main menu button, and some scrolling#29
feldoh merged 5 commits intofeldoh:mainfrom
keyz182:ui_overhaul_#6

Conversation

@keyz182
Copy link
Copy Markdown
Collaborator

@keyz182 keyz182 commented Aug 17, 2024

Adresses #6

<option name="commandLineOptions" value="" />
<option name="modListPath" value="$PROJECT_DIR$/modlist.xml" />
<option name="saveFilePath" value="" />
<option name="scriptName" value="C:\Program Files (x86)\Steam\steamapps\common\RimWorld\RimWorldWin64.exe" />
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'd prefer this to be picked up from an env var solely because mine isn't there lol mine's in E:\Epic.
$RIMWORLD_PATH maybe as that's what it is in the csproj?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I thought this was already.

<RimworldPath Condition=" '$(RIMWORLD_PATH)' == '' ">$(SteamCommonPath)/RimWorld/</RimworldPath>
<RimworldPath Condition=" '$(RIMWORLD_PATH)' != '' ">$(RIMWORLD_PATH)</RimworldPath>

<SteamModsPath Condition=" '$(STEAM_MODS_PATH)' == '' ">$(SteamCommonPath)/../workshop/content/294100</SteamModsPath>
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

this should have an and exists and a version without the workshop thing. The existing paths you removed ../../../../.. are the relative from mod to the local mods folder which is where you expect to find this during dev. For a non-steam copy like mine this is a breaking change because it'll no longer find my mods folder. I think the best way to work this is add a "LocalModsPath" var too which works for both steam and non-steam versions. Then the imports would have a hint path of local if exists otherwise steam because if you have a local copy you probably want that one for dev.

For reference the paths on my machine are:

  • E:\Epic\Rimworld\Mods\TotalControl\1.5\Source
  • D:\SteamLibrary\steamapps\common\RimWorld\Mods (steam only, mostly just used for release so only has my own mods in it, the steam copy doesn't have all the expansions so i couldn't and wouldn't use it for testing)
  • Steam mods D:\SteamLibrary\steamapps\workshop\content\294100 (usually empty for me or nearly so)

Amusingly your steam common path logic actually sets E:\Epic as my SteamCommonPath but that still technically works because the rimworld path is still just /Rimworld and the local mods folder is still /Rimworld/Mods

Comment on lines +77 to +81
"Huh, there's nothing here... Why not create a new preset by clicking the button below?"
);

ui.GapLine();
if (ui.ButtonText("Create new preset..."))
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

do swap out the hard coded english for translation strings if you're adding / changing things. I wouldn't expect anyone to do a full pass on all strings but lets not make the problem worse and fix it incrementally <3

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This whole class confuses me, this is damn near a copy paste of the main ModCore DoSettingsWindowContents but without the settings at the top?
a) why the lack of settings
b) why the copy pasta? if it's just being triggered from two different places, extract that into a separate method and just call it from both places?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

First pass just to get it working, and a base for cleanup.

For the settings specifically, I figured semantically they belong in the settings so would putting them here make sense? But in that vein, I guess this is all settings, so maybe it doesn't matter.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Basically, this class can be ignored for now as I intend to overhaul it. Then when it's good, I can redirect the settings to the same place.

Comment on lines +14 to +22
<SteamCommonPath Condition=" '$(STEAM_COMMON_PATH)' == '' AND Exists('D:/SteamLibrary/steamapps/common/')">D:/SteamLibrary/steamapps/common</SteamCommonPath>
<SteamCommonPath Condition=" '$(STEAM_COMMON_PATH)' == '' AND !Exists('D:/SteamLibrary/steamapps/common/')">../../../../../</SteamCommonPath>
<SteamCommonPath Condition=" '$(STEAM_COMMON_PATH)' != '' ">$(STEAM_COMMON_PATH)</SteamCommonPath>

<RimworldPath Condition=" '$(RIMWORLD_PATH)' == '' ">$(SteamCommonPath)/RimWorld/</RimworldPath>
<RimworldPath Condition=" '$(RIMWORLD_PATH)' != '' ">$(RIMWORLD_PATH)</RimworldPath>

<SteamModsPath Condition=" '$(STEAM_MODS_PATH)' == '' ">$(SteamCommonPath)/../workshop/content/294100</SteamModsPath>
<SteamModsPath Condition=" '$(STEAM_MODS_PATH)' != '' ">$(STEAM_MODS_PATH)/294100</SteamModsPath>
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

as above

{
if (optList.Any(opt => opt is ListableOption_WebLink))
{
optList.Add(new ListableOption_WebLink("FactionLoadout_SettingName".Translate(), delegate
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

idk but would it make more sense as one of the main buttons instead of one of the "web links" because it really isn't a weblink

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Not sure what makes sense here. I've seen other mods insert here. The in between mod I'm doing main buttons. There's more room in the weblinks for extras, whereas if someone has multiple mods adding to the main menu, it risks putting important buttons off screen. Maybe that's worrying too much about it though.

<ModsConfigData>
<version>1.5.4104 rev435</version>
<activeMods>
<li>brrainz.harmony</li>
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

doesn't matter in the slightest but I might be tempted to add smart speed ad plausibly dubs because they're just generally useful when testing things

@keyz182 keyz182 changed the title [WIP] UI overhaul #6 [DRAFT] UI overhaul #6 Aug 18, 2024
@keyz182 keyz182 marked this pull request as draft August 18, 2024 16:15
@keyz182
Copy link
Copy Markdown
Collaborator Author

keyz182 commented Aug 18, 2024

Converted to draft, forgot to do that when I create this.

@keyz182 keyz182 changed the title [DRAFT] UI overhaul #6 Main menu button, and some scrolling Aug 20, 2024
@keyz182 keyz182 marked this pull request as ready for review August 20, 2024 17:41
@feldoh feldoh merged commit c68ac5b into feldoh:main Aug 20, 2024
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