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.

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

chore: break apart menu multiline strings #119

Merged
merged 3 commits into from
Oct 10, 2023
Merged

Conversation

rjwignar
Copy link
Contributor

@rjwignar rjwignar commented Oct 5, 2023

Change Summary

This fixes #87
Following the discussion in #87 , in Menu.cpp I've changed the multiline handling in tooltips that use multiple text fields (consecutive uses of ImGui::Text()).

According to the above, I identified and modified the tooltips for the below features:

  • Clear Shader Cache button
  • Clear Disk Cache button
  • Test Interval slider

In the modified tooltips, I also maintained the habit of adding an extra space at the end of the final string, which was already present in the Toggle Error Message button tooltip:

ImGui::Text(
"Hide or show the shader failure message. "
"Your installation is broken and will likely see errors in game. "
"Please double check you have updated all features and that your load order is correct. "
"See CommunityShaders.log for details and check the NexusMods page or Discord server. ");
...and the Compiler Threads slider tooltip:
ImGui::Text(
"Number of threads to use to compile shaders. "
"The more threads the faster compilation will finish but may make the system unresponsive. ");

Possible Additional Changes (Pending maintainers' request)

In menu.cpp I found the following **multi-sentence, single-string tooltips ** but was unsure whether they required modification (please note that entire paragraphs - not individual sentences - are wrapped in double quotes):

  • The Enable Async checkbox tooltip (line 407):

    if (ImGui::Checkbox("Enable Async", &useAsync)) {
    shaderCache.SetAsync(useAsync);
    }
    if (ImGui::IsItemHovered()) {
    ImGui::BeginTooltip();
    ImGui::PushTextWrapPos(ImGui::GetFontSize() * 35.0f);
    ImGui::Text("Skips a shader being replaced if it hasn't been compiled yet. Also makes compilation blazingly fast!");
    ImGui::PopTextWrapPos();
    ImGui::EndTooltip();
    }

  • The Dump Shaders checkbox tooltip (line 466):

    if (ImGui::Checkbox("Dump Shaders", &useDump)) {
    shaderCache.SetDump(useDump);
    }
    if (ImGui::IsItemHovered()) {
    ImGui::BeginTooltip();
    ImGui::PushTextWrapPos(ImGui::GetFontSize() * 35.0f);
    ImGui::Text("Dump shaders at startup. This should be used only when reversing shaders. Normal users don't need this.");
    ImGui::PopTextWrapPos();
    ImGui::EndTooltip();
    }

  • The Log Level combo box tooltip (line 488):

    if (ImGui::Combo("Log Level", &item_current, items, IM_ARRAYSIZE(items))) {
    ImGui::SameLine();
    State::GetSingleton()->SetLogLevel(static_cast<spdlog::level::level_enum>(item_current));
    }
    if (ImGui::IsItemHovered()) {
    ImGui::BeginTooltip();
    ImGui::PushTextWrapPos(ImGui::GetFontSize() * 35.0f);
    ImGui::Text("Log level. Trace is most verbose. Default is info.");
    ImGui::PopTextWrapPos();
    ImGui::EndTooltip();
    }

  • ...and the Shader Defines tooltip (line 500):

    if (ImGui::InputText("Shader Defines", &shaderDefines)) {
    State::GetSingleton()->SetDefines(shaderDefines);
    }
    if (ImGui::IsItemHovered()) {
    ImGui::BeginTooltip();
    ImGui::PushTextWrapPos(ImGui::GetFontSize() * 35.0f);
    ImGui::Text("Defines for Shader Compiler. Semicolon \";\" separated. Clear with space. Rebuild shaders after making change. Compute Shaders require a restart to recompile.");
    ImGui::PopTextWrapPos();
    ImGui::EndTooltip();
    }

Please let me know if the above tooltips should also be modified to match the proposed changes.


Please let me know if other changes are required.

Additionally, if this PR is approved and merged, I'd like this contribution to count towards my Hacktoberfest 2023 progress.
If the above changes are approved and merged, could the hacktoberfest-accepted label be added to the PR/MR?

Thank you for considering these changes.

@alandtse alandtse changed the title Fixed menu multiline strings Chore: Break apart menu multiline strings Oct 5, 2023
@alandtse
Copy link
Collaborator

alandtse commented Oct 5, 2023

I think it's a judgment call. Is it wrapping in the text editor or in game? If it isn't, then probably no need to fix. I can't check if this builds till the weekend, did you confirm?

@rjwignar
Copy link
Contributor Author

rjwignar commented Oct 5, 2023

I must admit I don't have Skyrim on PC (although it was my favourite game on the PS3), and haven't been able check this myself.

@alandtse
Copy link
Collaborator

alandtse commented Oct 5, 2023

Wow thanks for taking up this work then. I'll have to build it before approving, but can't see any reason it'd cause issues.

@FlayaN
Copy link
Collaborator

FlayaN commented Oct 5, 2023

If you enable workflows on your fork you can also run the .github/workflows/build.yaml workflow manually! This will create a draft github release on your fork that can be linked on the PR that can be tested by someone with skyrim 👍

@rjwignar
Copy link
Contributor Author

rjwignar commented Oct 6, 2023

If you enable workflows on your fork you can also run the .github/workflows/build.yaml workflow manually! This will create a draft github release on your fork that can be linked on the PR that can be tested by someone with skyrim 👍

Thanks for the suggestion! My experience with workflows is limited, but I think I was able to run .github/workflows/build.yaml on my fork branch from GitHub Actions:
image

The workflow run is still running over here .

Where can I find the draft release once the workflow successfully completes?

@FlayaN
Copy link
Collaborator

FlayaN commented Oct 6, 2023

Where can I find the draft release once the workflow successfully completes?

Just realized that draft releases are private... So need to think over this some. Maybe I need to remove the draft flag.

But releases are over here for future reference:

https://github.com/rjwignar/skyrim-community-shaders/releases

@rjwignar
Copy link
Contributor Author

rjwignar commented Oct 7, 2023

I recently took a look at the release.
It looks like I can Publish the draft to make it visible.
image

image

Would someone be able to test the build if I published it?
More importantly, should I even publish the draft? I'm not sure if publishing would violate any guidelines.

@FlayaN
Copy link
Collaborator

FlayaN commented Oct 7, 2023

More importantly, should I even publish the draft? I'm not sure if publishing would violate any guidelines.

It's fine to publish, it's only published to your fork anyway! Then you can link that release here, if we want to test it before merging

@alandtse
Copy link
Collaborator

alandtse commented Oct 7, 2023

Should the action publish by default?

@rjwignar
Copy link
Contributor Author

rjwignar commented Oct 7, 2023

I recently published my draft release.
It should be available at: https://github.com/rjwignar/skyrim-community-shaders/releases/tag/issue-87

@alandtse alandtse changed the title Chore: Break apart menu multiline strings chore: Break apart menu multiline strings Oct 8, 2023
Copy link
Collaborator

@alandtse alandtse left a comment

Choose a reason for hiding this comment

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

Ok this works in game. Thank you. Would you be willing to also search for ImGui::Text and handle it in the various feature code? No worries if not; I'll just have to update the initial issue to make sure it's clear it's more than menu.cpp with this issue.

for example: https://github.com/doodlum/skyrim-community-shaders/blob/dev/src/Features/ExtendedMaterials.cpp#L36-L38

@rjwignar
Copy link
Contributor Author

rjwignar commented Oct 8, 2023

I might be occupied with school commitments this month.
However, I wouldn't mind working on it throughout the month.

I have a few questions though:

  1. Should I keep ignoring multi-sentence, single-string tooltips and only handle tooltips that use consecutive text fields (consecutive ImGui::Text() calls)?
  2. Which files should I be searching through? Only those inside src?
  3. Should I put future changes in a new, separate PR?

@alandtse
Copy link
Collaborator

alandtse commented Oct 8, 2023

  1. Yes.
  2. It should be only src since that's all the cpp code.
  3. Yes. I'll go ahead and merge this in as a partial fix for the issue since we make changes pretty fast and it will get stale if it's left out too long.

@alandtse alandtse changed the title chore: Break apart menu multiline strings chore: break apart menu multiline strings Oct 10, 2023
@alandtse alandtse merged commit 870cc64 into doodlum:dev Oct 10, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix menu multiline strings
3 participants