sloukit / pydew-valley-uzh

Mod of Pydew Valley for an experimental study in Psychology
Creative Commons Zero v1.0 Universal
32 stars 61 forks source link

added round end screen #162

Closed ylhs-262912 closed 1 month ago

ylhs-262912 commented 2 months ago

Please go the the Preview tab and select the appropriate sub-template:

ylhs-262912 commented 2 months ago

Note: I asked sloukit independently what this feature wanted, as it was very unclear. here are those text convos: image image image image so the first item doesn't apply. However, all the other concerns are valid. Given me a bit.

DangerousVanilla commented 2 months ago

I'm not sure I'm getting the same message you are (that the first thing doesn't apply). @sloukit, could you please clarify the intent on the round-end screen for me? The feature description (updated just yesterday) says: "Create simple round ending screen after every level (i.e., after every 15 minutes); contains statistics on how many plants and fruit have been planted/harvested".

My question is only around the second bold portion - do the messages between you two shown above indicate that this portion of the feature isn't actually required?

ylhs-262912 commented 2 months ago

All changes have been made!

sloukit commented 2 months ago

Hi, sorry for the confusion. The idea with the end of the round screen is to have a clear cut after 15 minutes and show the player the amount of crops and wood they currently have. This is cumulative, i.e. what the player owns goes into the next level and should also be included in the statistics. A 0 can be displayed for the crops the player doesn't have. I'm not sure I fully understand your question. Does this make it a bit clearer?

DangerousVanilla commented 2 months ago

Yes and no. Sorry if I'm seeming a bit dense here but I'm seeing multiple mechanics at play (starting inventory, buying/selling, planting/harvesting) and potential things to track and I'm affraid part of it is getting confused due to some potentially unfinished features and/or testing variables.

You start with some of everything currently but can then technically gain or lose items through buying, selling, planting, or harvesting. Lets just take your 5 starting corn seeds as an example and go through a few scenarios to clarify what should be displayed at the end of a round...

Scenario 1:

  1. You start with 5 corn seeds.
  2. You do nothing for the entire round and end with the same 5 corn seeds. Result opt. 1: You still have 5 corn seeds in your inventory so the round ending screen displays 5 corn seeds. Result opt. 2: You didn't plant or harvest anything so the round ending screen shows 0 corn seeds.

Scenario 2:

  1. You start with 5 corn seeds
  2. You plant 3 of those corn seeds then wait for the round to end doing nothing else. Result opt. 1: The round end screen shows 2 corn seeds because you have 2 in your inventory (having planted 3). Result opt. 2: The round end screen shows 3 corn seeds because you planted 3 (2 remained in your inventory). Result opt. 3: The round end screen shows 5 corn seeds because you still 'own' 5 corn seeds worth (2 in inventory and 3 planted) Result opt. 4: The round end screen shows seperate categories for current inventory (2) and planted values (3), totaling to 5.

Scenario 3:

  1. You start with 5 corn seeds
  2. You go buy 45 more corn seeds so you have 50 total in your inventory.
  3. You go plant 20 of these and realize your farm is full so you have no current use for the other 30 corn seeds.
  4. You go back to the shop and sell 20 of those remaining 30 corn seeds leaving you with 10 in your inventory. Result opt. 1: Round end shows 10 corn seeds because that's what you end with in your inventory. Result opt. 2: Round end shows 30 corn seeds because you ended with 10 in inventory and 20 planted, meaning you currently 'own' 30 corn seeds worth. This would also indicate it doesn't matter what you buy/sell, just what you end the round with in inventory+planted/harvested. Result opt. 3: Round end shows 20 corn seeds because you planted 20 corn seeds. Result opt. 4: Round end shows 50 corn seeds because you had up to 50 in your inventory earlier and it doesn't keep track of what you sell. Result opt. 5: Round end shows seperate categories for your invenotry and planted values, displaying 10 in inventory and 20 planted. Result opt. 6: Round end tracks everything seperately (purchased, sold, ending inventory, planted, harvested) and shows them all and a calculated balance. If this was the case, would the balance show 10 (meaning it only cares about your inventory) 20 meaning it only cares about your planted/havested amounts which would be cummulative over the rounds as you mentioned, keeping track of total progress), 30 meaning it cares about your total current ownership (could still count planted/harvested cummulatively on this but adds your inventory value on top because you 'own' those) or something else entirely?

So that might seem like a lot but hopefully it illustrates my questions around what exactlly is supposed to be tracked/shown...

All that aside, the actual issue I was pointing out is simply that the values being displayed on the round-end screen are just wrong no matter how you look at it, but to fix that properly I think it will be important to clarify the intended outcome of the scenarios above if you don't mind. The image below illustrates the actual issue I was trying to get at in this commit as is. It tracks two entire rounds of play testing with screenshots and explainations along the way: round_end_inventory_issues

It occurs to me when looking at the compilation of screenshots here that the round end screen's text alignment should look more like that of the shop (shown in screenshot 2). Notice how there's padding on the left side rather than the text and box starting in the same position? It would also be more cohesive to display the numbers in the same boxes right justified like the shop does but nobody has laid out a strict requirement for that so it's more of just a suggestion for visual continuity.

ylhs-262912 commented 2 months ago

Round end menu now exactly copies inventory. Problem was it wasn't updating everything it was called, but only when it was initiated. It should fit at least 1 of the options now, however we need sloukit for further verification

sloukit commented 2 months ago

Thank you for the detailed description of your concerns. They are valid, and I'm sorry for the confusion and for not getting it sooner.

The end of round screen copies the inventory. The seeds that are currently planted (i.e. potential future fruit/potential money or former seeds) don't matter. The function of this end-of-round screen is not extremely important, it's more about having a clear signal that the round has ended and the next round will start now, and showing the player what they have in their inventory in case they never looked. Does this make sense to you? Please let me know if you have any other questions or concerns.

sloukit commented 1 month ago

This PR can be merged, but there are some conflicts between it and the main branch. @ylhs-262912 could you check this?

DangerousVanilla commented 1 month ago

@ylhs-262912, thanks for the quick fix. I'm all cleared up now on sloukit's direction and the needs of the screen and it looks like the inventory matching is perfect. Your effort and attention here is greatly appreciated!

ylhs-262912 commented 1 month ago

Ok, I'll resolve conflicts this afternoon.

ylhs-262912 commented 1 month ago

Fixed the conflicts! please acknowledge the changes have passed @sloukit and @DangerousVanilla so the merge can go through.

sloukit commented 1 month ago

There is a problem with the style-lint check, could you check it again?

ylhs-262912 commented 1 month ago

try it now!

sloukit commented 1 month ago

We can merge this branch once @DangerousVanilla has approved the changes

DangerousVanilla commented 1 month ago

@ylhs-262912, it looks like the boxes in the scrollable area aren't being cleared anymore as the screen is scrolled, would you mind taking a look at that please? Here's a short clip for reference:

https://github.com/user-attachments/assets/328a87b8-a041-4591-b381-1e91103630b2

ylhs-262912 commented 1 month ago

Okay, I'll do it this afternoon

ylhs-262912 commented 1 month ago

Done! try it now.

ylhs-262912 commented 1 month ago

oop found a bug in the scrolling

ylhs-262912 commented 1 month ago

fixed the scrolling bug. yip

ylhs-262912 commented 1 month ago

Why did the lint fail this time?

DangerousVanilla commented 1 month ago

Taking a look at this now - I'll get you some details

DangerousVanilla commented 1 month ago

So after working around a long standing bug to actually get your latest updates I reran formatlint.py and all it did was remove a single blank line around line 46-47 in menu_round_end.py.

That said, it looks like you're not quite up to date with main (there's still a lot of references to set_token_status in your commit. If you wouldn't mind syncing up with main, addressing any conflicts, and finally running formatlint.py again I think it'll have your all sorted out.

ylhs-262912 commented 1 month ago

Okay, I'll do that.

ylhs-262912 commented 1 month ago

oop its because I forgot to push a commit. that's where this all came from. oof. Tell me if there are anymore leftover references, I don't believe so.

sloukit commented 1 month ago

Looks good now. Can you approve, @DangerousVanilla?

tomkommando commented 1 month ago

Hey there,

Great stuff. However, seems the changes in Character.draw method in character.py draw method cause some unexpected behaviour.

I started to write a bug report, but dug a bit deeper and think this merge has changed character drawing logic. So instead of new issue I copy paste it below:

Bug Description

The necklace is always drawn on ingroup NPCs - regardless of self.has_necklace == True or False

Expected Behavior

Those NPCs who have "has_necklace == False" should not have necklace drawn on them.

Steps to Reproduce

  1. make sure there are NPCs without necklace. For example, tweak method in NPC.assign_outfit_ingroup so that no NPC has necklace
  2. run the game and observe the necklace is drawn on every ingroup NPC anyway

Additional Context

This worked OK yesterday. The bug is very recent.

DangerousVanilla commented 1 month ago

@tomkommando, thank you for noticing the issue here - I'll look into it and see if I can fix that issue.