-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat: functional build for darwin #61
Conversation
README.md
Outdated
The app needs to know where the game is installed. On Steam, this will be | ||
|
||
C:\Program Files (x86)\Steam\steamapps\common\Satisfactory | ||
|
||
When running on Windows, it should auto-detect this path and you have nothing to worry about. | ||
|
||
But this won't be the case When running on MacOS with Satisfactory installed in Crossover. | ||
In this case you have to add the installation manually. | ||
The path to a crossover-installed game, assuming a bottle named "Steam" will be | ||
|
||
~/Library/Application\ Support/CrossOver/Bottles/Steam/drive_c/Program\ Files\ \(x86\)/Steam/steamapps/common/Satisfactory | ||
|
||
The command to add an installation is then: |
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.
The app needs to know where the game is installed. On Steam, this will be | |
C:\Program Files (x86)\Steam\steamapps\common\Satisfactory | |
When running on Windows, it should auto-detect this path and you have nothing to worry about. | |
But this won't be the case When running on MacOS with Satisfactory installed in Crossover. | |
In this case you have to add the installation manually. | |
The path to a crossover-installed game, assuming a bottle named "Steam" will be | |
~/Library/Application\ Support/CrossOver/Bottles/Steam/drive_c/Program\ Files\ \(x86\)/Steam/steamapps/common/Satisfactory | |
The command to add an installation is then: | |
The app needs to know where the game is installed to function. | |
If you're not sure where that is, see the [Satisfactory Modding FAQ section about locating your game install](https://docs.ficsit.app/satisfactory-modding/latest/faq.html#Files_GameInstall) to learn approaches for finding out. | |
To add an installation, either use the interactive interface, or use the command: |
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.
That FAQ DOES NOT HELP on MacOS. It still only shows the path in Windows-format as it appears inside the bottle, it still forces people to manually translate that path to the path on the MacOS filesystem containing that bottle, and it doesn't give a quick-and-easy example of the default path that 99% of users will see.
If you want to reject this PR on that basis then so be it, but I'm not prepared to accept a proposed change that WILL make this tool harder to use on MacOS.
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 don't understand why you are implying that we want to reject this PR.
I used the exact same path you used in your previous commit (6d70a5c#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R90). I have since updated it to the new path you made in another commit (2a45c6a#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R93) after I made the original FAQ change.
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.
Also please fix the linter issues
@@ -78,6 +78,27 @@ To launch the interactive CLI, run the executable without any arguments. | |||
|
|||
Run `ficsit help` to see a list of available commands. | |||
|
|||
## Game Installations |
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.
This readme change is out of scope of this PR, as well as should be handled differently.
Rather, docs should have a subsection specifically on ficsit-cli
just like it does for SMM: https://docs.ficsit.app/satisfactory-modding/latest/ForUsers/SatisfactoryModManager.html and the readme should reference that instead.
Please remove.
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.
Unless and until the docs have that relevant section, this README is the only place to discover that
- ficsit-cli, unlike SMM, does not auto-detect installations
- manually specifying the installation is a necessary pre-requisite to using the app
- What the correct value for that will be in 99% of cases
Given that there's no way to add an installation via the interactive menus, it actually too me longer to figure out how to make the app work than it did to make the change.
This is the "boy scout" principle, leave a codebase in the state you would like to have found it, to make life easier for those that follow. I'm not at all comfortable reverting a change that achieves that, especially when the information it contains isn't something that's realistically ever going to change.
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'm not sure where you got the idea that it's not possible to add an installation via the interactive menu? I added a video below of how to do it.
As for the documentation, then please either go the extra length and include basic default instructions for all systems, or none. The most likely audience for this specific tool are Linux users running it on a headless server, where an SMM UI is impossible to be rendered.
I agree that the readme should be more informational, as it's currently even outdated for the paths it does provide, as it's missing the Linux equivalents, but also, this project is not in a released condition, but rather a 0-semver version, which should be considered unstable and not ready for general public use.
In general your message comes across quite passive-aggressive and condescending, which I am really not a fan of.
In either case, in its current state, I am not approving this PR.
(ignore weird colors for logo, I'm messing around with my terminal)
fDewg3x.-.Imgur.mp4
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.
Honestly, it's mostly frustration.
This PR is designed specifically for MacOS/Crossover users. I had to actively track down
- the fact that I had to manually add an installation
- the default installation path for satisfactory
- whether or not I needed to use the base
Satisfactory
folder or the nestedFactoryGame
folder
I then had to translate that path from windows-format to be relative to Crossover's drive_c
folder.
This is especially relevant given that one of the core distinctions between SMM and the cli app is that the cli app doesn't auto-detect installations. You're looking at this from the perspective of folk running dedicated servers, I'm looking at it from the perspective of MacOS users running the regular game in crossover, users for whom SMM doesn't work, and this is currently the only game in town if you don't want to manually install mods.
That's all earned knowledge that I gladly chose to pay forward to future people who wanted to use this app. Requests for changes that essentially say "nope, don't tell others that, they should suffer the same burden too" don't make me feel at all comfortable.
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 think you need to realize that you are in an incredible niche. Currently macOS users consist of around 1.4% of traffic. Half even that of Linux.
None of our core dev team has an apple device to test with. The reason you had to find where the game install is, is because you are literally the first person to need it. The SMM app is plagued by the same issues, because we literally had no one to test this with.
I get that you want to make life easier for the next person (we do too, why do you think we made the platform in the first place?), but making such a prominent, yet niche definition in the readme is a bit overkill.
We are not proposing for you to discard this information completely, rather, it would be much more appropriate in the docs. That would prevent information fragmentation and ensure that if this info changes in the future, it would be kept up to date.
Superseded by #66 |
No description provided.