nkzw-tech / athena-crisis

Athena Crisis is an Advance Wars inspired modern-retro turn-based tactical strategy game. Athena Crisis is open core technology.
https://athenacrisis.com/open-source
Other
1.37k stars 105 forks source link

[Attempt] Show Game Player Statistics during Gameplay #23

Closed sookmax closed 1 month ago

sookmax commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

I've pushed https://github.com/nkzw-tech/athena-crisis/pull/23/commits/3f5a7ffa507456f55792f0ff1ffe6d06e5630490 containing the followings:

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:

https://github.com/nkzw-tech/athena-crisis/assets/71210554/9fba186b-09b1-4cac-8ac2-09aa11740fdf

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

cpojer commented 1 month ago

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 commented 1 month ago

@cpojer

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

cpojer commented 1 month ago

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

sookmax commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 https://github.com/nkzw-tech/athena-crisis/issues/17 next, but I could also look into Storybook setup first!

cpojer commented 1 month ago

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

sookmax commented 1 month ago

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.