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.

Sign up for GitHub

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

Jump to bottom

you found greg! #2458

Merged
merged 8 commits into from
Feb 17, 2023
Merged

you found greg! #2458

merged 8 commits into from
Feb 17, 2023

Conversation

briaguya-ai
Copy link
Contributor

@briaguya-ai briaguya-ai commented Feb 13, 2023 •
edited by github-actions bot

@garrettjoecox
Copy link
Contributor

So the main reason I backed off doing this earlier on was it gets complicated with MQ dungeons, as 3 more "GREG"'s are added to the item pool, any thoughts on this?

@briaguya-ai
Copy link
Contributor Author

@garrettjoecox i accounted for that already, only the one in mido's is greg, the others are still green rupees

@briaguya-ai
Copy link
Contributor Author

https://github.com/briaguya-ai/Shipwright/blob/01688b47bfcf32235201637491f43bcbf8ed99f3/soh/soh/Enhancements/randomizer/3drando/item_pool.cpp#L275-L281

@briaguya-ai briaguya-ai mentioned this pull request Feb 13, 2023
Merged
@briaguya-ai
Copy link
Contributor Author

example spoiler that shows greg as different from the other green rupees (12 mq dungeons)
35-18-78-23-21.json.txt

"DMC Volcano Freestanding PoH": "Green Rupee (Treasure Chest Minigame)",
"HF Deku Scrub Grotto": "Green Rupee (Treasure Chest Minigame)",
"Shadow Temple MQ After Wind Hidden Chest": "Green Rupee",
"Gerudo Training Grounds MQ Second Iron Knuckle Chest": "Greg Rupee",

@briaguya-ai
Copy link
Contributor Author

@PurpleHato as of 74d9e45 all of your suggested options should work

image

image

image

image

garrettjoecox
garrettjoecox reviewed Feb 17, 2023
View reviewed changes
soh/soh/Enhancements/debugger/debugSaveEditor.cpp
@@ -1783,6 +1787,13 @@ void InitSaveEditor() {
SohImGui::LoadResource(entry.second.name, entry.second.texturePath);
SohImGui::LoadResource(entry.second.nameFaded, entry.second.texturePath, ImVec4(1, 1, 1, 0.3f));
}
for (const auto& entry : gregMapping) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need a map/loop here? We’re just loading in the two textures for Greg right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think its good form to keep it as the others -- it leaves doors open

Copy link
Contributor Author

Choose a reason for hiding this comment

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

specifically it makes it easier to use the same patterns as the other groups when adding to the main window, instead of needing custom code just to add greg in there

garrettjoecox
garrettjoecox reviewed Feb 17, 2023
View reviewed changes
Copy link
Contributor

@garrettjoecox garrettjoecox left a comment

Choose a reason for hiding this comment

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

I’m on mobile and can’t closely review the item tracker changes but everything mostly looks good.

I think one remaining thing missing is you aren’t persisting the foundGreg to the save in the save manager

garrettjoecox
garrettjoecox approved these changes Feb 17, 2023
View reviewed changes
Copy link
Contributor

@garrettjoecox garrettjoecox left a comment

Choose a reason for hiding this comment

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

LGTM!

@briaguya-ai briaguya-ai merged commit 3d8752b into HarbourMasters:develop Feb 17, 2023
@briaguya-ai briaguya-ai deleted the greg-tracker branch March 1, 2023 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcvz dcvz dcvz approved these changes

@garrettjoecox garrettjoecox garrettjoecox approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@briaguya-ai @garrettjoecox @dcvz