OoTRandomizer / OoT-Randomizer

A randomizer for Ocarina of Time.
Other
401 stars 232 forks source link

Adult Trade Sequence Items - clarify SettingsList.py description #2259

Closed segue90 closed 2 weeks ago

segue90 commented 1 month ago

**initial post complete

I would like to help update the gui_tooltips to help better elaborate to users the intended functions of 'Adult Trade Shuffle' and 'Adult Trade Sequence Items'. Users unfamiliar with the specifics might assume the two options are mutually inclusive or confuse how they are dependent upon each other. The gui tooltip could be updated with minimal information necessary to help the user understand some of these specifics. Open to any feedback: I may be misunderstanding this feature set or the text below could be word-smithed better: thank you!

adult_trade_shuffle -------------------------- recommended gui_text & gui_tooltip:

Shuffle Multiple Adult Trade Items:

Enable to shuffle multiple adult trade sequence items. Functionality depends on 
'Shuffle Adult Trade Sequence Items' selected below. 

If disabled and at minimum one of 'Shuffle Adult Trade Sequence Items' is enabled, 
Anju will always give an item even if Pocket Egg is not shuffled. 

If enabled, but none of 'Shuffle Adult Trade Sequence Items' are enabled, this function
 has no effect on adult trade quest. 

adult_trade_start ---------------------------- recommended gui_text & gui_tooltip:

Shuffle Adult Trade Sequence Items:

Select the Adult Trade Sequence items to shuffle. 
If 'Shuffle Multiple Adult Trade Items' is enabled, all of the enabled items will be shuffled.
If 'Shuffle Multiple Adult Trade Items' is disabled, only one of the enabled items will be shuffled. 

---------- additional comments -----------

I'm enrolled in an Open Source class requiring we submit an issue through Git Hub first, then we submit a pull request. At minimum, I intend to submit a pull request to help clarify the function of those two affordances in the gui_tooltip. But, I am good to shift discussion over to discord because I think that's the preferred location for maintainers to discuss how features are intended to work.

mracsys commented 1 month ago

For reference, the current text and tooltips:

adult_trade_shuffle

Shuffle All Adult Trade Items Shuffle all adult trade sequence items. If disabled, a random item will be selected, and Anju will always give an item even if Pocket Egg is not shuffled.

adult_trade_start

Adult Trade Sequence Items Select the items to shuffle in the adult trade sequence.


These new descriptions are much clearer. Thanks for submitting!

The adult_trade_shuffle tooltip is on the longer side. Does it fit on mobile? See Notes/GUI/web-gui-testing.md for running the website locally.

With adult_trade_shuffle disabled, it's worth mentioning in the tooltip that all post-Anju-Pocket-Egg trade items can be collected from their vanilla sources even if it's chosen as the shuffled item. This is relevant for the reverting items (Odd Mushroom, Eyeball Frog, Eyedrops). I'm not sure what the best way to say that concisely looks like.

I would also add bold and/or italics to the settings names in the tooltips to distinguish them. HTML tags work, see dungeon_shortcuts_choice for an example.

It might be a good idea to swap the Multiple Adult Trade items enabled/disabled lines in the adult_trade_start tooltip. I think disabled is the most commonly used option in the community. Not a huge deal either way.

segue90 commented 1 month ago

Hi mracsys,

1st paragraph: thank you! 2nd paragraph: Let me compile on mobile and see how it looks. May take me a bit to understand all the dev kits. This is new to me.

3rd - 5th paragraph: I think you're saying, you want to also include a description of this type of situation:

Example: Odd Mushroom is acquired in a shuffled location. An action causes either the Odd Mushroom timer or the subsequent item (Odd Potion) timer to expire. The trade item in player's inventory reverts to the first non-timer item (Cojiro). The player can reacquire the Odd Mushroom at its non shuffled location (Lost Woods Trade with Grog).

