-
Notifications
You must be signed in to change notification settings - Fork 520
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
Randomizer Auto Location tracker #1942
Randomizer Auto Location tracker #1942
Conversation
…hen entering a grotto
…1000, Thawed Zora
- Fixes Claim Check check not tracking - Fixes spoiler for Claim Check check when getting Biggoron's Sword - Fixes LACS being spoiled and not tracking
Wouldn't it be better to be it's own thing under the Randomizer Menu? With this current implementation, the user need to bring the item tracker settings, to then click a checkbox to show the Check Tracker If someone want to bring it up It would be way easier to simply make it a button like the Item Tracker and Item Tracker Settings |
- Fixes Bongo Bongo and Twinrova checks
@sonoftunk looks like quite a bit of work has been done on this since I last reviewed it, is it ready for another review or are you still adding/testing stuff? |
@briaguya-ai Mostly bug fixes and the features we talked about. I believe the only feature in those commits that we didn't discuss in the earlier comments was f936299 for some basic vanilla support. I'll do a cleanup commit for all the remaining TODOs and comments that are no longer valid. I'll ping you when I'm ready for final review, which hopefully I should have time to finish up in the next day or so. |
@sonoftunk anything i can do to help get this across the finish line? |
…d macros, adds language TODOs,
…osmeticsEditor.cpp (Breaking Change for rainbow in UI)
… needed header, and some trivial whitespace fixes.
@briaguya-ai Thanks for the offer! I've just finished up my last pass of code cleanup and bugfixing, and it is ready for review again. |
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.
overall this is looking pretty good! there are a few places where i left suggestions that could clean up the logic a little, and a few places where i have some questions
looking forward to the next pass!
void RainbowTick() { | ||
float freqHue = hue * 2 * M_PI / (360 * CVar_GetFloat("gCosmetics.RainbowSpeed", 0.6f)); | ||
for (auto cvar : rainbowCVars) { | ||
if (CVar_GetS32((cvar + "RBM").c_str(), 0) == 0) | ||
continue; | ||
|
||
Color_RGBA8 newColor; | ||
newColor.r = sin(freqHue + 0) * 127 + 128; | ||
newColor.g = sin(freqHue + (2 * M_PI / 3)) * 127 + 128; | ||
newColor.b = sin(freqHue + (4 * M_PI / 3)) * 127 + 128; | ||
newColor.a = 255; | ||
|
||
CVar_SetRGBA(cvar.c_str(), newColor); | ||
} | ||
|
||
hue++; | ||
hue %= 360; | ||
} | ||
|
||
inline void SaveColorToCVar(std::string cvarname, ImVec4 color) { | ||
CVar_SetFloat((cvarname + "R").c_str(), color.x); | ||
CVar_SetFloat((cvarname + "G").c_str(), color.y); | ||
CVar_SetFloat((cvarname + "B").c_str(), color.z); | ||
CVar_SetFloat((cvarname + "A").c_str(), color.w); | ||
SohImGui::RequestCvarSaveOnNextTick(); | ||
} | ||
inline ImVec4 GetColorFromCVar(std::string cvarname) { | ||
float R = CVar_GetFloat((cvarname + "R").c_str(), 255); | ||
float G = CVar_GetFloat((cvarname + "G").c_str(), 255); | ||
float B = CVar_GetFloat((cvarname + "B").c_str(), 255); | ||
float A = CVar_GetFloat((cvarname + "A").c_str(), 255); | ||
return ImVec4(R, G, B, A); | ||
} | ||
inline Color_RGBA8 ImVec4_to_Color_RGBA8(ImVec4& color) { | ||
Color_RGBA8 ret; | ||
ret.r = color.x * 255.0f; | ||
ret.g = color.y * 255.0f; | ||
ret.b = color.z * 255.0f; | ||
ret.a = color.w * 255.0f; | ||
return ret; | ||
} | ||
inline ImVec4 Color_RGBA8_to_ImVec4(Color_RGBA8& color) { | ||
ImVec4 ret; | ||
ret.x = color.r / 255.0f; | ||
ret.y = color.g / 255.0f; | ||
ret.z = color.b / 255.0f; | ||
ret.w = color.a; | ||
return ret; | ||
} | ||
|
||
void ImGuiDrawTwoColorPickerSection(const char* text, const char* cvarMainName, const char* cvarExtraName, | ||
Color_RGBA8& main_color, Color_RGBA8& extra_color, Color_RGBA8& main_default_color, | ||
Color_RGBA8& extra_default_color, const char* cvarHideName) { | ||
Color_RGBA8 cvarMainColor = CVar_GetRGBA(cvarMainName, main_default_color); | ||
Color_RGBA8 cvarExtraColor = CVar_GetRGBA(cvarExtraName, extra_default_color); | ||
main_color = cvarMainColor; | ||
extra_color = cvarExtraColor; | ||
|
||
if (ImGui::CollapsingHeader(text)) { | ||
if (*cvarHideName != '\0') { | ||
std::string label = cvarHideName; | ||
label += "##Hidden"; | ||
ImGui::PushID(label.c_str()); | ||
UIWidgets::EnhancementCheckbox("Hidden", cvarHideName, false, | ||
"When active, checks will hide by default when updated to this state. Can " | ||
"be overriden with the \"Show Hidden Items\" option."); | ||
ImGui::PopID(); | ||
} | ||
if (ImGui::BeginTable(text, 2, ImGuiTableFlags_BordersH | ImGuiTableFlags_BordersV | ImGuiTableFlags_Hideable)) { | ||
ImGui::TableNextRow(); | ||
ImGui::TableNextColumn(); | ||
ImGui::PushItemWidth(ImGui::GetContentRegionAvail().x); | ||
if (UIWidgets::EnhancementColor("Check", cvarMainName, Color_RGBA8_to_ImVec4(main_color), Color_RGBA8_to_ImVec4(main_default_color))) { | ||
main_color = CVar_GetRGBA(cvarMainName, main_default_color); | ||
}; | ||
ImGui::PopItemWidth(); | ||
|
||
ImGui::TableNextColumn(); | ||
ImGui::AlignTextToFramePadding(); | ||
ImGui::PushItemWidth(ImGui::GetContentRegionAvail().x); | ||
if (UIWidgets::EnhancementColor("Details", cvarExtraName, Color_RGBA8_to_ImVec4(extra_color), | ||
Color_RGBA8_to_ImVec4(extra_default_color))) { | ||
main_color = CVar_GetRGBA(cvarExtraName, extra_default_color); | ||
} | ||
ImGui::PopItemWidth(); | ||
|
||
ImGui::EndTable(); | ||
} | ||
} | ||
} |
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 feel like these exist in other places already, could this be done without duplicated code?
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 rainbow code is a modification of Cosmetic Editor's code for rainbows. With a recent refactor of it from develop, I wasn't able to use Cosmetic Editor directly any more. I can look at refactoring both Cosmetic Editor and Check Tracker to both use a new standard rainbow generator? Or take a second crack at jamming in my UI code into the Cosmetics?
The colour CVar save/load can also be refactored to just use the CVar_GetRGBA directly instead of the floats. It was only there to avoid having to rewrite the background colour/opacity.
I couldn't find a standard implementation of the colour conversion method between RGBA8 and ImVec4 that the ImGui Widgets use. I can directly inline the code if the method is a problem.
Similarly, I wasn't able to see how I could use Cosmetic Editor's grid drawing to my advantage here in the check tracker options. I'll take another look for getting it to work and/or look to see if there's any other places
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 figure worst case scenario just adding a todo and making an issue to remove the copypasta could work, but hopefully you can figure something out!
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.
Commit e9e0a36 fixes a bug in UIWidgets where alpha-enabled EnhancementColor was practically useless, and 47af3df refactors to get rid of the inline methods. Although, it doesn't seem much better. I would argue that Color_RGBA8_to_ImVec4 kept the code cleaner, and I couldn't find an alternative anywhere.
As for the other points on the Cosmetic Editor, I still don't see a way of easily getting the two code-bases to cooperate. The two colour picker cannot be replaced by DrawCosmeticGroup, and the rainbow with CosmeticsUpdateTick, without major rework two both modules.
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.
let's make a follow up issue for it, combining the codebases feels out of scope for this
as for e9e0a36, i'm guessing you mean it wasn't being used before and the incorrect logic wasn't noticed?
@briaguya-ai Thanks for the feedback cycle! I went ahead and applied code changes for the majority of your feedback. I left some conversations unresolved as I believe they will definitely need a second look to be sure they are up to snuff. |
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.
Good stuff!
I left a couple final comments, but nothing that needs to be addressed in this PR.
Let me know when you're ready and we'll !
improvements to be implemented later: #2131 |
Adds an Automatic Check Tracker, an enhancement of the Item Tracker.
Props to PR #1117 for the idea and the inspiration!
*
indicator for the last item retrieved.Known issues:
In some settings, checks will be spoiled even without checkingThese should all be caught at this point, but not guaranteed*
is flaky, disappearing and reappearing or showing on the wrong item. Always shows the wrong area on loading the save fileOnly supports Randomizer, not vanilla OoTWon't doItems that are switched with blue rupees show the item it would have beenWon't doSome checks save to the file prior to the item showing on screen, allowing a few seconds of spoiler when autosave enabledWon't DoFuture ideas:
Build Artifacts