Closed internetisaiah closed 7 months ago
Hey @CHollingworth ,
Made some changes here, although I'm not 100% sure how to fix the issues with the PR. The CONTRIBUTING.md doc says to prepend "Fix:" to PR titles, however when I did that, it didn't seem to fix my issue?
Anyways, let me know if you have any questions!
Fix will need to be lower case, ill modify contributing now
Fix will need to be lower case, ill modify contributing now
Great. Checks passed now. ✅
Also, let me know how you feel about migrating the wiki to the docs
directory instead. If so, I can download the wiki as a zip and create a quick PR that just moves everything over as is.
I can update the links on the README
and CONTRIBUTING.md
to point to this new location as well.
This would allow people to submit edits directly to the docs rather than you copy+pasting each time.
Just let me know either way!
I wouldn't mind at all along as its easy to navigate
On Fri, 29 Dec 2023, 22:24 isaiah robinson, @.***> wrote:
Also, let me know how you feel about migrating the wiki to the docs directory instead. If so, I can download the wiki as a zip and create a quick PR that just moves everything over as is.
I can update the links on the README and CONTRIBUTING.md to point to this new location as well.
This would allow people to submit edits directly to the docs rather than you copy+pasting each time.
Just let me know either way!
— Reply to this email directly, view it on GitHub https://github.com/CHollingworth/Lampray/pull/113#issuecomment-1872370489, or unsubscribe https://github.com/notifications/unsubscribe-auth/ARGLQVJBLPCT72J45OFYWRDYL47IXAVCNFSM6AAAAABBG6V3F6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZSGM3TANBYHE . You are receiving this because you were mentioned.Message ID: @.***>
@CHollingworth , cool. I'll add it to this PR tomorrow (when I have time), then you can review and merge 👍🏽 . I'll tag you when its ready!
Hey @CHollingworth : just a little update. I rewrote all the existing docs and moved them to the new docs
directory. Tomorrow I'm going to:
README.md
to point to the new docs instead of the wikidocs
for better organization, but we'll see 🤔 Anyways, once I do that, I'll tag you! But in the meantime, feel free to review my work so far for content accuracy 👍🏽
Hey @CHollingworth ,
Sorry for the delay! Here's the commit with all the changes so you can see what it'll look like for end-users on GitHub:
https://github.com/CHollingworth/Lampray/blob/755bf4db8ec05ced7080224f510ba639db28387d/README.md
This is ready for review. Feel free to request changes or merge 👍🏽
I will pop in with some of my thoughts, starting with README.md. I will try to get to other files in additional comments as I go through the files.
p7zip-full
or an equivalent providing a valid 7z.so file). If we remove that from the readme, I expect we would get some people not realizing that they are missing required software..rar
files are not currently supported, just to address potential issues of users trying to use .rar mods.Instead of saying to download the latest release for the code, I would think people building from source would typically want to clone the repo. I think that you should update that text to reflect that, or I suppose we could have both:
Clone this repo:
git clone https://github.com/CHollingworth/Lampray.git
or download the source from the latest release
For building the application, I think it could be worth mentioning the setup.sh
and build.sh
scripts that are included in the repository. I typically build the project using the build.sh
script myself. For example:
Building
You can likely use the included
setup.sh
script to grab dependencies and then build the application with thebuild.sh
script. Alternatively, you can build from source by following these directions: [your directions]
I don't think the 7z stuff is relevant to building, so I would either remove that section, or at least have it link to the relevant spot in docs/customizing-lampray.md
. It may also be worth mentioning the potential 7z issue and link to that page as well in the README, as it could be a somewhat common issue/oversight for users.
Users do not need to build Lampray from source, as we provide a binary for each release, so you could probably remove that Prerequisites section. I would also suggest removing references to Build
in paths on this page, as typical users will not have built Lampray from source, and will not have files in a "Build" directory (instead, they should be looking for their Lamp_Data
directory).
I had looked at changing some wording for the 7z stuff as well in #107. I think what you have is good, I would just like to suggest maybe including this text to let users know that they likely don't need to do anything with it:
If your system has 7zip available, Lampray will usually find it and use it automatically.
So it could maybe read something like:
7z is required to use Lampray. Lampray will usually be able find the 7z.so file from your installation automatically, and will use it without your intervention. If your 7z.so utility is located in a non-standard location, you will need to manually set the path to 7z.so in Lampray. To do this, go to your Lampray path (for example
~/Lampray/Lamp_Data/Config/
) and openconfig.mdf
in a text editor. Find and replace the following line with the path to 7z.so on your system.<bit7zLibaryLocation>/usr/lib/p7zip/7z.so</bit7zLibaryLocation>
Ideally, we should either list the dependencies again for the "Lamp will not open" section, or link to the page listing dependencies users need to run Lampray
For the "Pressing Deploy does nothing and mods aren't installed" section, you should update the link talking about 7z to the relevant section on docs/customizing-lampray.md
.
I think this seems fine for now.
Again, I would remove references to the Build
directory.
I think this:
Setting the path to Steam
Should be changed to:
Setting the your game paths
Then, instead of "At the top of the window, select Lampray > Steam Directory", it should probably read something like:
At the top of the window, select the game you want to mod from the "Game Selection" option, then open that top menu again and select the "Game Configuration" option. Set the paths listed on this page to the correct directories. For Baldurs Gate 3, you need to set both the Steam directory and the AppData directory.
Under "Adding mods", we support both .zip
and .7zip
, so please add .7zip
to your note.
We should probably have a section regarding deployments, which should mention that users need to deploy to apply any enabling or disabling of mods (or added to another section if it makes more sense, the point mentioning that you need to deploy after disabling mods to actually remove them):
Deploying
When you have enabled or disabled mods, you will need to go through the deploy process again to apply the changes.
What you have in this section applies specifically to Baldurs Gate 3, so that should probably be made clear somehow. Since each game could have its own set of mod types, we may want to change this page to be a general overview of the "mod type" concept, and then link to separate pages for each game we feel the need to mention. For example, we might want to move this Baldurs Gate mod type stuff to something like: docs/about-lampray/mod-types/bg3.md
or docs/about-lampray/mod-types-bg3.md
(not sure what makes the most sense for this), and have links to each of those files on this page.
For reference, right now Cyberpunk has, effectively, 1 mod type that is automatically selected. In this case, the user does not even need to select the mod type, and mods should install just fine. Right now, if they saw this section, they would likely be a bit confused and be looking for mod types that only show up for BG3
I think this should basically cover all the page and my main thought and issues right now.
Looks great, A few corrections im going to push through as ive noticed a few things that are incorrect for the current versions
Hey @SnazzyPanda ,
Thanks for the incredibly thorough review! Here are my overall thoughts:
I'm definitely fine with implementing all of your feedback overtime, but to avoid this PR from becoming a giant docs project, I think it would be best to break your suggestions down into smaller/more manageable PRs. Here's what I'm thinking:
Build
directory (in managing mods and others).Mod Types in that list are currently BG3 Exclusive, Other games wont have the same types, for example cyberpunk which only has one mod type.
Mod Types in that list are currently BG3 Exclusive, Other games wont have the same types, for example cyberpunk which only has one mod type.
Ok I'll make a quick fix then, to do this instead:
Directory:
docs
- mod-types
- - bg3.md
I think this organization/naming convention will be more scalable if in the future Lampray supports 10+ games each with their own mod types. This will prevent the main directory from getting too busy.
I'll tackle this in a sec 👍🏽
Hey @SnazzyPanda and @CHollingworth ,
Hope you're doing well today! I just pushed a commit with all the requested changes from the reviews.
Let me know if you have any questions or would like further changes. If you do, may I ask that you leave an in-line comment on the specific file/line in Files Changed, so its easier to keep track and keep discussions organized? Thanks!
EDIT:
A future PR will need to contain the following changes:
Just a couple minor tweaks. I think you have my main concerns addressed now.
I do think it would probably be good to separate the build dependencies from the run dependencies in the readme, but I am not sure how best to handle that at this time (do we want to try to give package names, install instructions, etc?), so that can likely be handled outside of this PR.
@SnazzyPanda I just pushed a commit with your requested changes. Thanks for leaving the inline comments, that was very helpful!
Please let me know if there's anything else you need from me 👍🏽
Hey @SnazzyPanda and @CHollingworth , just following up here to see if we can get this PR merged today or tomorrow. I'm gonna be out of town for a week or so and would love to get this wrapped up before then.
Let me know if there's anything you need from me!
I'll read through and merge tonight if I can.
On Wed, 17 Jan 2024, 21:08 isaiah robinson, @.***> wrote:
Hey @SnazzyPanda https://github.com/SnazzyPanda and @CHollingworth https://github.com/CHollingworth , just following up here to see if we can get this PR merged today or tomorrow. I'm gonna be out of town for a week or so and would love to get this wrapped up before then.
Let me know if there's anything you need from me!
— Reply to this email directly, view it on GitHub https://github.com/CHollingworth/Lampray/pull/113#issuecomment-1896712349, or unsubscribe https://github.com/notifications/unsubscribe-auth/ARGLQVKMPIEZQQ3LV6SMYCTYPA4WNAVCNFSM6AAAAABBG6V3F6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJWG4YTEMZUHE . You are receiving this because you were mentioned.Message ID: @.***>
Since you're currently storing docs using the repo Wiki, feel free to simply copy+paste my edits and dismiss this PR.
Changes
docs
from the.gitignore
so I could store my files there.docs/building.md
which is an update to the existing building doc. This includes: