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

[Attempt] Show Game Player Statistics during Gameplay #23

Merged
merged 5 commits into from
May 20, 2024

Conversation

sookmax
Copy link
Contributor

@sookmax sookmax commented May 18, 2024

Hello @cpojer! First of all, I would like to thank you for open-sourcing this amazing project 🤗

This is my attempt at #2.

I've decided to store fundsPerTurn in Player.stats.fundsPerTurn, though I'm not entirely sure that is the right place for it. And I've also decided to show the stats only when CurrentGameCard is expanded since there aren't enough spaces to fit the stats nicely when the card is collapsed.

Lastly, apart from the actual implementation, I added inset={100} to <MapEditor> in docs/content/examples/map-editor.tsx so that I can actually see the CurrentGameCard (it seems it was hidden under the docs header when inset = 0)

Screenshots

Screenshot 2024-05-18 at 8 03 58 PM Screenshot 2024-05-18 at 8 04 07 PM

@cpojer
Copy link
Contributor

cpojer commented May 19, 2024

Hello @sookmax 👋

Thank you for contributing to Athena Crisis. I like the solution using statistics, however I was thinking we don't need to cache any of this information at all and we can simply calculate it when showing the info by expanding the menu. Would you mind reverting the changes to the stats object and simply calculating it on-demand? Also note that createdBuildings and createdUnits is only for buildings/units created during gameplay, so the existing once when starting a game shouldn't count.

There is no limit on the size of this panel when it is expanded. I almost think it would be best if we put this info on the next line since the win condition stuff is also shown in that line and it could be too much with many conditions etc. What do you think?

@sookmax
Copy link
Contributor Author

sookmax commented May 19, 2024

@cpojer thanks for the quick feedbacks!

however I was thinking we don't need to cache any of this information at all and we can simply calculate it when showing the info by expanding the menu. Would you mind reverting the changes to the stats object and simply calculating it on-demand?

Ah I see. Since we have access to both map and player in <PlayerCard>, you mean you'd prefer calling calculateFunds() directly in <PlayerCard> whenever it's rendered perhaps?

Also note that createdBuildings and createdUnits is only for buildings/units created during gameplay, so the existing once when starting a game shouldn't count.

Got it. I was primarily working in the map editor UI in the docs, and it seemed reasonable to show the counts of existing buildings and units on the map when I hit the Playtest button—well on a second thought, that means the information shown in the map editor when play testing would be incorrect initially, would you be okay with that?

Oh and I also wanted to ask if there's any other place in the docs where I can see the <CurrentGameCard> other than the map editor? (the demo in the landing page doesn't seem to have it?)

There is no limit on the size of this panel when it is expanded. I almost think it would be best if we put this info on the next line since the win condition stuff is also shown in that line and it could be too much with many conditions etc.

Sounds about right to me too! So just to make sure we're on the same page, you want the new info line to be placed below the funds+win conditions line but above the charge gauge line right?


I'll work on your feedbacks and get back to you, thanks 😁

@sookmax
Copy link
Contributor Author

sookmax commented May 19, 2024

I've pushed 3f5a7ff containing the followings:

  • remove Player.stats.fundsPerTurn
  • render the stats in a new line
  • remove initial building & unit counts

Side notes

While I was re-organizing the dom structures for the stats, I realized the div with the playerInfoStyle is absolutely positioned, and so its height wasn't properly reflected on the parent div with playerStyle, resulting in the expanded card being cut off like this:

Screenshot 2024-05-19 at 5 56 48 PM

So I've added a resize observer that observes this absolutely positioned div with className={playerStyle} to correctly resize the parent div when the card is expanded:

Screen.Recording.2024-05-19.at.5.54.05.PM.mov

But I'm not entirely sure if it's the best way to solve the issue, so let me know what you think!

Comment on lines 332 to 335
? Math.max(
player.stats.createdUnits - player.stats.lostUnits,
0,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, maybe I wasn't explaining it well earlier. We shouldn't use stats at all, they are just for internal tracking.

For units and buildings we should use the map object and filter map.units and map.buildings for map.matchesPlayer(unit, player).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry my bad! I should've looked more into MapData before jumping into reading Player 😅

Another thing I've realized is that the initial stats for the map editor (when play-testing) now correctly reflect the number of units and buildings placed on the board!

Comment on lines 351 to 356
const icon =
key === 'fundsPerTurn'
? Coin
: key === 'netUnits'
? HumanHandsdown
: Buildings;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make the icon the third argument instead of checking here. Also, mind calling it just units and buildings instead of netUnits etc.?

@cpojer
Copy link
Contributor

cpojer commented May 19, 2024

I left a few more comments. If you could do one more pass on it, then I can take a look at the sizing issue.

@sookmax
Copy link
Contributor Author

sookmax commented May 20, 2024

@cpojer

I addressed your feedbacks in e2a9682, and it's now ready for a review 🫡

@cpojer cpojer merged commit 0f4aab6 into nkzw-tech:main May 20, 2024
2 checks passed
@cpojer
Copy link
Contributor

cpojer commented May 20, 2024

@sookmax Thank you so much for the contribution. The funds were sent to you. Please consider contributing again!

@sookmax
Copy link
Contributor Author

sookmax commented May 21, 2024

@cpojer Thanks for guiding me through this half-baked PR! I feel like I've only done half the work 😅. And I would love to contribute again!

A quick question if you don't mind:
What do you think about adding Storybook to the project?

@cpojer
Copy link
Contributor

cpojer commented May 21, 2024

@sookmax that's ok, it's a big codebase and it was fun to collaborate with you!

Re Storybook, I'm not too sure to be honest. I got the docs setup nicely now so that game components can be rendered inline in the docs. What exactly would you be interested in adding?

@sookmax
Copy link
Contributor Author

sookmax commented May 21, 2024

What exactly would you be interested in adding?

I think Storybook is great for developing / maintaining / testing UI components in isolation. It would also be much easier for external contributors to dive in and fix a specific issue in a specific UI component quickly.

And if set up correctly, it'd be much easier to see different states of a particular UI in Storybook. For example, while I was working on this PR, I wanted to render a win condition (any) on the player card so I could see how the new stats UI would fit between the funds and the win condition—being ignorant of how to trigger one of the win condition, I ended up temporarily enabling a condition (and commenting out the others) to see how it presents on the screen.

If setting up Storybook in the project is not too painful (given that the project uses Vite, I think there shouldn't be much frictions since Storybook supports Vite-based projects relatively well) I highly think it'd be worthwhile to have it. And once set up properly, writing stories can be as incremental/gradual as you want it to be. Actually I enjoy writing stories on UI components, so I could write them while learning how all these UI pieces work together and what data they need, etc.

@cpojer
Copy link
Contributor

cpojer commented May 21, 2024

I'd definitely be curious to explore what that might look like. In my experience Storybook can be kinda heavy and bring in a whole toolchain, but if it works with Vite it might be worth a try!

@sookmax
Copy link
Contributor Author

sookmax commented May 21, 2024

Haha hey that was fast 🤯. Glad you're interested!

In my experience Storybook can be kinda heavy and bring in a whole toolchain

Yeah I mean it's a mess actually.. I happened to have contributed to Storybook 7 a little last year, and there were just too many things broken (+tech debts).

But as a user (especially Vite-binding) with pretty basic use cases in mind, it's not too bad.

I have some mediocre examples deployed on Vercel

I was going to look into #17 next, but I could also look into Storybook setup first!

@cpojer
Copy link
Contributor

cpojer commented May 21, 2024

#17 is definitely more important, but it's also a big task. Up to you!

@sookmax
Copy link
Contributor Author

sookmax commented May 21, 2024

Yeah I figured #17 would take a while haha. I'll try that one first and bring up the Storybook discussion again after, then! (well if someone else beats me to it, then I can look into Storybook earlier 😂). But I'll give #17 a shot first.

cpojer added a commit that referenced this pull request May 21, 2024
Co-authored-by: cpojer <[email protected]>
GitOrigin-RevId: 5e3af9a90b7df7438f6efb9520e4f3b609bc8f88
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.

2 participants