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

Better upgrade visibility in editor UI #4491

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Oliveriver
Copy link
Contributor

@Oliveriver Oliveriver commented Sep 22, 2023

Brief Description of What This PR Does

  • Displays upgrade icon in buttons for organelles that can be modified/upgraded
  • Displays modification notice in tooltips for organelles that can be modified/upgraded
  • Automatically opens the organelle modify/upgrade UI when a modifiable organelle is placed, under the following conditions:
    • Organelle was not placed with symmetry
    • Organelle placement tutorial is not active

Note this often results in the player having to undo twice if they want to remove a modifiable organelle due to #4091 which sadly still seems to be a bug.

image

Related Issues

Progress Checklist

Note: before starting this checklist the PR should be marked as non-draft.

  • PR author has checked that this PR works as intended and doesn't
    break existing features:
    https://wiki.revolutionarygamesstudio.com/wiki/Testing_Checklist
    (this is important as to not waste the time of Thrive team
    members reviewing this PR)
  • Initial code review passed (this and further items should not be checked by the PR author)
  • Functionality is confirmed working by another person (see above checklist link)
  • Final code review is passed and code conforms to the
    styleguide.

Before merging all CI jobs should finish on this PR without errors, if
there are automatically detected style issues they should be fixed by
the PR author. Merging must follow our
styleguide.

@buckly90 buckly90 added this to In progress in Thrive Planning via automation Sep 23, 2023
@buckly90 buckly90 added this to the Release 0.6.4 milestone Sep 23, 2023
@hhyyrylainen
Copy link
Member

Automatically opens the organelle modify/upgrade UI when a modifiable organelle is placed, under the following conditions:

I disagree with this being the default. I think fundamentally players should be fine with playing default organelle state for a while, and upgrades are mostly something different. So prompting immediately to modify a placed organelle doesn't make sense in a lot of cases. For example vacuoles, I don't think players would always want to specialize them, or for the pilus to convert it to deal toxin damage.

There could be an option in the options menu to enable this but I don't think it should be on by default.

@hhyyrylainen hhyyrylainen requested review from a team September 25, 2023 07:50
@Oliveriver
Copy link
Contributor Author

Fair enough. Would automatically opening the right click menu be a sensible compromise? Or should I just change the text in the tooltip to read 'Right click to modify once placed' and leave it as it is otherwise?

@hhyyrylainen
Copy link
Member

I'm not sure the menu opening would be that good flow either. At least it would require me extra clicks when building specific things to test. Also explicitly referring to right click is not a long term solution (I suggest using as neutral terminology as possible to avoid having a separate text for controller input): #4003

@Oliveriver
Copy link
Contributor Author

Right, but I'm just concerned that it's still obscuring information from the player if they aren't shown or told explicity how to modify the organelle. I'll remove the menu auto-opening for now so the tooltip and icon changes can at least go in.

Copy link
Member

@hhyyrylainen hhyyrylainen left a comment

Choose a reason for hiding this comment

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

Besides the translation conflict, I'd like to see a tiny bit of technical side clean up for this before merging.

@@ -260,6 +260,7 @@ private void UpdateOrganelleUnlockTooltips()
var tooltip = GetSelectionTooltip(organelle.InternalName, "organelleSelection");
if (tooltip != null)
{
tooltip.CanBeModified = organelle.CanBeModified;
Copy link
Member

Choose a reason for hiding this comment

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

Is this change actually required considering the code in SetupMicrobePartSelections?
If this is needed, then I think it might warrant renaming this method to something like UpdateOrganelleTooltips (as long as there isn't already a method with basically that name).

@@ -3892,6 +3892,10 @@ msgstr "Models"
msgid "MODIFY"
msgstr "Modify"

#: ../src/microbe_stage/editor/tooltips/SelectionMenuToolTip.tscn:163
msgid "MODIFY_ONCE_PLACED"
msgstr "Modify once placed"
Copy link
Member

Choose a reason for hiding this comment

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

Is it the best that this text tells the player "hey, modify this" instead of being like "Can be modified after placing"? I feel like that's a bit off in terms of tone in a general characteristics info box listing.

margin_bottom = 19.0
custom_colors/font_color = Color( 1, 0.898039, 0.337255, 1 )
custom_fonts/font = ExtResource( 10 )
text = "MODIFY_ONCE_PLACED"
Copy link
Member

Choose a reason for hiding this comment

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

I think the text key would be better to have something like "PART_IS_MODIFIABLE"

@@ -2015,6 +2010,7 @@ private void SetupMicrobePartSelections()
var control = (MicrobePartSelection)organelleSelectionButtonScene.Instance();
control.Locked = organelle.Unimplemented;
control.PartIcon = organelle.LoadedIcon ?? throw new Exception("Organelle with no icon");
control.CanBeModified = organelle.CanBeModified;
Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, now I remember that this was separate from the tooltip. So this is also needed.
I guess we decided that for now showing the modifiable icon on the organelles that can be modified is fine. It'll be easy enough to make this an option / disable it once almost every single organelle can be modified.

@revolutionary-bot
Copy link

We are currently in feature freeze until the next release.
If your PR is not just a simple fix, then it may take until the release to get reviewed and merged.

@hhyyrylainen hhyyrylainen removed this from the Release 0.6.5 milestone Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Thrive Planning
  
In progress
Development

Successfully merging this pull request may close these issues.

None yet

4 participants