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

Bubble of a growing population #627

Merged
merged 26 commits into from
Nov 16, 2023
Merged

Bubble of a growing population #627

merged 26 commits into from
Nov 16, 2023

Conversation

kkosiorowska
Copy link
Contributor

@kkosiorowska kkosiorowska commented Nov 3, 2023

Closes #607

What else should be done

  • Setting the correct animation for bubbles.
  • If possible, add animations for RealmsBar. RealmBarIcon should move more smoothly when the population is changed.
  • After joining the realm, show a bubble instantly.
  • Improve intervals for realms.
  • Adding bubble to the modal window.

What

The app should be more lively. This PR adds +1 bubble over a realm every time somebody joins. The bubble should be visible over the realm on the map but also in the modal window. The population is fetched from the contracts every minute. We shouldn't show 20 bubbles at the same time and then not show anything for 1 minute. We should spread out the number of bubbles we show.

  • So let's add a variable displayedPopulation to increment it and show the bubbles with the correct interval.
  • Intervals should be different for the realms to make the app more lively.
  • When we init game data, we should refresh the displayedPopulation and align it with the true number.

Testing

Let's use a patch file to increase the population and do tests. Use the following file:
increase-population.patch The population is increased randomly every minute.

git apply increase-population.patch
  • When a user joins the region, a bubble should show up immediately in the modal.
  • Bubbles should occur at different intervals between realms.
  • After refreshing the page, the population should show the correct number.

@kkosiorowska kkosiorowska self-assigned this Nov 3, 2023
Copy link

netlify bot commented Nov 3, 2023

Deploy Preview for taho-development ready!

Name Link
🔨 Latest commit 4851cbe
🔍 Latest deploy log https://app.netlify.com/sites/taho-development/deploys/6554dc2f6cdb7900089fde98
😎 Deploy Preview https://deploy-preview-627--taho-development.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@xpaczka xpaczka marked this pull request as ready for review November 8, 2023 08:44
Copy link
Contributor

@jagodarybacka jagodarybacka left a comment

Choose a reason for hiding this comment

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

Huh that is a big one 😅 Haven't tested it manually yet, some thoughts below

src/redux-state/selectors/population.ts Outdated Show resolved Hide resolved
src/redux-state/utils/population.ts Outdated Show resolved Hide resolved
src/shared/constants/partners.ts Outdated Show resolved Hide resolved
src/shared/utils/misc.ts Outdated Show resolved Hide resolved
src/ui/Island/Realm.tsx Outdated Show resolved Hide resolved
src/ui/Footer/RealmBar/RealmBarIcon.tsx Outdated Show resolved Hide resolved
src/shared/hooks/population.ts Outdated Show resolved Hide resolved
src/shared/hooks/population.ts Outdated Show resolved Hide resolved
src/redux-state/thunks/island.ts Outdated Show resolved Hide resolved
jagodarybacka
jagodarybacka previously approved these changes Nov 9, 2023
Copy link
Contributor

@jagodarybacka jagodarybacka left a comment

Choose a reason for hiding this comment

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

Overall it is working fine but I would encourage @VladUXUI and @andreachapman to QA it manually a bit (ideally with two browser windows open so you can "emulate" changes in populations). Or I can setup the environment so you can take a look at live on-chain population changes.

What I don't like here is:
we were trying very hard to tie the bubbles to the population chart so when bubble appears the population chart is updated as well - this seems good idea in theory but as population is fetched every X seconds we will have situations where population chart is lagging behind the real values (will be more visible when more people are joining the realm all at once).
This is not a huge deal but tbh our goal was to make the map more alive. We can achieve that just by showing bubbles and the population chart can show the real data as soon as it gets the update. Without making connection +1 bubble means +1 on the population chart.

Let me know guys what you think about it.

@andreachapman
Copy link
Contributor

Overall it is working fine but I would encourage @VladUXUI and @andreachapman to QA it manually a bit (ideally with two browser windows open so you can "emulate" changes in populations). Or I can setup the environment so you can take a look at live on-chain population changes.

