-
Notifications
You must be signed in to change notification settings - Fork 300
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
ACHIEVEMENTS: Support achievements with no matching Steam achievement #1930
Conversation
# Make background transparent and replace green with black | ||
# The icons will be recolored by css filters matching the player's theme | ||
# MacOS uses FreeBSD-style sed instead of GNU sed | ||
sed -i '' "s/fill:#000000;/fill-opacity: 0%;/g" "$i" |
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.
Please check the suggestion from d0sboots at https://github.com/bitburner-official/bitburner-src/pull/1508/files/562479c5cc117709126b7c01e04ecfb6d2545cb7#r1689124659.
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 forgot to remove this file.
title: string; | ||
achievements: { achievement: Achievement }[]; | ||
allAchievements?: { achievement: Achievement }[]; | ||
pad?: boolean; |
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.
Optional: Use another suggestive name for this variable. For example: needPadding
or usePadding
.
// The 264px minWidth feels scuffed, but fixes an unknown edge case (Brought up in PR #1508). | ||
return ( | ||
<Accordion defaultExpanded={!!allAchievements} disableGutters square sx={{ minWidth: "264px" }}> |
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 264px minWidth feels scuffed, but fixes an unknown edge case (Brought up in PR #1508). | |
return ( | |
<Accordion defaultExpanded={!!allAchievements} disableGutters square sx={{ minWidth: "264px" }}> | |
/** | |
* For each achievement, we need to display the icon and the detail on the same "row" (icon on the left and detail on | |
* the right). When the viewport is too small, the detail part of some achievements is "moved" to a separate "row". It | |
* looks like this: | |
<achievement 1> | |
<icon><detail> | |
</achievement 1> | |
<achievement 2> | |
<icon> | |
<detail> | |
</achievement 2> | |
<achievement 3> | |
<icon><detail> | |
</achievement 3> | |
* Using "minWidth" fixes this issue by setting a min value for the width of each row. | |
*/ | |
return ( | |
<Accordion defaultExpanded={!!allAchievements} disableGutters square sx={{ minWidth: "645px" }}> |
I guess that the edge case is what I wrote in the suggested change. Note that using 264px is not enough to fix this issue. Using 645px should be good enough.
Before:
After:
There are other CSS tricks, but it's unnecessary for this PR. Maybe I'll use them in another PR. It's best to keep this one simple.
root: { | ||
width: 50, | ||
padding: theme.spacing(2), |
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 is not your change, so you can ignore my comment here.
If you don't mind, please test this change and take some screenshots. I'm curious about why we need this change.
src/Achievements/README.md
Outdated
- Run `node ./tools/fetch-steam-achievements-data DEVKEYHERE` | ||
- Get your key here: https://steamcommunity.com/dev/apikey | ||
- If making a Steam achievement, create the achievement in Steam Dev Portal | ||
- Run `sh ./assets/Steam/achievements/pack-for-web.sh`, or `pack-for-web-mac.sh` for 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.
After applying the suggestion from d0sboots to pack-for-web.sh
, there is no need for pack-for-web-mac.sh
.
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.
+1, revert this particular line
Got it, I'll add these in a few hours when I get time |
# Make background transparent and replace green with black | ||
# The icons will be recolored by css filters matching the player's theme | ||
# MacOS uses FreeBSD-style sed instead of GNU sed | ||
sed -i '' "s/fill:#000000;/fill-opacity: 0%;/g" "$i" |
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 forgot to remove this file.
sed -i "" "s/fill:#000000;/fill-opacity: 0%;/g" "$i" | ||
sed -i "" "s/fill:#00ff00;/fill:#000000;/g" "$i" |
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.
It's important that there be no space between -i
and the "".
sed -i "" "s/fill:#000000;/fill-opacity: 0%;/g" "$i" | |
sed -i "" "s/fill:#00ff00;/fill:#000000;/g" "$i" | |
sed -i"" "s/fill:#000000;/fill-opacity: 0%;/g" "$i" | |
sed -i"" "s/fill:#00ff00;/fill:#000000;/g" "$i" |
@@ -1,6 +1,5 @@ | |||
{ | |||
"note": "***** Generated from a script, overwritten by steam achievements data *****", | |||
"fetchedOn": 1641517584274, | |||
"note": "Originally generated by a script using Steam achievement data. Going forward, must be edited manually.", |
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.
Were you able to run this script? I don't think they've changed on Steam since this PR was originally made, but I'm not 100% sure.
src/Achievements/README.md
Outdated
- Run `node ./tools/fetch-steam-achievements-data DEVKEYHERE` | ||
- Get your key here: https://steamcommunity.com/dev/apikey | ||
- If making a Steam achievement, create the achievement in Steam Dev Portal | ||
- Run `sh ./assets/Steam/achievements/pack-for-web.sh`, or `pack-for-web-mac.sh` for 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.
+1, revert this particular line
This looks good now, but there's a not-completely-trivial merge conflict I'm gonna let you handle. |
Bitburner achievements have been limited by Steam's cap of 100 achievements for games with limited profile features, but we can add more achievements internally. This change makes a few changes to allow the creation of new Bitburner achievements with no equivalent Steam achievement, including adding an optional NotInSteam boolean to the Achievement interface, and updates the Achievements page with new UI to display this to players. I tested the changes by temporarily setting NotInSteam to true for several existing achievements, and finally adding a new achievement (all in a different branch not in this pull request, achievements-test in my fork). The achievement README, which describes how to add a new achievement, was updated to (hopefully) explain how to do so going forward.
The achievements still properly save and load on both the web and electron versions, and connecting to Steam did not cause any issues.
NOTE: This is a continuation of #1508 due to the author abandoning the PR.
Screenshots of UI changes:



(note that the branch actually being merged would still show, e.g.,
Acquired (1/98, 1/98 for Steam)
)