Do I understand that correctly? I don't know all the ends and outs of the logic and safeguards, but this would be my attempt that gui tooltip, with your feedback added in as well.

    adult_trade_shuffle = Checkbutton(
        gui_text       = 'Shuffle Multiple Adult Trade Items:',
        gui_tooltip    = '''\
                Shuffle multiple Adult Trade Sequence items. 
                Must select minimum one of <b>Shuffle Adult Trade
                Sequence Items</b> to affect the Adult Trade Sequence. 

                If disabled and at minimum one of <b>Shuffle Adult 
                Trade Sequence Items</b> is selected, Anju will always
                give an item even if Pocket Egg is not shuffled. 
        ''',
        shared         = True,
        default        = False,
    )
    adult_trade_start = MultipleSelect(
        gui_text       = 'Shuffle Adult Trade Sequence Items:',
        gui_tooltip    = '''\
                Select the Adult Trade Sequence items to shuffle.

                If <b>Shuffle Multiple Adult Trade Items<b> is enabled,
                all selected items will be shuffled. 

                If <b>Shuffle Multiple Adult Trade Items<b> is disabled,
                only one of the selected items will be shuffled. If any 
                acquired item is replaced by its Adult Trade Sequence
                predecessor due to an expired timer or game reset, 
                the item can be reacquired at its non-shuffled location. 
        ''',
        shared         = True,
    )

"due to an expired timer or game reset" ?? = not sure if this section should be included, it might be too specific. Or if we should mention the game reset only applies to non-ER settings.

*truncated the actual item arrays for better visibility in this discussion

mracsys commented 1 month ago

Example: Odd Mushroom is acquired in a shuffled location. An action causes either the Odd Mushroom timer or the subsequent item (Odd Potion) timer to expire. The trade item in player's inventory reverts to the first non-timer item (Cojiro). The player can reacquire the Odd Mushroom at its non shuffled location (Lost Woods Trade with Grog).

Do I understand that correctly?

You've got it. Odd Potion does not have a timer though, only the mushroom.

In the new tooltips, your closing <b> tags need a slash, </b>.

I'd reword the last sentence in the last paragraph a bit to be

                If <b>Shuffle Multiple Adult Trade Items</b> is disabled,
                only one of the selected items will be shuffled. If the
                Odd Mushroom, Eyeball Frog, or Eyedrops revert due to
                an expired timer or game reset, the item can be
                reacquired at its non-shuffled location.

I don't think you need to mention what settings disable vanilla trade revert behavior. If the item doesn't revert, the player doesn't have to do anything.

You can open a draft PR anytime with your current proposed work. Github's code review features can add in-line comments instead of hashing everything out in quotes in this issue. If your professor prefers doing it all up front in the issue that works too.

segue90 commented 1 month ago

Wow. I never noticed odd potion doesn't have a timer.- thank you.

Good points on excluding detailed documentation on Entrance Randomizer. I thought about your points and I realize I was also conflating the scenarios tied to the "reverting" process... I think this is a bit more specific, but I really like you listing out the three specific items versus my using of overly general names. I see references to player's inventory in Gerudo Fortress settings so I think it doesn't jar too much from the other tooltips:

                If <b>Shuffle Multiple Adult Trade Items</b> is disabled,
                only one of the selected items will be shuffled. If the
                Odd Mushroom, Eyeball Frog, or Eyedrops is removed
                from the player's inventory due to an expired timer
                or game reset, that item can be reacquired at its 
                non-shuffled location.

Thank you for clarifying the html standards. I'll need to some time to understand the dev kits and test a bit before the PR to ensure I'm not missing something fundamental with the formatting


7-23 8pm cst just a quick update, I will do the dev testing this weekend/next week while I'm off work and do PR soon thereafter; I'm not sure what your policy is on open issues but I will try and close this asap

segue90 commented 1 month ago

closing issue. I feel like further discussion can be had on the PR if necessary

fenhl commented 1 month ago

Typically you'll want to keep an issue open until it's actually implemented. I've set it up so this issue is automatically closed once the PR is merged.

segue90 commented 1 month ago

Hey, sorry about that. I recall a ootr dev letting me know that having both an issue and a PR is somewhat superfluous. I interpreted that as don't clog up the randomizer with duplicate communications. Let me know if there's anything else I can do better moving forward :)