@jagodarybacka - Happy to take a look (but it'll probably be tomorrow before I can dig in too deep). The one thing I think though is that the experience might be quite different in an environment with only a few accounts stakes versus the more prod-like scenario. So because of that, I do like the idea of having some way to look at live population changes. I think that'll give us the best view of it. So if that's something that could happen before I come online tomorrow then I can spend some time looking at it first thing.

@jagodarybacka
Copy link
Contributor

@andreachapman @xpaczka I've setup a live chain branch https://population-bubble--taho-development.netlify.app/ and I already see there is something off there, population is not updating correctly for me 😞

Screen.Recording.2023-11-09.at.17.06.08.mov

@xpaczka
Copy link
Contributor

xpaczka commented Nov 9, 2023

@andreachapman @xpaczka I've setup a live chain branch https://population-bubble--taho-development.netlify.app/ and I already see there is something off there, population is not updating correctly for me 😞

I can't see any issues (video and live), so I must be missing something

@andreachapman
Copy link
Contributor

https://population-bubble--taho-development.netlify.app/

Before you connect the wallet it shows like 31,000 and then when you connect and see the map, I see two weird things:

  1. The population bar takes a few seconds to come up - it's like it doesn't come up til a new account stakes
  2. It seems like it's not showing the entire population, only that since you connected your wallet. Maybe? It's definitely showing far less than the population count on the portal screen
Screen.Recording.2023-11-09.at.11.19.48.AM.mov

@xpaczka
Copy link
Contributor

xpaczka commented Nov 9, 2023

https://population-bubble--taho-development.netlify.app/

Before you connect the wallet it shows like 31,000 and then when you connect and see the map, I see two weird things:

  1. The population bar takes a few seconds to come up - it's like it doesn't come up til a new account stakes
  2. It seems like it's not showing the entire population, only that since you connected your wallet. Maybe? It's definitely showing far less than the population count on the portal screen
  1. I'll have a look at the condition that is placed on the population bar, it may be incorrect
  2. The issue here is probably, that it start counting from zero to the target population now (31k). It didn't happen in the local development, so that's why I haven't caught that one.

I will dig into it starting tomorrow morning.

jagodarybacka
jagodarybacka previously approved these changes Nov 10, 2023
Copy link
Contributor

@jagodarybacka jagodarybacka left a comment

Choose a reason for hiding this comment

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

Looks ok but let's merge after manual QA will be accepted.

I also think we could fine tune the algorithm of showing bubbles so it is showing them a bit quicker.
We can safely make the interval between population fetches for example ~15-20s and the bubbles can be showing one by one or so. Because if more people will be joining realms then we will be very behind with displayed population as we are adding just 1 person every 5-15 seconds, right?
(not a blocker, we can improve later if reloading the website will show most recent population)

@andreachapman
Copy link
Contributor

I've watched this for a while this morning, including while staking from another browser, and it looks like it's working fine. I think Vlad still wanted some time to test it out too though (from what I heard on the sprint planning call yesterday).

@VladUXUI
Copy link
Contributor

Been looking at it for 10-15 min and i can't really tell tbh, population only went down :))
I think the implementation is spot on, but it's not giving that lively effect that we were looking for, and this is because activity is low. Although this isn't the result we hoped for, i still think it's going to be a great liveliness factor for when we go to mainnet

There might be some UI tweaks that we need to do (bubbles aren't really all that visible unless you know to look/wait for them)

But if you all are ok, i would like to ticket that later.

My advice is to ship this as is, and iterate on it more in a future date if necessary.

(While writing this, a bubble showed up ❤️ )

@jagodarybacka
Copy link
Contributor

I'll give it some time today/tomorrow to see if anything can be done quickly. If not let's ship it.

@jagodarybacka
Copy link
Contributor

How often are we fetching population here? can we do it every 10/15s? That will be the quickest thing to improve the probablity of noticing the bubbles :P

@xpaczka
Copy link
Contributor

xpaczka commented Nov 15, 2023

How often are we fetching population here? can we do it every 10/15s? That will be the quickest thing to improve the probablity of noticing the bubbles :P

It is randomly between 5 and 15 seconds

@jagodarybacka
Copy link
Contributor

@xpaczka https://github.com/tahowallet/dapp/blob/population-bubble/src/shared/hooks/wallets.ts#L122 I was thinking about making this time ~10-15s, it is still 60s

Copy link
Contributor

@jagodarybacka jagodarybacka left a comment

Choose a reason for hiding this comment

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

Let's ship it 🚢

@jagodarybacka jagodarybacka merged commit 28b01f4 into main Nov 16, 2023
4 checks passed
@jagodarybacka jagodarybacka deleted the population-bubble branch November 16, 2023 13:24
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.

Add new citizen over realms
5 participants